-
Notifications
You must be signed in to change notification settings - Fork 361
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
Backport #308 to apply to 0.6 #370
Comments
@loganfsmyth or @fitzgen I'm sorry if this is an improper way to try to get your attention, but I'd love it if you could consider my proposal and state whether it is something you'd be willing to consider. I'm sure this change will help many other developers using source-maps 0.6.x with Node 10, but I'd understand if you aren't interested in addressing new issues that may result from this change. |
I'd very much love to see this fix back ported as well, it's causing us a lot of headaches. |
I would love to see this PR. In my opinion, taking a v0.6 library of this scope and releasing a minor version update that changes the API from sync to async, introduces a new dependency on WASM, with no plans to support the older API going forward or advice on how to handle (then current) versions of nodejs, was totally ill-advised. There's already been a request to create a support branch for 0.6.x (#324), which was closed; without a support branch to merge into, I don't see how to even get the conversation started about additional changes to 0.6.x. Another option is potentially a new synchronous API, like #369; doesn't solve the pre-WASM node problem, but those are rolling off pretty quickly now (even for companies using them past end of life). FWIW I think it was a huge mistake to have ever introduced this WASM build into |
I've started looking at our Webpack builds again, and have had some pretty good results with using patch-package to apply @benthemonkey's patch (thanks Ben!) to Initial tests show the patched Memory / perf issues were blocking us from upgrading to Webpack 4 previously, but I'm hoping with this approach we can do that too, that's my task for the rest of the week 🤞 |
I also applied the patch and saw build time with Terser & Source-maps drop from over 6 minutes, to just 1.5 minutes. PLEASE PLEASE PLEASE consider back-porting this to 0.6.x. |
mozilla/source-map#370 This makes Webpack about 9% faster. Before: $ multitime -n9 -s0 node_modules/.bin/webpack --config-name=frontend --display=errors-only -p ===> multitime results 1: node_modules/.bin/webpack --config-name=frontend --display=errors-only -p Mean Std.Dev. Min Median Max real 18.243 0.107 18.090 18.236 18.443 user 27.913 0.188 27.714 27.843 28.251 sys 2.028 0.043 1.933 2.039 2.074 After: $ multitime -n9 -s0 node_modules/.bin/webpack --config-name=frontend --display=errors-only -p ===> multitime results 1: node_modules/.bin/webpack --config-name=frontend --display=errors-only -p Mean Std.Dev. Min Median Max real 16.686 0.085 16.542 16.684 16.885 user 25.965 0.167 25.559 26.022 26.163 sys 1.965 0.064 1.807 1.998 2.010 Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
mozilla/source-map#370 This makes Webpack about 9% faster. Before: $ multitime -n9 -s0 node_modules/.bin/webpack --config-name=frontend --display=errors-only -p ===> multitime results 1: node_modules/.bin/webpack --config-name=frontend --display=errors-only -p Mean Std.Dev. Min Median Max real 18.243 0.107 18.090 18.236 18.443 user 27.913 0.188 27.714 27.843 28.251 sys 2.028 0.043 1.933 2.039 2.074 After: $ multitime -n9 -s0 node_modules/.bin/webpack --config-name=frontend --display=errors-only -p ===> multitime results 1: node_modules/.bin/webpack --config-name=frontend --display=errors-only -p Mean Std.Dev. Min Median Max real 16.686 0.085 16.542 16.684 16.885 user 25.965 0.167 25.559 26.022 26.163 sys 1.965 0.064 1.807 1.998 2.010 Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
mozilla/source-map#370 This makes Webpack about 9% faster. Before: $ multitime -n9 -s0 node_modules/.bin/webpack --config-name=frontend --display=errors-only -p ===> multitime results 1: node_modules/.bin/webpack --config-name=frontend --display=errors-only -p Mean Std.Dev. Min Median Max real 18.243 0.107 18.090 18.236 18.443 user 27.913 0.188 27.714 27.843 28.251 sys 2.028 0.043 1.933 2.039 2.074 After: $ multitime -n9 -s0 node_modules/.bin/webpack --config-name=frontend --display=errors-only -p ===> multitime results 1: node_modules/.bin/webpack --config-name=frontend --display=errors-only -p Mean Std.Dev. Min Median Max real 16.686 0.085 16.542 16.684 16.885 user 25.965 0.167 25.559 26.022 26.163 sys 1.965 0.064 1.807 1.998 2.010 Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
mozilla/source-map#370 This makes Webpack about 9% faster. Before: $ multitime -n9 -s0 node_modules/.bin/webpack --config-name=frontend --display=errors-only -p ===> multitime results 1: node_modules/.bin/webpack --config-name=frontend --display=errors-only -p Mean Std.Dev. Min Median Max real 18.243 0.107 18.090 18.236 18.443 user 27.913 0.188 27.714 27.843 28.251 sys 2.028 0.043 1.933 2.039 2.074 After: $ multitime -n9 -s0 node_modules/.bin/webpack --config-name=frontend --display=errors-only -p ===> multitime results 1: node_modules/.bin/webpack --config-name=frontend --display=errors-only -p Mean Std.Dev. Min Median Max real 16.686 0.085 16.542 16.684 16.885 user 25.965 0.167 25.559 26.022 26.163 sys 1.965 0.064 1.807 1.998 2.010 Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Has this changed now that node 6 is out of LTS? |
@jkrems it didn't. You can check the versions tab for stats https://www.npmjs.com/package/source-map Most of the libraries stuck on 0.6.1 and 0.5.7. Maybe you could consider to backport changes from https://github.com/7rulnik/source-map-js and release it as 0.6.2? With these two PRs 7rulnik#3 7rulnik#2 it's just 2x slower than the current WASM implementation but still has sync JS only API. |
It has now an even bigger difference. @jkrems would you consider merging a PR if one would be created? |
I think that would be a reasonable short-term fix. Though I'll have to check the release process to make sure we can also publish it to npm. |
Many libraries won't be upgrading to
source-map
0.7 until Node 6 is out of the LTS schedule (example: webpack/webpack-sources#34 (comment)). That is still 5-6 months away.When I upgraded my project from Node 8 to Node 10, I was surprised to see that the performance of source-map generation was significantly diminished. A webpack production build went from taking 4 minutes to 20 minutes, and all that extra time can be attributed to source-map generation. Thanks to this issue thread -- webpack-contrib/uglifyjs-webpack-plugin#272 (comment) -- it looks like #308 fixes the issue. I'm hoping it's possible to take @marcins 's suggestion, backport this significant performance improvement, and release it as 0.6.2.
I've made the necessary changes to backport the performance improvements in #308 on my fork: 0.6.1...benthemonkey:patch-0.6.1
With these changes I found that my production build time goes back to taking 4 minutes.
The text was updated successfully, but these errors were encountered: