WIP: load sourcemaps #306
WIP: load sourcemaps #306
Conversation
|
Hmm should I commit the |
There was a problem hiding this comment.
Yesssss thank you for doing this 🙏 !
The approach seems totally right to me from a high level. A few requested changes inline, but this looks pretty solid.
I'd prefer to take the approach you have in this PR rather than parsing source maps ourselves, since there are a variety of different sourcemap formats that IIRC get kind of complicated. (the spec is here if you're curious https://docs.google.com/document/d/1U1RGAehQwRypUTovF1KRlpiOFze0b-_2gc6fAH0KY0k/edit, it's bizarrely a google doc)
Before landing, this will also need tests which exercise remapNamesForLineAndColumn as well as importJavaScriptSymbolMap.
| @@ -13,6 +13,7 @@ import {LeftHeavyFlamechartView, ChronoFlamechartView} from './flamechart-view-c | |||
| import {CanvasContext} from '../gl/canvas-context' | |||
| import {Graphics} from '../gl/graphics' | |||
| import {Toolbar} from './toolbar' | |||
| import {importJavascriptSymbolMap} from '../lib/js-source-map' | |||
There was a problem hiding this comment.
I try to use code-splitting fairly aggressively in speedscope to keep the JS needed to boot the application as small as possible. To get this to be a part of a different chunk (which can be lazy-loaded), you can use the pattern below with const importModule = import('../import'). So in this case:
const jsSourceMapModule = import('../lib/js-source-map')
jsSourceMapModule.then(() => {})
And then put that below the importModule bits below to load it with a lower priority than the import.ts module.
This gives you a promise which resolves as the module. So you should be able to something like the following inline inside of the loadFromFile function:
const {importJavascriptSymbolMap} = await jsSourceMapModule
| @@ -80,6 +80,7 @@ | |||
| }, | |||
| "dependencies": { | |||
| "opn": "5.3.0", | |||
| "react": "^16.13.1" | |||
| "react": "^16.13.1", | |||
| "source-map": "^0.7.3" | |||
There was a problem hiding this comment.
Oh whoops, neither of these should be dependencies, they should only be devDependencies because it's needed to build speedscope but isn't needed to run speedscope as an npm module (the npm module only exposes a binary, not a JavaScript API).
I'm actually not sure why react is in here at all 😬 , but that's a mistake.
So as part of this PR, can you remove react, move source-map to devDependencies, and also commit the changes to package-lock.json?
| import * as sourcemap from 'source-map' | ||
| import {MappingItem} from 'source-map' | ||
|
|
||
| type JavascriptSymbolMap = Map<number, Map<number, string>> |
There was a problem hiding this comment.
Super nitty nit, but JavaScriptSymbolMap please, and similarly importJavaScriptSymbolMap.
| // Looks like the d.ts description doens't properly define `initialize` | ||
| // @ts-ignore | ||
| sourcemap.SourceMapConsumer.initialize({ | ||
| 'lib/mappings.wasm': 'https://unpkg.com/[email protected]/lib/mappings.wasm', |
There was a problem hiding this comment.
Woah this is pretty zany. I'm not sure what this is doing exactly, but it should be possible to pull this in as an import somehow? speedscope currently works offline (except that the fonts are different), so I'd like to keep it fully self-contained if possible (you can literally run speedscope from an extracted zip file with no internet connection).
There was a problem hiding this comment.
It seems like it should be possible to import the wasm module and have it be base64 encoded somehow. A tiny bit of information available here: mozilla/source-map#408. It's possible that doing this will require mucking with the packaging config (I'm using https://parceljs.org/ at the moment, though have plans to switch to esbuild).
| const {profile, index} = this.props.activeProfileState | ||
| console.log('Importing as javascript symbol map') | ||
| profile.remapNamesForLineAndColumn(frame => { | ||
| console.log(frame.line, frame.col, jsMap.get(frame.line ?? -1)?.get(frame.col ?? -1)) |
There was a problem hiding this comment.
I think this is part of this being WIP, but please remember to remove!
This PR adds the ability to remap an already-loaded profile using a JavaScript source map. This is useful for e.g. recording minified profiles in production, and then remapping their symbols when the source map isn't made directly available to the browser in production. This is a bit of a hidden feature. The way it works is to drop a profile into speedscope, then drop the sourcemap file on top of it. To test this, I used a small project @cricklet made (https://gist.github.com/cricklet/0deaaa7dd63657adb6818f0a52362651), and also tested against speedscope itself. To test against speedscope itself, I profiled loading a file in speedscope in Chrome, then dropped the resulting Chrome timeline profile into speedscope, and dropped speedscope's own sourcemap on top. Before dropping the source map, the symbols look like this:  After dropping the source map, they look like this:  I also added automated tests using a small JS bundle constructed with various different JS bundlers to make sure it was doing a sensible thing in each case. # Background Remapping symbols in profiles using source-maps proved to be more complex than I originally thought because of an idiosyncrasy of which line & column are referenced for stack frames in browsers. Rather than the line & column referencing the first character of the symbol, they instead reference the opening paren for the function definition. Here's an example file where it's not immediately apparent which line & column is going to be referenced by each stack frame: ``` class Kludge { constructor() { alpha() } zap() { alpha() } } function alpha() { for (let i = 0; i < 1000; i++) { beta() delta() } } function beta() { for (let i = 0; i < 10; i++) { gamma() } } const delta = function () { for (let i = 0; i < 10; i++) { gamma() } } const gamma = () => { let prod = 1 for (let i = 1; i < 1000; i++) { prod *= i } return prod } const k = new Kludge() k.zap() ``` The resulting profile looks like this:  The relevant line & column for each function are... ``` // Kludge: line 2, column 14 class Kludge { constructor() { ^ ... // zap: line 6, column 6 zap() { ^ ... // alpha: line 11, column 15 function alpha() { ^ ... // delta: line 24, column 24 const delta = function () { ^ ... // gamma: line 31, column 1 const gamma = () => { ^ ``` If we look up the source map entry that corresponds to the opening paren, we'll nearly always get nothing. Instead, we'll look at the entry *preceding* the one which contains the opening paren, and hope that has our symbol name. It seems this works at least some of the time. Another complication is that some, but not all source maps include the original names of functions. For ones that don't, but do include the original source-code, we try to deduce it ourselves with varying amounts of success. Supersedes #306 Fixes #139
|
Closing since #317 has now been landed! |
This PR adds the ability to remap an already-loaded profile using a JavaScript source map. This is useful for e.g. recording minified profiles in production, and then remapping their symbols when the source map isn't made directly available to the browser in production. This is a bit of a hidden feature. The way it works is to drop a profile into speedscope, then drop the sourcemap file on top of it. To test this, I used a small project @cricklet made (https://gist.github.com/cricklet/0deaaa7dd63657adb6818f0a52362651), and also tested against speedscope itself. To test against speedscope itself, I profiled loading a file in speedscope in Chrome, then dropped the resulting Chrome timeline profile into speedscope, and dropped speedscope's own sourcemap on top. Before dropping the source map, the symbols look like this:  After dropping the source map, they look like this:  I also added automated tests using a small JS bundle constructed with various different JS bundlers to make sure it was doing a sensible thing in each case. # Background Remapping symbols in profiles using source-maps proved to be more complex than I originally thought because of an idiosyncrasy of which line & column are referenced for stack frames in browsers. Rather than the line & column referencing the first character of the symbol, they instead reference the opening paren for the function definition. Here's an example file where it's not immediately apparent which line & column is going to be referenced by each stack frame: ``` class Kludge { constructor() { alpha() } zap() { alpha() } } function alpha() { for (let i = 0; i < 1000; i++) { beta() delta() } } function beta() { for (let i = 0; i < 10; i++) { gamma() } } const delta = function () { for (let i = 0; i < 10; i++) { gamma() } } const gamma = () => { let prod = 1 for (let i = 1; i < 1000; i++) { prod *= i } return prod } const k = new Kludge() k.zap() ``` The resulting profile looks like this:  The relevant line & column for each function are... ``` // Kludge: line 2, column 14 class Kludge { constructor() { ^ ... // zap: line 6, column 6 zap() { ^ ... // alpha: line 11, column 15 function alpha() { ^ ... // delta: line 24, column 24 const delta = function () { ^ ... // gamma: line 31, column 1 const gamma = () => { ^ ``` If we look up the source map entry that corresponds to the opening paren, we'll nearly always get nothing. Instead, we'll look at the entry *preceding* the one which contains the opening paren, and hope that has our symbol name. It seems this works at least some of the time. Another complication is that some, but not all source maps include the original names of functions. For ones that don't, but do include the original source-code, we try to deduce it ourselves with varying amounts of success. Supersedes jlfwong#306 Fixes jlfwong#139
This is WIP! Let me know what you think of the overall approach and any guideline on testing. This PR is pretty much gluing
speedscopeto Mozilla'ssource-mapproject. Interestingly, they use a wasm binary to do the decoding quickly.Another possible approach could be doing the source-map decoding manually, eg like: https://gist.github.com/bengourley/c3c62e41c9b579ecc1d51e9d9eb8b9d2.
I tested this with a tiny test project here: https://gist.github.com/cricklet/0deaaa7dd63657adb6818f0a52362651
Before:
After:
Fixes: #139