Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use WebAssembly to speed up SourceMapConsumer #306

Merged
merged 46 commits into from Jan 18, 2018
Merged

Conversation

@fitzgen
Copy link
Contributor

@fitzgen fitzgen commented Jan 8, 2018

r? @tromey

fitzgen added 30 commits Dec 19, 2017
Easier to graph / work with after benchmarking this way.
```js
var consumer = new sourceMap.SourceMapConsumer(rawSourceMapJsonData);
consumer.destroy();

This comment has been minimized.

@tromey

tromey Jan 16, 2018
Contributor

Bummer but I guess there's no good way around it.

This comment has been minimized.

@fitzgen

fitzgen Jan 17, 2018
Author Contributor

I have ideas to improve this in follow ups:

Either add an async RAII-ish function like:

SourceMapConsumer.with = async function (rawSourceMap, f) {
  const consumer = await new SourceMapConsumer(rawSourceMap);
  try {
    await f(consumer);
  } finally {
    consumer.destroy();
  }
};

Or alternatively give every SourceMapConsumer its own wasm module instance, make the Mappings a global/implicit in the wasm heap, and let the GC manage lifetimes. Requires further investigation.

};
}

if (typeof print !== "function") {
print = console.log.bind(console);
print = function (x = "") {
console.log(`${x}`);

This comment has been minimized.

@tromey

tromey Jan 16, 2018
Contributor

Not important but I thought console.log was self-bound already.

This comment has been minimized.

@fitzgen

fitzgen Jan 17, 2018
Author Contributor

I know it differed across engines at one point in time, unsure if it still does or not.

In fact, the shell version of the benchmarks doesn't work anymore (added too many options that are only exposed in the Webpage), so I should just remove it.

module.exports = function readWasm() {
if (typeof mappingsWasmUrl !== "string") {
throw new Error("You must provide the URL of lib/mappings.wasm by calling " +
"SourceMapConsumer.initialize({ 'lib/mappings.wasm': ... }) " +

This comment has been minimized.

@tromey

tromey Jan 16, 2018
Contributor

Would it be crazy to just have the wasm be a giant string constant in some .js file?

This comment has been minimized.

@fitzgen

fitzgen Jan 17, 2018
Author Contributor

It would bloat the code size, which I took quite a bit of effort in keeping small.

// The offset fields are 0-based, but we use 1-based indices when
// encoding/decoding from VLQ.
generatedLine: offsetLine + 1,
generatedColumn: offsetColumn + 1

This comment has been minimized.

@tromey

tromey Jan 16, 2018
Contributor

Not introduced here, but elsewhere it says that columns are 0-based (what I recall as well), so I wonder if this is a latent bug.

This comment has been minimized.

@fitzgen

fitzgen Jan 17, 2018
Author Contributor

Perhaps? I had to do this to get the tests passing with the wasm. I think we can investigate further in a follow up.

for (var j = 0; j < sectionMappings.length; j++) {
var mapping = sectionMappings[j];

var source = section.consumer._sources.at(mapping.source);
source = util.computeSourceURL(section.consumer.sourceRoot, source, this._sourceMapURL);
var source = util.computeSourceURL(section.consumer.sourceRoot, source, this._sourceMapURL);
this._sources.add(source);
source = this._sources.indexOf(source);

var name = null;

This comment has been minimized.

@tromey

tromey Jan 16, 2018
Contributor

I didn't examine more context but I suspect this line should be removed.

This comment has been minimized.

@fitzgen

fitzgen Jan 17, 2018
Author Contributor

The var name = null; line? The name variable is assigned to and used a little bit down this method.

This comment has been minimized.

@tromey

tromey Jan 17, 2018
Contributor

Thanks.

var sourceRoot = this.sourceRoot;
mappings.map(function (mapping) {
var source = null;
if(mapping.source !== null) {

This comment has been minimized.

@tromey

tromey Jan 16, 2018
Contributor

Trivia: space after the "if".

exports['test .fromSourceMap'] = function (assert) {
var map = SourceMapGenerator.fromSourceMap(new SourceMapConsumer(util.testMap));
exports['test .fromSourceMap'] = async function (assert) {
var map = SourceMapGenerator.fromSourceMap(await new SourceMapConsumer(util.testMap));

This comment has been minimized.

@tromey

tromey Jan 16, 2018
Contributor

I suppose it's not a big deal to leak the consumers in the tests.

This comment has been minimized.

@fitzgen

fitzgen Jan 17, 2018
Author Contributor

Hm, I thought I caught them all, did I miss some?

This comment has been minimized.

@tromey

tromey Jan 17, 2018
Contributor

I didn't see where these consumers were destroyed. But maybe fromSourceMap does and I missed it.

@tromey
tromey approved these changes Jan 16, 2018
Copy link
Contributor

@tromey tromey left a comment

This is big enough and covers enough different things that it is going to suffer from the other side of bikeshedding -- that is, I tried to read it all but I'm not sure I can really do it justice.

I do wonder if it's possible to remove the need for a separate wasm URL and thus (IIUC) the need for the constructor to return a promise.

Otherwise I'm inclined to push ahead. Perhaps I ought to go address #291 and #305...

@fitzgen
Copy link
Contributor Author

@fitzgen fitzgen commented Jan 17, 2018

I do wonder if it's possible to remove the need for a separate wasm URL and thus (IIUC) the need for the constructor to return a promise.

Compiling wasm is still done off-thread, and returns a promise, so even if we already had the blob, the constructor would still return a promise.

@tromey
Copy link
Contributor

@tromey tromey commented Jan 17, 2018

Compiling wasm is still done off-thread, and returns a promise, so even if we already had the blob, the constructor would still return a promise.

Thanks. Never mind then.

fitzgen added 2 commits Jan 17, 2018
@fitzgen
Copy link
Contributor Author

@fitzgen fitzgen commented Jan 17, 2018

Ok, I think this is just about ready to land. Waiting on OSX jobs in Travis CI.

@fitzgen
Copy link
Contributor Author

@fitzgen fitzgen commented Jan 18, 2018

FYI, https://hacks.mozilla.org/2018/01/oxidizing-source-maps-with-rust-and-webassembly/ is live with details on the experience.

Going to go ahead and merge this PR!

@fitzgen fitzgen merged commit 4a047e7 into mozilla:master Jan 18, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
```

