Don't create duplicated frames in the stack for Chrome JS.

These duplicated frames are only useful if the native frame has extra information, like the JIT tier. But Chrome ETW JIT frames don't have that information.
mstange · Jul 17, 2024 · a81109e · a81109e
1 parent 570e276
commit a81109e
Show file tree

Hide file tree

Showing 3 changed files with 26 additions and 19 deletions.
diff --git a/samply/src/shared/jit_category_manager.rs b/samply/src/shared/jit_category_manager.rs
@@ -4,7 +4,9 @@ use fxprof_processed_profile::{
 
 #[derive(Debug, Clone, Copy)]
 pub enum JsFrame {
-    Regular(JsName),
+    #[allow(dead_code)]
+    NativeFrameIsJs,
+    RegularInAdditionToNativeFrame(JsName),
     BaselineInterpreterStub(JsName),
     BaselineInterpreter,
 }
@@ -134,7 +136,8 @@ impl JitCategoryManager {
         if let Some(ion_ic_rest) = name.strip_prefix("IonIC: ") {
             let category = self.ion_ic_category.get(profile);
             if let Some((_ic_type, js_func)) = ion_ic_rest.split_once(" : ") {
-                let js_func = JsFrame::Regular(Self::intern_js_name(profile, js_func));
+                let js_func =
+                    JsFrame::RegularInAdditionToNativeFrame(Self::intern_js_name(profile, js_func));
                 return (category.into(), Some(js_func));
             }
             return (category.into(), None);
@@ -143,7 +146,8 @@ impl JitCategoryManager {
         if let Some(ion_ic_rest) = name.strip_prefix("IonIC: ") {
             let category = self.ion_ic_category.get(profile);
             if let Some((_ic_type, js_func)) = ion_ic_rest.split_once(" : ") {
-                let js_func = JsFrame::Regular(Self::intern_js_name(profile, js_func));
+                let js_func =
+                    JsFrame::RegularInAdditionToNativeFrame(Self::intern_js_name(profile, js_func));
                 return (category.into(), Some(js_func));
             }
             return (category.into(), None);
@@ -156,10 +160,9 @@ impl JitCategoryManager {
                 let category = lazy_category_handle.get(profile);
 
                 let js_name = if is_js {
-                    Some(JsFrame::Regular(Self::intern_js_name(
-                        profile,
-                        name_without_prefix,
-                    )))
+                    Some(JsFrame::RegularInAdditionToNativeFrame(
+                        Self::intern_js_name(profile, name_without_prefix),
+                    ))
                 } else {
                     None
                 };
@@ -185,7 +188,9 @@ impl JitCategoryManager {
                 if let Some((v8_wasm_name, func_index)) = v8_wasm_name_with_index.rsplit_once('-') {
                     let new_name = format!("{v8_wasm_name} (WASM:{func_index})");
                     let category = category.get(profile);
-                    let js_func = JsFrame::Regular(Self::intern_js_name(profile, &new_name));
+                    let js_func = JsFrame::RegularInAdditionToNativeFrame(Self::intern_js_name(
+                        profile, &new_name,
+                    ));
                     return (category.into(), Some(js_func));
                 }
             }
@@ -269,7 +274,7 @@ mod test {
             &mut profile,
         );
         match js_name {
-            Some(JsFrame::Regular(JsName::NonSelfHosted(s))) => {
+            Some(JsFrame::RegularInAdditionToNativeFrame(JsName::NonSelfHosted(s))) => {
                 assert_eq!(profile.get_string(s), "AccessibleButton (main.js:3560:25)")
             }
             _ => panic!(),

diff --git a/samply/src/shared/stack_converter.rs b/samply/src/shared/stack_converter.rs
@@ -213,13 +213,17 @@ impl<I: Iterator<Item = SecondPassFrameInfo>> Iterator for ConvertedStackIter<I>
         // Usually, a BaselineInterpreter frame is directly preceded by a BaselineInterpreterStub frame.
         // However, sometimes you get Regular(x) -> None -> None -> None -> BaselineInterpreter,
         // without a BaselineInterpreterStub frame. In that case, the name "x" from the ancestor
-        // JsFrame::Regular (which is really an InterpreterStub frame for the C++ interpreter)
+        // JsFrame::RegularInAdditionToNativeFrame (which is really an InterpreterStub frame for the C++ interpreter)
         // should be used for the BaselineInterpreter frame. This will create a stack
         // node with the right name, category and JS-only flag, and helps with correct attribution.
         // Unfortunately it means that we'll have two prepended JS label frames for the same function
         // in that case, but that's still better than accounting those samples to the wrong JS function.
-        let js_name = match js_frame {
-            Some(JsFrame::Regular(js_name)) => {
+        let extra_js_name = match js_frame {
+            Some(JsFrame::NativeFrameIsJs) => {
+                frame_info.flags |= FrameFlags::IS_JS;
+                None
+            }
+            Some(JsFrame::RegularInAdditionToNativeFrame(js_name)) => {
                 // Remember the name for a potentially upcoming unnamed BaselineInterpreter frame.
                 self.js_name_for_baseline_interpreter = Some(js_name);
                 Some(js_name)
@@ -233,7 +237,7 @@ impl<I: Iterator<Item = SecondPassFrameInfo>> Iterator for ConvertedStackIter<I>
             None => None,
         };
 
-        if let Some(JsName::NonSelfHosted(js_name)) = js_name {
+        if let Some(JsName::NonSelfHosted(js_name)) = extra_js_name {
             // Prepend a JS frame.
             // We don't treat Spidermonkey "self-hosted" functions as JS (e.g. filter/map/push).
             let prepended_js_frame = FrameInfo {

diff --git a/samply/src/windows/profile_context.rs b/samply/src/windows/profile_context.rs
@@ -17,7 +17,7 @@ use crate::shared::context_switch::{
     ContextSwitchHandler, OffCpuSampleGroup, ThreadContextSwitchData,
 };
 use crate::shared::included_processes::IncludedProcesses;
-use crate::shared::jit_category_manager::{JitCategoryManager, JsFrame, JsName};
+use crate::shared::jit_category_manager::{JitCategoryManager, JsFrame};
 use crate::shared::jit_function_add_marker::JitFunctionAddMarker;
 use crate::shared::jit_function_recycler::JitFunctionRecycler;
 use crate::shared::lib_mappings::{LibMappingAdd, LibMappingInfo, LibMappingOp, LibMappingOpQueue};
@@ -1566,15 +1566,13 @@ impl ProfileContext {
                 // For now we just add the script URL at the end of the function name.
                 // In the future, we should store the function name and the script URL
                 // separately in the profile.
-                method_name.push(' ');
-                method_name.push_str(url);
+                use std::fmt::Write;
+                write!(&mut method_name, " {url}").unwrap();
                 if line != 0 {
-                    use std::fmt::Write;
                     write!(&mut method_name, ":{line}:{column}").unwrap();
                 }
-                let s = self.profile.intern_string(&method_name);
                 let category = self.js_jit_lib.default_category();
-                let js_frame = Some(JsFrame::Regular(JsName::NonSelfHosted(s)));
+                let js_frame = Some(JsFrame::NativeFrameIsJs);
                 (category, js_frame)
             }
         } else {