Add source map symbolication and source view support#6018
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6018 +/- ##
==========================================
- Coverage 83.78% 83.65% -0.13%
==========================================
Files 329 335 +6
Lines 34528 35388 +860
Branches 9659 9823 +164
==========================================
+ Hits 28930 29605 +675
- Misses 5169 5354 +185
Partials 429 429 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
c372d65 to
5fe5ca4
Compare
|
This PR is now ready for review! |
mstange
left a comment
There was a problem hiding this comment.
Submitting what I have for now
| // The negotiated WebChannel version. Callers use this to gate features that | ||
| // require a minimum version. | ||
| readonly webChannelVersion: number; |
There was a problem hiding this comment.
Do we need this? Could we have a supportsGetSourceMap flag instead so that we don't have to propagate knowledge about versions outwards?
There was a problem hiding this comment.
Hehe, I initially did the same thing, and then thought that it's too specific for the browser connection and changed to this one. I reverted again.
| "redux-thunk": "^3.1.0", | ||
| "reselect": "^4.1.8", | ||
| "source-map": "^0.7.6", | ||
| "url": "^0.11.4", |
There was a problem hiding this comment.
I'm confused - does the source-map dependency just straight up not work in the browser if you don't also depend on url?
There was a problem hiding this comment.
Yeah, it's a bit weird and it's more like a workaround. source-map has this at lib/url.js: module.exports = typeof URL === "function" ? URL : require("url").URL;. At runtime in the browser the first branch wins (since window.URL is a global), so the require("url") is dead code. But esbuild statically follows every require() it sees during bundling, even if the branch is not reachable, so without a real "url" module in node_modules the build fails. This is the error message:
$ yarn start
yarn run v1.22.22
$ cross-env NODE_ENV=development node scripts/run-dev-server.mjs
------------------------------------------------------------------------------------------
> Firefox Profiler is listening at: http://localhost:4242
> You can change this default port with the environment variable FX_PROFILER_PORT.
> esbuild development server enabled
------------------------------------------------------------------------------------------
✘ [ERROR] Could not resolve "url"
node_modules/source-map/lib/url.js:13:59:
13 │ module.exports = typeof URL === "function" ? URL : require("url").URL;
│ ~~~~~
╵ "./url"
The package "url" wasn't found on the file system but is built into node. Are you trying to bundle
for node? You can use "platform: 'node'" to do that, which will remove this error.
| dependencies: | ||
| "@lezer/common" "^1.2.0" | ||
| "@lezer/highlight" "^1.1.3" | ||
| "@lezer/lr" "^1.3.0" |
There was a problem hiding this comment.
Looks like we're not sharing the same library as we're using for source view syntax highlighting? Do we need to update codemirror?
There was a problem hiding this comment.
Ah right, I missed this one. codemirror declares ^1.0.0", which permits 1.5.4 as well. But unfortunately yarn doesn't do deduplication automatically. You have to execute npx yarn-deduplicate to do that. Looks like I forgot to execute it. Updated it now.
| ### Version 63 | ||
|
|
||
| A new `SourceMapInfoTable` has been added to `profile.shared.sourceMapInfo`. Each entry maps a generated (compiled) position to its original source position as determined by source map symbolication. | ||
|
|
||
| - `originalSource: IndexIntoSourceTable[]`: original source file index. Set independently for both func entries (the function's definition file) and frame entries (the execution point's file). | ||
| - `originalLine: number[]`: 1-based original line number | ||
| - `originalColumn: number[]`: 1-based original column number | ||
|
|
||
| Two new columns were added that index into this table: | ||
|
|
||
| - `FrameTable.sourceMapInfo: Array<IndexIntoSourceMapInfoTable | null>`: points to the original execution point for the frame | ||
| - `FuncTable.sourceMapInfo: Array<IndexIntoSourceMapInfoTable | null>`: points to the original definition site for the function | ||
|
|
||
| A new `content: Array<string | null>` column was added to `SourceTable`. | ||
|
|
There was a problem hiding this comment.
Not a fan of the names. Here are some ideas:
sourceMappedLocation
SourceMappedLocationTable
SourceLocationTable
originalLocation
sourceLocationFromSourceMap
There was a problem hiding this comment.
Hmm, I think I like SourceLocationTable and originalLocation. Changed it.
| const funcLine = funcTable.lineNumber[funcIndex]; | ||
| // source and assembly view to scroll those into view. funcLine and the per-sample | ||
| // frame lines come from getOriginalPositionForFrame, so the scroll target lines | ||
| // up with the (original) source view's coordinate system when symbolicated. |
There was a problem hiding this comment.
coordinate system? what does this mean?
There was a problem hiding this comment.
Oh lol, yeah it's weird. I wanted to mean "source view's line numbering when symbolicated." but "coordinate system" was a slop output.
| const { source: sourceIndexOfThisStack, line: selfLineFromMapping } = | ||
| getOriginalPositionForFrame( |
There was a problem hiding this comment.
This scares me from a performance perspective because it runs for every stack in the stack table. How does this impact e.g. the profile linked in #5761?
There was a problem hiding this comment.
Hmm you're right. I profiled it and saw that getStackLineInfo went from 144ms to 373ms (with 209ms is inside this function). I think the easiest option is to inline this into this function so we don't allocate though. I tried that and it went to 186ms. Still worse than 144ms but definitely better than before. What do you think?
Profiles:
main
non-inlined
inlined
| ): { | ||
| source: IndexIntoSourceTable | null; | ||
| line: number | null; | ||
| column: number | null; | ||
| } { |
There was a problem hiding this comment.
Ideally we'd be able to return an IndexIntoSourceLocationTable here. I wonder if this justifies using the SourceLocationTable for both source-mapped and non-source-mapped locations. For example, the FrameTable could have both a location and a locationFromSourceMap column.
There was a problem hiding this comment.
I thought about that one as well but decided not do it eventually. Here was my reasoning:
- If we move the source, line, and column table values in the frame table to another map, it means that we will have 4 table columns that we allocate for each frame instead of 3 that we have currently.
- We will dedup some of the things, so the above issue is a bit better, but then we will have de dedup cost during the profile loading etc.
- The above issue is mostly okay for the source mapped locations because the data is a lot less dense, for example most native frames are going to have only a single
nullas oppose to 4 nulls. - It's a big migration. Not a huge concern considering that we can just add an upgrader, but it touches a lot of places, I'm not so sure about it considering the above issues.
What do you think? I think the first item was the biggest thing that made me decide not to go with this option.
|
|
||
| const fileNameStrIndex = sources.filename[sourceIndex]; | ||
| return fileNameStrIndex !== null | ||
| return fileNameStrIndex !== null && fileNameStrIndex !== undefined |
There was a problem hiding this comment.
So we're looking up a sourceIndex >= sources.length? I wonder if we should block such a sourceIndex from getting into the state at an earlier part of the pipeline.
There was a problem hiding this comment.
Yeah, but I wanted this STR to work:
- Let's say that I captured a profile and I didn't publish it yet.
- I opened up the JS source view that has some source mapped source content.
- I reload the profile.
After this STR, I want the profile to load the source content as expected (after the source map resolution).
This is happening because first the source table is short, and then we append a bunch of new sources later.
Without this commit it crashes. If we move this to an earlier part of the pipeline, it means that we'll just clear out the sourceIndex. If we do that, we lose that state. It's kind of a niche use case, but I found it very useful especially when I was working on the feature.
| * Names come from two sources, in priority order. The compiled source is | ||
| * always available (it's the bundle text we already have). The original | ||
| * source is whatever `sourcesContent` carried: often everything, sometimes | ||
| * nothing. |
There was a problem hiding this comment.
The compiled source is always available? Does this assume we only hit this code if the user is in Firefox and capturing a profile with the jssources feature enabled?
Also what is the meaning of the second sentence?
There was a problem hiding this comment.
Yeah, currently this requires JS Sources feature, because we need the CST parser to have accurate names. But I was thinking about the same thing actually. Maybe we can still do the source map resolution with the information that we have and accept that it will not be fully correct.
For the second sentence. source maps have this sourcesContent array that contains the original source contents. I wanted to emphasize that sometimes they can be removed by the bundlers. But it's not the perfect phrasing. Updated it.
There was a problem hiding this comment.
Edit: Also updated it to always attempt to do source map symbolication if there is no compiled source. It'll be imperfect, but probably still better.
| * var foo = arrow // -> foo (bare named, no chain) | ||
| * var foo = wrap(arrow) // -> outer/foo< (contributes-to: chain + `<`) | ||
| * obj[key] = arrow // -> outer/obj[key] (member-style: chain) |
There was a problem hiding this comment.
var foo = arrow? What does this mean?
There was a problem hiding this comment.
Ah, I wanted to mean an arrow function like var foo = () => {} or wrap(() => {}). Updated them to () => {}.
|
|
||
| let eligibility = funcEligibility[funcIndex]; | ||
| if (eligibility === 0) { | ||
| eligibility = _isFuncSymbolicable(funcIndex, funcTable, sources) ? 1 : 2; |
There was a problem hiding this comment.
Might be a good idea to use const enum here, rather than magic numbers.
There was a problem hiding this comment.
I thought the same! Then I implemented it and thought that it's an overkill since it's only used here and nowhere else. But I can update it still if it makes things clearer.
| * Approximated by: GLB(pos) resolves differently from GLB(predecessor of pos). | ||
| * | ||
| * For pos.col > 0 the predecessor is (line, col - 1). For pos.col === 0 the | ||
| * predecessor is the end of the previous line: source-map's GLB will clip to |
There was a problem hiding this comment.
source-map's originalPositionFor is a greatest-lower-bound (GLB) lookup, so it returns the largest mapping whose generated position is <= the query rather than requiring an exact match.
The _isExactSourceMapEntry helper detects "is this position the start of a new mapping?" by comparing the lookup result at pos with the result at its predecessor: if they differ, pos must be an exact entry.
Updated some of the comments to make it clearer.
| * TODO: This approximation produces false negatives when two consecutive | ||
| * source-map mappings resolve to the same original position (GLB(pos) == | ||
| * GLB(predecessor) even though pos is an exact entry). If this causes | ||
| * incorrect function names in practice, replace with a direct mapping-entry | ||
| * lookup once the source-map library exposes that API. |
There was a problem hiding this comment.
Huh? Do you have an example?
There was a problem hiding this comment.
When I was looking at the profiles that I captured from the profiler.firefox.com source maps, I was hitting an issue frequently that it was getting the first argument name instead of an anonymous function (since it doesn't have any name itself. That's why I had to add this _isExactSourceMapEntry. When I was comparing it with Chrome's devtools, I was seeing the same issue on them as well, it was frequently getting the wrong name.
I added this function to make sure that it's an exact entry. I tested it heavily and I can say that so far all the frames I see are correct. But claude was telling me that it could produce false negatives, but I haven't found any so far. So this TODO is more of a theoretical edge case that could happen, I added here to be more defensive. I can remove it if you prefer as well.
| const tracksWithOrder = { | ||
| globalTracks: getGlobalTracks(getState()), | ||
| globalTrackOrder: getGlobalTrackOrder(getState()), | ||
| localTracksByPid: getLocalTracksByPid(getState()), | ||
| localTrackOrderByPid: getLocalTrackOrderByPid(getState()), | ||
| }; | ||
| const hiddenTracks = { | ||
| hiddenGlobalTracks: getHiddenGlobalTracks(getState()), | ||
| hiddenLocalTracksByPid: getHiddenLocalTracksByPid(getState()), | ||
| }; | ||
| const visibleThreadIndexes = getVisibleThreads( | ||
| tracksWithOrder, | ||
| hiddenTracks | ||
| ); |
There was a problem hiding this comment.
Doing this only for the visible threads is really weird. Is it necessary?
There was a problem hiding this comment.
Probably not fully necessary but I wanted to be a bit more defensive at first. I was thinking the cases where we have hundreds of web contents with source maps on all of them. But probably that's not super realistic.
Edit: Removed that, now we do it for all.
| const result = await _runSourceMapWorker(input); | ||
| switch (result.type) { | ||
| case 'success': | ||
| dispatch({ | ||
| type: 'BULK_SOURCE_MAP_SYMBOLICATION', | ||
| newFuncTable: result.newFuncTable, | ||
| newFrameTable: result.newFrameTable, | ||
| newSourceMapInfo: result.newSourceMapInfo, | ||
| newSources: result.newSources, | ||
| newStringArray: result.newStringArray, | ||
| }); |
There was a problem hiding this comment.
I think it would be better to treat the work in the worker as something like a symbolication API request, where the response doesn't know about our profile tables but still has the necessary information. And then when the result from the worker comes in, we can compute the new tables with main thread JS, like we do for native symbolication. This would address the "stomp over each other" concern.
…SOURCES Version 7 of the WebChannel renames the GET_JS_SOURCES request field from `sourceUuids` to `sourceIds`. The sender picks the field name based on the negotiated WebChannel version so both older Firefox (v6) and newer (v7+) builds are supported.
Lets the browser fetch source maps from URLs the frontend cannot reach. The frontend identifies the bundle source via its sourceId, and the browser returns the parsed source map. Currently it's plumbing only. No caller dispatches GET_SOURCE_MAP yet. Adds source-map and url as direct dependencies; `source-map` provides the RawSourceMap type and runtime parser, and `url` is required by source-map's URL resolution in browser bundles.
Extends the processed profile with a per-thread-shared SourceLocationTable (stored at profile.shared.originalLocation) and a content column on the SourceTable. Each frame and func now has a nullable originalLocation index, set to null by all existing initializers and upgraders. Source content stays null until JS symbolication populates it. The new tables are plumbed through data-structures, profile-compacting, merge-compare, and every importer. It adds a v63 upgrader and a CHANGELOG entry. No call site reads the new columns yet, behavior is unchanged.
The nonymous algorithm (johnjbarton.github.io/nonymous) is how SpiderMonkey labels anonymous JS functions inside its NameFunctions.cpp pass. Adds a parser and serializer for the `/`-separated, `<`-marked name format Gecko emits, so later symbolication can produce names that match what the browser already shows. Pure utility, no callers yet.
Parses compiled JS with @lezer/javascript and walks the CST to build a tree of function scopes with name-mapping locations. The character offsets later probed against the source map to recover original function names. `@lezer/javascript` is already a transitive dep via `@codemirror/lang-javascript`. Pure utility, no callers yet. Tested via the new source-map-scope-tree.test.ts.
Implements the off-main-thread part of JS source map symbolication. SourceMapStore wraps the source-map library's WASM-backed SourceMapConsumer, source-map-symbolication.ts walks every func and frame and maps its compiled position back to the original source via exact-match source-map lookups (with scope-tree probes to recover function names), and source-map.worker.ts wires those pieces up as a Web Worker entry point. The action thunk doSourceMapSymbolication spawns the worker on demand and dispatches BULK_SOURCE_MAP_SYMBOLICATION on success. No caller dispatches the thunk yet.
The Web Worker added in the previous patch needs its own bundle so the @lezer/javascript and source-map dependencies (plus the source-map WASM mappings) ship to the browser. Still no caller invokes the worker, so this patch only changes how `yarn build` builds the dist directory.
Activates JS source map symbolication end-to-end. After native symbolication settles, the receive-profile flow now fetches source maps and compiled JS for the bundle sources visible in any track, hands them to doSourceMapSymbolication, and lets the worker rewrite funcTable / frameTable / sourceMapInfo / sources / stringArray in-place via BULK_SOURCE_MAP_SYMBOLICATION. After this patch, sourceMapInfo is populated but UI code still reads the compiled positions. The next patch makes it look at sourceMapInfo.
The sourceMapInfo table is populated after the previous patch's wire-up, but no UI code read it yet. This patch teaches the source view, call tree, tooltip, line timings, and getOriginAnnotationForFunc to prefer source-mapped positions when present and fall back to the compiled position otherwise. Frame-level entries override func-level entries (so inlined code lands in the right original file). Adds the inline-content fallback in selectors/code.tsx: when the profile already carries sources.content (from a shared profile or from JS symbolication), the source view uses it directly instead of hitting the browser.
The source map pipeline (fetching + worker) previously ran invisibly after profile load. This surfaces two generic status messages in the existing SymbolicationStatusOverlay: "Fetching source maps..." while doResolveSourceMaps runs, and "Symbolicating JS source maps..." while the worker runs. Native symbolication takes priority. The source map messages only appear when the native overlay is not active. It's tracked via a new sourceMapSymbolicationStatus on viewOptions. Note that it looks like this component was never localized. We should do it! But I think this is a task for a follow-up
I noticed this bug while testing the source map PR locally. I was doing this: - Capture a profile with the JS sources feature enabled that includes source maps. - After the symbolication and source fetching, double click on a JS function that is source mapped. - Refresh the page. I was expecting it to work, but it was throwing errors because the source that I already opened wasn't in the profile and string table anymore (since it's added after the source map symbolication). So this fixes this issue by checking if the string table index is undefined
…debugging It proved somewhat difficult when I had the debug the source map symbolication worker on the worker thread. It makes things a lot easier when it runs on the main thread instead. I tried adding a const flag for this with its proper function, but esbuild wasn't properly removing the dead code, so it was inflating the bundle a lot when this function was there even if it was unused. I tried also converting that into an async import, which extracted the symbolication into a different bundle, but it was still outputting the extra bundle even if it was false constant value. So, I decided to include it as a commented out documentation in case we need it in the future. It's not the best, but it should still help us when we need to debug the worker in the future.
`fetchSourceAnnotation` was always reading from funcTable.source[funcIndex], which points at the compiled bundle. For JS-symbolicated functions the original file already lives behind funcTable.sourceMapInfo, and its text is typically embedded on the shared source table either by sourcesContent during symbolication or by the publish-with-sources path. Resolve the effective source through getOriginalPositionForFrame so the filename, line hit attribution, and content lookup all target the original source when sourceMapInfo is present. When sources.content is populated, use it directly and skip the network fetch entirely so the source view works offline for published-with-sources profiles.
…h native Refactor the source-map worker to return position-keyed resolutions instead of full table snapshots. The main thread applies the response against the current shared state via applySourceMapSymbolicationResponse, allocating sourceMapInfo rows, interning names, and deduping URLs against whatever native symbolication has committed by then. With per-funcIndex/per-frameIndex idempotency (skip rows already populated) and JS funcs/frames being insulated from native symbolication, the worker no longer needs to wait for native symbolication to finish. finalizeProfileView now runs source-map fetch, native symbolication, and the source-map worker all in parallel.
getStackLineInfo runs once per stack in the stack table, which can
mean hundreds of thousands of iterations on large profiles. Calling
getOriginalPositionForFrame in that loop allocated a fresh
{source, line, column} object per stack and always resolved the line
(and column) even though the line is only used when the stack's
source matches the source view, and the column is never used here.
Inline the 3-tier source-map fallback so we drop the per-stack
allocation, skip the column entirely, and defer the line lookup
until matchesSource is true. Apply the same inlining to
getTotalLineTimingsForCallNode, which has the same shape and only
needs the line.
Measured on a large profile: getStackLineInfo went from ~373ms back
to ~199ms.
canova
left a comment
There was a problem hiding this comment.
Thanks for the review @mstange! I addressed and added comments. I already squashed some of the things that are not tricky to their own commits, but created their own commits for some things that we might want to discuss. Let me know what you think!
| // The negotiated WebChannel version. Callers use this to gate features that | ||
| // require a minimum version. | ||
| readonly webChannelVersion: number; |
There was a problem hiding this comment.
Hehe, I initially did the same thing, and then thought that it's too specific for the browser connection and changed to this one. I reverted again.
| const tracksWithOrder = { | ||
| globalTracks: getGlobalTracks(getState()), | ||
| globalTrackOrder: getGlobalTrackOrder(getState()), | ||
| localTracksByPid: getLocalTracksByPid(getState()), | ||
| localTrackOrderByPid: getLocalTrackOrderByPid(getState()), | ||
| }; | ||
| const hiddenTracks = { | ||
| hiddenGlobalTracks: getHiddenGlobalTracks(getState()), | ||
| hiddenLocalTracksByPid: getHiddenLocalTracksByPid(getState()), | ||
| }; | ||
| const visibleThreadIndexes = getVisibleThreads( | ||
| tracksWithOrder, | ||
| hiddenTracks | ||
| ); |
There was a problem hiding this comment.
Probably not fully necessary but I wanted to be a bit more defensive at first. I was thinking the cases where we have hundreds of web contents with source maps on all of them. But probably that's not super realistic.
Edit: Removed that, now we do it for all.
| "redux-thunk": "^3.1.0", | ||
| "reselect": "^4.1.8", | ||
| "source-map": "^0.7.6", | ||
| "url": "^0.11.4", |
There was a problem hiding this comment.
Yeah, it's a bit weird and it's more like a workaround. source-map has this at lib/url.js: module.exports = typeof URL === "function" ? URL : require("url").URL;. At runtime in the browser the first branch wins (since window.URL is a global), so the require("url") is dead code. But esbuild statically follows every require() it sees during bundling, even if the branch is not reachable, so without a real "url" module in node_modules the build fails. This is the error message:
$ yarn start
yarn run v1.22.22
$ cross-env NODE_ENV=development node scripts/run-dev-server.mjs
------------------------------------------------------------------------------------------
> Firefox Profiler is listening at: http://localhost:4242
> You can change this default port with the environment variable FX_PROFILER_PORT.
> esbuild development server enabled
------------------------------------------------------------------------------------------
✘ [ERROR] Could not resolve "url"
node_modules/source-map/lib/url.js:13:59:
13 │ module.exports = typeof URL === "function" ? URL : require("url").URL;
│ ~~~~~
╵ "./url"
The package "url" wasn't found on the file system but is built into node. Are you trying to bundle
for node? You can use "platform: 'node'" to do that, which will remove this error.
|
|
||
| const fileNameStrIndex = sources.filename[sourceIndex]; | ||
| return fileNameStrIndex !== null | ||
| return fileNameStrIndex !== null && fileNameStrIndex !== undefined |
There was a problem hiding this comment.
Yeah, but I wanted this STR to work:
- Let's say that I captured a profile and I didn't publish it yet.
- I opened up the JS source view that has some source mapped source content.
- I reload the profile.
After this STR, I want the profile to load the source content as expected (after the source map resolution).
This is happening because first the source table is short, and then we append a bunch of new sources later.
Without this commit it crashes. If we move this to an earlier part of the pipeline, it means that we'll just clear out the sourceIndex. If we do that, we lose that state. It's kind of a niche use case, but I found it very useful especially when I was working on the feature.
| ): { | ||
| source: IndexIntoSourceTable | null; | ||
| line: number | null; | ||
| column: number | null; | ||
| } { |
There was a problem hiding this comment.
I thought about that one as well but decided not do it eventually. Here was my reasoning:
- If we move the source, line, and column table values in the frame table to another map, it means that we will have 4 table columns that we allocate for each frame instead of 3 that we have currently.
- We will dedup some of the things, so the above issue is a bit better, but then we will have de dedup cost during the profile loading etc.
- The above issue is mostly okay for the source mapped locations because the data is a lot less dense, for example most native frames are going to have only a single
nullas oppose to 4 nulls. - It's a big migration. Not a huge concern considering that we can just add an upgrader, but it touches a lot of places, I'm not so sure about it considering the above issues.
What do you think? I think the first item was the biggest thing that made me decide not to go with this option.
| ### Version 63 | ||
|
|
||
| A new `SourceMapInfoTable` has been added to `profile.shared.sourceMapInfo`. Each entry maps a generated (compiled) position to its original source position as determined by source map symbolication. | ||
|
|
||
| - `originalSource: IndexIntoSourceTable[]`: original source file index. Set independently for both func entries (the function's definition file) and frame entries (the execution point's file). | ||
| - `originalLine: number[]`: 1-based original line number | ||
| - `originalColumn: number[]`: 1-based original column number | ||
|
|
||
| Two new columns were added that index into this table: | ||
|
|
||
| - `FrameTable.sourceMapInfo: Array<IndexIntoSourceMapInfoTable | null>`: points to the original execution point for the frame | ||
| - `FuncTable.sourceMapInfo: Array<IndexIntoSourceMapInfoTable | null>`: points to the original definition site for the function | ||
|
|
||
| A new `content: Array<string | null>` column was added to `SourceTable`. | ||
|
|
There was a problem hiding this comment.
Hmm, I think I like SourceLocationTable and originalLocation. Changed it.
| * Names come from two sources, in priority order. The compiled source is | ||
| * always available (it's the bundle text we already have). The original | ||
| * source is whatever `sourcesContent` carried: often everything, sometimes | ||
| * nothing. |
There was a problem hiding this comment.
Edit: Also updated it to always attempt to do source map symbolication if there is no compiled source. It'll be imperfect, but probably still better.
| const { source: sourceIndexOfThisStack, line: selfLineFromMapping } = | ||
| getOriginalPositionForFrame( |
There was a problem hiding this comment.
Hmm you're right. I profiled it and saw that getStackLineInfo went from 144ms to 373ms (with 209ms is inside this function). I think the easiest option is to inline this into this function so we don't allocate though. I tried that and it went to 186ms. Still worse than 144ms but definitely better than before. What do you think?
Profiles:
main
non-inlined
inlined
The bugzilla part of this PR: https://bugzilla.mozilla.org/show_bug.cgi?id=2035493
This requires a Firefox that has patches applied in bug 2035493. And also it requires the "JavaScript Sources" features to be on.
It adds a pipeline that maps compiled/bundled JS frame positions back to their original source files using source maps fetched from the browser after profile load, similarly to our native symbolication step. And it adds a way to see the source mapped source contents.
After this PR, the call tree, flame graph tooltip, source view, and line timings show original TS/JS positions and function names for frames recorded in minified bundles.
I split the work in multiple patches so it's easier to review, but admittedly it's a lot of changes. The initial commits in the PR don't change the behavior until the commit that wires everything up and updates the visualization.
Example STR:
You should then see the symbolicated functions on the frames that are coming from Firefox Profiler source code. (Note that the react code will still be minified as it doesn't have source maps). You should also be able to double click on a JS function function to be able to see the source mapped JS source.