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 GenMapping for sourcemap generation #1190
Conversation
[`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.
lib/sourcemap.js
Outdated
generator.addMapping({ | ||
generated : { line: gen_line + options.dest_line_diff, column: gen_col }, | ||
original : { line: orig_line + options.orig_line_diff, column: orig_col }, | ||
maybeAddMapping(generator, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybeAddMapping
produces a cleaner sourcemap by discarding mappings that add no value. Eg, if two consecutive segments point to the same source location, that provides no new information.
This is a change from the source-map
's addMapping
. If you'd like, I can switch to gen-mapping
's addMapping
, which gives us the same keep-everything behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like less RAM usage to me, so this is a nice change!
lib/minify.js
Outdated
}); | ||
} | ||
}); | ||
result.decodedMap = options.format.source_map.getDecoded(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decoded maps are free to generate from GenMapping
(because mappings are stored in decoded form), it's just a shallow clone.
enumerable: true, | ||
get() { | ||
const map = options.format.source_map.getEncoded(); | ||
return (result.map = options.sourceMap.asObject ? map : JSON.stringify(map)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Encoded maps require serialization from decoded form into an encoded VLQ string. I've placed this behind a getter so that it's not performed unless required by the dev.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated suggestion:
I looked at the implementation underneath getEncoded here and I wonder if a JSON string rendition of it could be optimized.
VLQ strings are guaranteed to not contain \
or "
so for large maps it may be more memory efficient to generate the JSON manually.
'{(other sourcemap properties...), "mappings": "' + encode(decoded.mappings) + '"}'
I'm making the assumption that V8 would make this more memory efficient by not copying the encoded string into the JSON, but instead making the output JSON into a rope containing 3 strings.
}); | ||
if (options.sourceMap.includeSources) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section was moved into the SourceMap
. Before, calling setSourceContent
for the original source contents and the input contents would mean there are 2 contents stored because they have different file names. SourceMapGenerator
would traverse all mappings and determine if a particular file isn't referenced in the mappings, and would exclude that file's contents from the output.
GenMapping
doesn't do that traversal. If you tell it that file.js
has contents, then file.js
will be in the output map even if it's not actually referenced by any mappings. By moving this into SourceMap
, we can do a property lookup for source contents as they're needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requested a couple of changes.
Additionally, can you update the typescript definition? It still refers to source-map and doesn't have the new decoded_map result property.
https://github.com/terser/terser/blob/master/tools/terser.d.ts
"@jridgewell/trace-mapping": "^0.3.5", | ||
"acorn": "^8.5.0", | ||
"commander": "^2.20.0", | ||
"source-map": "~0.8.0-beta.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests still use this (namely test/mocha/sourcemaps.js) so it should go into devDependencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I didn't purge my node_modules
. Fixed.
I'm thinking both this PR and your previous #1180 are breaking changes, as they change how Terser can be loaded in the browser without a package manager (see https://github.com/terser/terser#api-reference). So the next release will be Terser 6.0.0, and I'll likely take away this browser-loading capability. |
Co-authored-by: Fábio Santos <fabiosantosart@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as they change how Terser can be loaded in the browser without a package manager
- Oh, we forgot to update the
rollup.config.js
file - I wrote a small wrapper package that will provides the same
SourceMapConsumer
andSourceMapGenerator
interface assource-map
. That way, if loaded in node, it'll require the new libraries. And if a loaded in a browser, the dev could load eithersource-map
or@jridgewell/source-map
and it'll work.
This preserves API compatibility, so that loading source-map in the browser is still supported.
I've noticed a small uptick in memory usage, but better speed. I'm comparing with this branch merged to
This isn't concerning at all, just wanted to point it out. |
Since you're switching to a library called |
I'll check later. Sadly I'm back from holidays tomorrow so I'll be as non-responsive as usual. |
This is a little surprising. I added memory usage benchmarks which are favorable. I can massage the
Hopefully. I reused the same |
// We support both @jridgewell/source-map (which has a sync | ||
// SourceMapConsumer) and source-map (which has an async | ||
// SourceMapConsumer). | ||
orig_map = await new SourceMapConsumer(options.orig); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
This looks good to merge! |
Awesome work, thanks @jridgewell! |
The `decodedMap` prop comes from terser/terser#1190 > 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.
The `decodedMap` prop comes from terser/terser#1190 > 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.
gen-mapping
is faster, smaller, and lighter sourcemap generation library. This doesn't remove WASM (becauseSourceMapGenerator
doesn't require it), but it's still nice to get off an unmaintained dependency.This also exposes a new
decodedMap
property on the result object. Decoded maps are free to create (it's a shallow clone of theGenMapping
instance), and passing them to@jridgewell/trace-mapping
is copy-free. With Babel recently adding adecodedMap
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.