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

Switch to @jridgewell/gen-mapping for sourcemap generation #14497

Merged
merged 6 commits into from Apr 28, 2022

Conversation

jridgewell
Copy link
Member

@jridgewell jridgewell commented Apr 27, 2022

Q                       A
Fixed Issues? Closes #12805
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes? Yes, removed final source-map and switch to @jridgewell/gen-mapping
License MIT

This removes the final use (of an ancient version 5yo version) of source-map. This gives a few benefits:

  1. Adding mappings to the sourcemap is ~30-300% faster.
  2. Serializing the map for writing is 250-550% faster
  3. Preparing a map for merging is free, and actual merging is 5-25x (x, not %) faster

Somewhat controversial, but I'm making rawMappings slower. Why? Because it isn't used by anyone. And, we're always generating a map output (because babel-cli reads the .map getter). I think it's more important to prioritize the regular sourcemap than the rawMappings. If someone wants to use rawMappings for speed, I'd encourage them to use the new decodedMap field, and @jridgewell/trace-mapping to trace or @ampproject/remapping to remap. Regardless, adding items to the GenMapping should be just as fast as pushing items onto the _rawMappings, and possibly considerably faster since the raw mappings covers every line/column vs the gen-mapping's per-line array.

@babel-bot
Copy link
Collaborator

@babel-bot babel-bot commented Apr 27, 2022

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51761/

@jridgewell jridgewell changed the title Gen mapping babel-generator: Switch to @jridgewell/gen-mapping for sourcemap generation Apr 27, 2022
@jridgewell jridgewell changed the title babel-generator: Switch to @jridgewell/gen-mapping for sourcemap generation babel-generator: Switch to gen-mapping for sourcemap generation Apr 27, 2022
@jridgewell jridgewell added pkg: generator PR: New Dependency PR: Performance 🏃‍♀️ labels Apr 27, 2022
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Awesome, thanks! 😄

@@ -1 +1 @@
{"version":3,"sources":["../src/foo.js"],"names":["arr","map","x","MULTIPLIER"],"mappings":";;AAAAA,GAAG,CAACC,GAAJ,CAAQ,UAAAC,CAAC;AAAA,SAAIA,CAAC,GAAGC,UAAR;AAAA,CAAT","sourcesContent":["arr.map(x => x * MULTIPLIER);"],"file":"foo.js"}
{"version":3,"file":"foo.js","names":["arr","map","x","MULTIPLIER"],"sources":["../src/foo.js"],"sourcesContent":["arr.map(x => x * MULTIPLIER);"],"mappings":";;AAAAA,GAAG,CAACC,GAAJ,CAAQ,UAAAC,CAAC;AAAA,SAAIA,CAAC,GAAGC,UAAR;AAAA,CAAT"}
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Apr 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reviewers: the various source map changes are just differences in the fields order; the contents are the same.

source:
line == null
? undefined
: (filename || this._opts.sourceFileName).replace(/\\/g, "/"),
: filename?.replace(/\\/g, "/") || this._sourceFileName,
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Apr 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reviewers: this._sourceFileName is already normalized in the constructor.

@nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Apr 27, 2022

The Jest failures are caused by this PR, because of #14497 (comment). We can let them fail until the next patch, so that Jest can then update their snapshots.

@jridgewell
Copy link
Member Author

@jridgewell jridgewell commented Apr 27, 2022

Given this is causing breaking changes in downstream projects, we can preserve the prop ordering of the sourcemap pretty easily. I'll push a commit.

@nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Apr 27, 2022

I mean, I wouldn't consider it a breaking change but just a "testing implementation details". The order of object keys is never considered part of the "public contract" 😛

It's enough to let @SimenB know about this so that he won't be surprised about the Jest failures when updating Babel, and we don't need to worry about preserving the original properties order.

@SimenB
Copy link
Contributor

@SimenB SimenB commented Apr 27, 2022

Hey! 👋

Exciting to see more of source-map modernized (replaced) 👏

I totally agree key order shouldn't be part of the contract - will update our snapshots once there's a release 🙂

(you can probably just delete the test in the meantime in your e2e test if you want it green)

Copy link
Contributor

@JLHwung JLHwung left a comment

🥳

@nicolo-ribaudo nicolo-ribaudo changed the title babel-generator: Switch to gen-mapping for sourcemap generation Switch to @jridgewell/gen-mapping for sourcemap generation Apr 28, 2022
@nicolo-ribaudo nicolo-ribaudo merged commit b6d8434 into babel:main Apr 28, 2022
34 of 36 checks passed
@jridgewell jridgewell deleted the gen-mapping branch Apr 28, 2022
jridgewell added a commit to jridgewell/terser that referenced this issue Apr 30, 2022
[`gen-mapping`](https://github.com/jridgewell/gen-mapping) is faster, smaller, and lighter  sourcemap generation library.

This also exposes a new `decodedMap` property on the result object. Decoded maps are free to create (it's a shallow clone of the `GenMapping` instance), and passing them to `@jridgewell/trace-mapping` is copy-free. With Babel  [recently](babel/babel#14497) adding a `decodedMap` field, a dev could pass from the Babel transpilation to Terser without any added memory use for sourcemaps.

And if there's a multi stage build process, a dev could use `@ampproject/remapping` to remap Babel, Terser, and (eg) a bundler's outputs without having to feed input maps into each stage.
jridgewell added a commit to jridgewell/terser that referenced this issue Apr 30, 2022
[`gen-mapping`](https://github.com/jridgewell/gen-mapping) is faster, smaller, and lighter  sourcemap generation library.

This also exposes a new `decodedMap` property on the result object. Decoded maps are free to create (it's a shallow clone of the `GenMapping` instance), and passing them to `@jridgewell/trace-mapping` is copy-free. With Babel  [recently](babel/babel#14497) adding a `decodedMap` field, a dev could pass from the Babel transpilation to Terser without any added memory use for sourcemaps.

And if there's a multi stage build process, a dev could use `@ampproject/remapping` to remap Babel, Terser, and (eg) a bundler's outputs without having to feed input maps into each stage.
fabiosantoscode added a commit to terser/terser that referenced this issue May 10, 2022
* Switch to GenMapping for sourcemap generation

[`gen-mapping`](https://github.com/jridgewell/gen-mapping) is faster, smaller, and lighter  sourcemap generation library.

This also exposes a new `decodedMap` property on the result object. Decoded maps are free to create (it's a shallow clone of the `GenMapping` instance), and passing them to `@jridgewell/trace-mapping` is copy-free. With Babel  [recently](babel/babel#14497) adding a `decodedMap` field, a dev could pass from the Babel transpilation to Terser without any added memory use for sourcemaps.

And if there's a multi stage build process, a dev could use `@ampproject/remapping` to remap Babel, Terser, and (eg) a bundler's outputs without having to feed input maps into each stage.

* Apply suggestions from code review

Co-authored-by: Fábio Santos <fabiosantosart@gmail.com>

* Fix dependency

* Switch to source-map compatible @jridgewell/source-map

This preserves API compatibility, so that loading source-map in the browser is still supported.

* Update TS types

* Support source-map's async SourceMapConsumer

* Fix sourcesContent check with source-map's SourceMapGenerator

Co-authored-by: Fábio Santos <fabiosantosart@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: generator PR: New Dependency PR: Performance 🏃‍♀️
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants