From e9a17d5d14af11a990e5e6bd2e8789f2d0a58472 Mon Sep 17 00:00:00 2001 From: Zachary Marion Date: Wed, 27 Dec 2023 23:56:27 -0500 Subject: [PATCH] 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 | | ----- | ----- | | Screenshot 2023-12-27 at 2 25 32 PM | Screenshot 2023-12-27 at 2 26 02 PM | | Screenshot 2023-12-27 at 2 13 37 PM | Screenshot 2023-12-27 at 2 14 20 PM | --- .../trace-event/hermes-multiple-frames.json | 156 ++++++++++++++++++ .../__snapshots__/trace-event.test.ts.snap | 39 ++++- src/import/trace-event.test.ts | 4 + src/import/trace-event.ts | 18 +- 4 files changed, 210 insertions(+), 7 deletions(-) create mode 100644 sample/profiles/trace-event/hermes-multiple-frames.json 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 00000000..452f4059 --- /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/__snapshots__/trace-event.test.ts.snap b/src/import/__snapshots__/trace-event.test.ts.snap index c416b4fe..32c2403b 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 cc6eddf1..2a041774 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') }) diff --git a/src/import/trace-event.ts b/src/import/trace-event.ts index 43b05f20..44702cbb 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,