Improve hermes profile frame keys to better group frames (#459)

Because all args are serialized in the key for hermes profiles, frames were not properly getting grouped since the "parent" property was different. This PR ensures that the frame key is properly constructed from the function name, file name, line + column number. | Before | After | | ----- | ----- | | <img width="1727" alt="Screenshot 2023-12-27 at 2 25 32 PM" src="https://github.com/jlfwong/speedscope/assets/9957046/4ec04653-5a80-4f34-9506-48e0eb415983"> | <img width="1728" alt="Screenshot 2023-12-27 at 2 26 02 PM" src="https://github.com/jlfwong/speedscope/assets/9957046/c3edc447-e8ad-4757-8576-cbb8c27eafe3"> | | <img width="1720" alt="Screenshot 2023-12-27 at 2 13 37 PM" src="https://github.com/jlfwong/speedscope/assets/9957046/10194aba-8f26-48ca-bf15-06917b44ea99"> | <img width="1728" alt="Screenshot 2023-12-27 at 2 14 20 PM" src="https://github.com/jlfwong/speedscope/assets/9957046/75c32401-1705-4b23-b71a-5f0a720160dd"> |
jlfwong · Dec 28, 2023 · e9a17d5 · e9a17d5
1 parent 3f3da22
commit e9a17d5
Show file tree

Hide file tree

Showing 4 changed files with 210 additions and 7 deletions.
diff --git a/sample/profiles/trace-event/hermes-multiple-frames.json b/sample/profiles/trace-event/hermes-multiple-frames.json
@@ -0,0 +1,156 @@
+[
+  {
+    "pid":0,
+    "tid":0,
+    "ph":"B",
+    "name":"[root]",
+    "ts":0,
+    "args":{
+      "name":"[root]",
+      "category":"root",
+      "url":null,
+      "line":null,
+      "column":null,
+      "params":null,
+      "allocatedCategory":"root",
+      "allocatedName":"[root]"
+    }
+  },
+  {
+    "pid":0,
+    "tid":0,
+    "ph":"B",
+    "name":"beta",
+    "ts":1,
+    "args":{
+      "line":54,
+      "column":12,
+      "funcLine":"1",
+      "funcColumn":"1",
+      "name":"beta",
+      "category":"JavaScript",
+      "parent":1,
+      "url":"/Users/example/test_project/node_modules/metro-runtime/src/polyfills/require.js",
+      "params":null,
+      "allocatedCategory":"JavaScript",
+      "allocatedName":"beta"
+    }
+  },
+  {
+    "pid":0,
+    "tid":0,
+    "ph":"E",
+    "name":"beta",
+    "ts":2,
+    "args":{
+      "line":54,
+      "column":12,
+      "funcLine":"1",
+      "funcColumn":"1",
+      "name":"beta",
+      "category":"JavaScript",
+      "parent":1,
+      "url":"/Users/example/test_project/node_modules/metro-runtime/src/polyfills/require.js",
+      "params":null,
+      "allocatedCategory":"JavaScript",
+      "allocatedName":"beta"
+    }
+  },
+  {
+    "pid":0,
+    "tid":0,
+    "ph":"B",
+    "name":"gamma",
+    "ts":4,
+    "args":{
+      "line":54,
+      "column":12,
+      "funcLine":"1",
+      "funcColumn":"1",
+      "name":"beta",
+      "category":"blah",
+      "parent":1,
+      "url":"/Users/example/test_project/node_modules/metro-runtime/src/polyfills/require.js",
+      "params":null,
+      "allocatedCategory":"JavaScript",
+      "allocatedName":"beta"
+    }
+  },
+  {
+    "pid":0,
+    "tid":0,
+    "ph":"E",
+    "name":"gamma",
+    "ts":13,
+    "args":{
+      "line":54,
+      "column":12,
+      "funcLine":"1",
+      "funcColumn":"1",
+      "name":"beta",
+      "category":"blah",
+      "parent":1,
+      "url":"/Users/example/test_project/node_modules/metro-runtime/src/polyfills/require.js",
+      "params":null,
+      "allocatedCategory":"JavaScript",
+      "allocatedName":"beta"
+    }
+  },
+  {
+    "pid":0,
+    "tid":0,
+    "ph":"B",
+    "name":"beta",
+    "ts":4,
+    "args":{
+      "line":54,
+      "column":12,
+      "funcLine":"1",
+      "funcColumn":"1",
+      "name":"beta",
+      "category":"blah",
+      "parent":1,
+      "url":"/Users/example/test_project/node_modules/metro-runtime/src/polyfills/require.js",
+      "params":null,
+      "allocatedCategory":"JavaScript",
+      "allocatedName":"beta"
+    }
+  },
+  {
+    "pid":0,
+    "tid":0,
+    "ph":"E",
+    "name":"beta",
+    "ts":10,
+    "args":{
+      "line":54,
+      "column":12,
+      "funcLine":"1",
+      "funcColumn":"1",
+      "name":"beta",
+      "category":"blah",
+      "parent":1,
+      "url":"/Users/example/test_project/node_modules/metro-runtime/src/polyfills/require.js",
+      "params":null,
+      "allocatedCategory":"JavaScript",
+      "allocatedName":"beta"
+    }
+  },
+  {
+    "pid":0,
+    "tid":0,
+    "ph":"E",
+    "name":"[root]",
+    "ts":14,
+    "args":{
+      "name":"[root]",
+      "category":"root",
+      "url":null,
+      "line":null,
+      "column":null,
+      "params":null,
+      "allocatedCategory":"root",
+      "allocatedName":"[root]"
+    }
+  }
+]
diff --git a/src/import/__snapshots__/trace-event.test.ts.snap b/src/import/__snapshots__/trace-event.test.ts.snap
@@ -404,6 +404,41 @@ exports[`importTraceEvents event reordering name match: indexToView 1`] = `0`;
 
 exports[`importTraceEvents event reordering name match: profileGroup.name 1`] = `"event-reordering-name-match.json"`;
 
+exports[`importTraceEvents hermes profile with multiple frames 1`] = `
+Object {
+  "frames": Array [
+    Frame {
+      "col": null,
+      "file": null,
+      "key": "[root]:null:null:null",
+      "line": null,
+      "name": "[root]",
+      "selfWeight": 2,
+      "totalWeight": 14,
+    },
+    Frame {
+      "col": 12,
+      "file": "/Users/example/test_project/node_modules/metro-runtime/src/polyfills/require.js",
+      "key": "beta:/Users/example/test_project/node_modules/metro-runtime/src/polyfills/require.js:54:12",
+      "line": 54,
+      "name": "beta",
+      "selfWeight": 12,
+      "totalWeight": 12,
+    },
+  ],
+  "name": "pid 0, tid 0",
+  "stacks": Array [
+    "[root] 1.00µs",
+    "[root];beta 12.00µs",
+    "[root] 1.00µs",
+  ],
+}
+`;
+
+exports[`importTraceEvents hermes profile with multiple frames: indexToView 1`] = `0`;
+
+exports[`importTraceEvents hermes profile with multiple frames: profileGroup.name 1`] = `"simple-hermes.json"`;
+
 exports[`importTraceEvents invalid x nesting 1`] = `
 Object {
   "frames": Array [
@@ -1321,7 +1356,7 @@ Object {
     Frame {
       "col": null,
       "file": null,
-      "key": "[root] {\\"name\\":\\"[root]\\",\\"category\\":\\"root\\",\\"url\\":null,\\"line\\":null,\\"column\\":null,\\"params\\":null,\\"allocatedCategory\\":\\"root\\",\\"allocatedName\\":\\"[root]\\"}",
+      "key": "[root]:null:null:null",
       "line": null,
       "name": "[root]",
       "selfWeight": 2,
@@ -1330,7 +1365,7 @@ Object {
     Frame {
       "col": 12,
       "file": "/Users/example/test_project/node_modules/metro-runtime/src/polyfills/require.js",
-      "key": "beta {\\"line\\":54,\\"column\\":12,\\"funcLine\\":\\"1\\",\\"funcColumn\\":\\"1\\",\\"name\\":\\"beta\\",\\"category\\":\\"JavaScript\\",\\"parent\\":1,\\"url\\":\\"/Users/example/test_project/node_modules/metro-runtime/src/polyfills/require.js\\",\\"params\\":null,\\"allocatedCategory\\":\\"JavaScript\\",\\"allocatedName\\":\\"beta\\"}",
+      "key": "beta:/Users/example/test_project/node_modules/metro-runtime/src/polyfills/require.js:54:12",
       "line": 54,
       "name": "beta",
       "selfWeight": 12,

diff --git a/src/import/trace-event.test.ts b/src/import/trace-event.test.ts
@@ -127,6 +127,10 @@ test('importTraceEvents simple hermes profile', async () => {
   await checkProfileSnapshot('./sample/profiles/trace-event/simple-hermes.json')
 })
 
+test('importTraceEvents hermes profile with multiple frames', async () => {
+  await checkProfileSnapshot('./sample/profiles/trace-event/simple-hermes.json')
+})
+
 test('importTraceEvents simple profile with samples', async () => {
   await checkProfileSnapshot('./sample/profiles/trace-event/simple-with-samples.json')
 })

diff --git a/src/import/trace-event.ts b/src/import/trace-event.ts
@@ -185,7 +185,7 @@ function selectQueueToTakeFromNext(
   // to ensure it opens before we try to close it.
   //
   // Otherwise, process the 'E' queue first.
-  return keyForEvent(bFront) === keyForEvent(eFront) ? 'B' : 'E'
+  return getEventId(bFront) === getEventId(eFront) ? 'B' : 'E'
 }
 
 function convertToEventQueues(events: ImportableTraceEvent[]): [BTraceEvent[], ETraceEvent[]] {
@@ -309,7 +309,13 @@ function getEventName(event: TraceEvent): string {
   return `${event.name || '(unnamed)'}`
 }
 
-function keyForEvent(event: TraceEvent): string {
+/**
+ * Attempt to construct a unique identifier for an event. Note that this
+ * is different from the frame key, as in some cases we don't want to include
+ * some arguments to allow from frame grouping (e.g. parent in the case of
+ * hermes profiles)
+ */
+function getEventId(event: TraceEvent): string {
   let key = getEventName(event)
   if (event.args) {
     key += ` ${JSON.stringify(event.args)}`
@@ -321,20 +327,22 @@ function frameInfoForEvent(
   event: TraceEvent,
   exporterSource: ExporterSource = ExporterSource.UNKNOWN,
 ): FrameInfo {
-  const key = keyForEvent(event)
-
   // In Hermes profiles we have additional guaranteed metadata we can use to
   // more accurately populate profiles with info such as line + col number
   if (exporterSource === ExporterSource.HERMES) {
+    const hermesFrameKey = `${event.name}:${event.args.url}:${event.args.line}:${event.args.column}`
+
     return {
       name: getEventName(event),
-      key: key,
+      key: hermesFrameKey,
       file: event.args.url,
       line: event.args.line,
       col: event.args.column,
     }
   }
 
+  const key = getEventId(event)
+
   return {
     name: key,
     key: key,