The resulting `wasm` file will be located at
`source-map-mappings-c-api/target/wasm32-unknown-unknown/release/source_map_mappings.wasm`.

This comment has been minimized.

@Kagami

Kagami Jan 22, 2018

s/c-api/wasm-api/

This comment has been minimized.

@fitzgen

fitzgen Jan 22, 2018
Author Contributor

Nice catch!

Open `bench.html` in a browser and click on the appropriate button.
```
$ cd source-map/
$ python -m SimpleHTTPServer

This comment has been minimized.

@Kagami

Kagami Jan 22, 2018

Better python2 -m SimpleHTTPServer or python3 -m http.server

This comment has been minimized.

@fitzgen

fitzgen Jan 22, 2018
Author Contributor

Thanks

module.exports = function readWasm() {
if (typeof mappingsWasmUrl !== "string") {
throw new Error("You must provide the URL of lib/mappings.wasm by calling " +
"SourceMapConsumer.initialize({ 'lib/mappings.wasm': ... }) " +

This comment has been minimized.

@Kagami

Kagami Jan 22, 2018

initialize() accepts just string though?

This comment has been minimized.

@fitzgen

fitzgen Jan 22, 2018
Author Contributor

The public SourceMapConsumer.initialize function takes an object, the interal initialize function in lib/read-wasm.js just takes a string.

@stewx

This comment has been minimized.

Copy link

@stewx stewx commented on f0e7420 Feb 5, 2018

Should this not require a major version number increase, given that it is a breaking change, no longer supporting versions of node lower than 8.0? This is causing issues for us now since we're on Node 6.10

This comment has been minimized.

Copy link
Contributor Author

@fitzgen fitzgen replied Feb 5, 2018

It did. Below 1.0, a 0.X bump is considered a breaking change.

This comment has been minimized.

Copy link

@contra contra replied May 1, 2019

I know this is late to the game - but is there any documentation about why this change was made? Most of the tools that use source-map did not update to 0.7 because this was a major breaking API change that is difficult/impossible for people to pull in without having to do their own breaking API change (babel is still on 0.5 because they would need to make their entire API async to land this as one example).

Looking at the actual change I can't immediately see any benefits of making this async - I'm sure there were reasons for it but it would be good to have some documentation about it somewhere.

This comment has been minimized.

Copy link
Contributor

@loganfsmyth loganfsmyth replied May 1, 2019

Best to continue this in #331 if you'd like, but the short answer is that this module now uses WASM, and loading WASM synchronously is mostly a no-go.

This comment has been minimized.

Copy link

@contra contra replied May 2, 2019

@loganfsmyth Ah ok, makes complete sense - just looking at this commit in isolation made it seem unrelated, but if it's related to the WASM change that seems right. Thanks for the response.

@khalilof

This comment has been minimized.

Copy link

@khalilof khalilof commented on 59fab43 Feb 26, 2018

this nice change just blew up our code :)

jridgewell added a commit to jridgewell/source-map that referenced this pull request Jul 3, 2019
This is a leftover from mozilla#306.
jasonLaster added a commit that referenced this pull request Oct 29, 2019
This is a leftover from #306.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants