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

Upgrade source-map & drop Node v6 #385

Closed
mourner opened this issue Jul 2, 2019 · 18 comments
Closed

Upgrade source-map & drop Node v6 #385

mourner opened this issue Jul 2, 2019 · 18 comments

Comments

@mourner
Copy link
Contributor

mourner commented Jul 2, 2019

Bug report or Feature request? Performance improvement.

Version (complete output of terser -V or specific git commit) v4.0.2

Profiling bundling time of a large project, I noticed that a significant portion of Terser running time is taken by source map generation. Terser currently uses source-map v0.6.1, which is quite slow — later versions should be up to 11x faster, but require WebAssembly support (Node v8+).

Since Node v6 reached End of Life a few months ago, it would be reasonable to drop support for it, and upgrade to source-map v0.7.3 for the next version of Terser.

Thanks!

@mourner
Copy link
Contributor Author

mourner commented Jul 2, 2019

Or, alternatively (although this would require more effort), switch from source-map to magic-string, which is very fast and powers tools like Rollup.

@fabiosantoscode
Copy link
Collaborator

Those are some interesting facts! Thanks for bringing this up :)

I love magic-string! But it doesn't really fit what we're doing in Terser. We really have to use an AST.

I plan to update source-map anyway when they fix mozilla/source-map#385, so I'll do that when the fix lands and get the performance update for free. Sad about having to drop node 6 but hey, we have to move forward eventually :) I bet @L2jLiga already has ideas on what we can do when it gets dropped.

a significant portion of Terser running time
Would you kindly provide some numbers?

@mourner
Copy link
Contributor Author

mourner commented Jul 3, 2019

@fabiosantoscode I tried to get more specific numbers but it looks like in my case the whole source map handling only takes ~14% of the total time. I've been misled by the following profile, where the garbage collection pause probably just happened at an unfortunate time:

image

So, given that the impact won't be as big as I anticipated, there's no rush in upgrading — sorry!

@mourner mourner closed this as completed Jul 3, 2019
@fabiosantoscode
Copy link
Collaborator

Hey, no reason to close it either :) I'm still interested in dropping node 6 and upgrading source-map so it's useful to keep the issue around.

@Conduitry
Copy link
Contributor

Are later versions of source-map async? I believe this is why Svelte is staying on an older version - we want the compilation to stay synchronous. Is it important to keep Terser's minification sync?

@L2jLiga
Copy link
Contributor

L2jLiga commented Jul 3, 2019

Yeah, you're right, source-map >= 0.7.0 are async. I think we can deal with it. At least async/await (which available in node 8+) let us to write code in synchronous style.

I hope sometime we will switch to newer source-map.. just thinking out loud

@fabiosantoscode
Copy link
Collaborator

Damn. I don't want to go async. Think of the work every user of this library would have to do!

@fabiosantoscode
Copy link
Collaborator

The original purpose of this library was to be a drop-in replacement for uglifyjs, even if it breaks compatibility with old node versions I don't want to change the general interface of the library. It makes it more useful, and doesn't push work to library users, most importantly maintainers of tools which use Terser. I want to make life easy for them.

I'm guessing there's many more tools and scripts that have been built using Terser (a lot of them originally uglifyjs) than the few that are open source software. This change would be very impactful for whoever has to maintain these scripts.

@fabiosantoscode
Copy link
Collaborator

But I'm confident we can find a good solution for this. I'm going to have a look at what we can do.

@alexander-akait
Copy link

alexander-akait commented Jul 4, 2019

Maybe just implement Terser.asyncMinify(code)? Or better Terser.minify(code, { async: true })

@L2jLiga
Copy link
Contributor

L2jLiga commented Jul 5, 2019

@evilebottnawi, I didn't get your idea

Terser uses source-map: 0.6.z - sync version, while source-map >= 0.7.0 - async, how we can have both at same time to provide sync and async APIs ?

@alexander-akait
Copy link

@L2jLiga hm, you are right, in theory we can use fibers, but i think it is not good idea, need investigate, maybe it can be improved on source-map side

@fabiosantoscode
Copy link
Collaborator

We're definitely going to stay sync. I don't want major breaking changes on an age-old interface. Minor breaking changes are OK if justified. This is not even justified. We're just generating a string. Mozilla is doing great by moving onto WASM with source-map but we need a pure javascript solution in this project.

@Conduitry maybe you know the behind-the-scenes reason why Svelte is not using this for source maps? This is from Rich Harris. https://github.com/Rich-Harris/sourcemap-codec

The only problem I can see with it is that you have to create the entire arrays before you tell it to generate a string. I bet it would be more efficient in terms of RAM to use a generator and for...of instead of an array.

I'm happy to use it as is if the performance loss is not significant though. It seems to be well maintained.

@Conduitry
Copy link
Contributor

sourcemap-codec is already used by Svelte, by way of magic-string.

I double-checked Svelte's use of source-map, and it looks like it's actually only currently used during tests. (Some tests are using its SourceMapConsumer.) I was probably confusing this with a potential unimplemented future feature that would use source-map in the compiler, where the async-ness of later versions would be a problem.

@fabiosantoscode
Copy link
Collaborator

Seems to me like sourcemap-codec is a serious contender then

@josephrocca
Copy link
Contributor

Just to confirm, Terser has transitioned away from sync completely, right? The sync API of UglifyES is quite handy in some cases - e.g. for code like this:

let minifyOptions = {nameCache: {}};
html = html.replace(/<script>(.+?)<\/script>/gs, (_, js) => "<script>"+UglifyES.minify(js, minifyOptions).code+"</script>");

It's not that difficult to work around (matchAll + slice at matched indices is probably simplest?), but it is a bit more cumbersome. It's be handy to have a terser.minifySync (perhaps using something like deasync) even if it has a significant performance drop -- just for a bit of "discouraged" compatibility with codebases that currently use UglifyES.

That said, I can see how this might add some extra mess that contributors/maintainers don't want to have to deal with.

@fabiosantoscode
Copy link
Collaborator

I think we can add a note in the readme about using deasync, but I'm not comfortable with adding a compiled dependency.

If you'd like you can add this recommendation in a PR, which may help in some situations as you described.

I'm not sure about the performance cost but my guess is that it's not going to be significant.

@puzrin
Copy link

puzrin commented Sep 2, 2020

Note, sourcemap 0.7.3 still has issues with lost sourceContent data in indexed format. Those are fixed in 0.8.0-beta0.

PS. indexed format is prefered in bundlers, when fast concat of multiple files needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants