Repository: jlfwong/speedscope
Pull request #459 (2023-12-27 19:27:13)

Improve hermes profile frame keys to better group frames
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"> |

zacharyfmarion: @jlfwong one more for you before we start thinking about communicating this out 😅
coveralls: 
[![Coverage Status](https://coveralls.io/builds/64777075/badge)](https://coveralls.io/builds/64777075)

coverage: 43.535% (+0.006%) from 43.529%
when pulling **cd0167b8ca297e0ea21c8e131efe86e02631b293 on zacharyfmarion:zac/better-frame-key**
into **3f3da228534beef7c49e2ec38e54c3d31317e38d on jlfwong:main**.

jlfwong: Looks good! If you update the PR name to give this a more descriptive name for the commit log, I'll merge
zacharyfmarion: @jfwong updated, I always miss something 😃

Patch:
diff --git a/sample/profiles/trace-event/hermes-multiple-frames.json b/sample/profiles/trace-event/hermes-multiple-frames.json
new file mode 100644
index 0000000..452f405
--- /dev/null
+++ 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]"
+    }
+  }
+]
\ No newline at end of file
diff --git a/src/import/trace-event.ts b/src/import/trace-event.ts
index 43b05f2..44702cb 100644
--- 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,


Test patch:
diff --git a/src/import/__snapshots__/trace-event.test.ts.snap b/src/import/__snapshots__/trace-event.test.ts.snap
index c416b4f..32c2403 100644
--- 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
index cc6eddf..2a04177 100644
--- 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')
 })