Single pass ReplaceSource .node performance improvement #23

brentwilton · 2017-07-21T01:13:13Z

What kind of change does this PR introduce?
Performance improvement. Up to 35% faster webpack builds on large solutions.
(small solutions do not spend as much time in ReplaceSource)

Did you add tests for your changes?
Added tests to improve coverage around prepending and appending replaces as these were not covered by existing tests.

If relevant, link to documentation update:
N/A

Summary
Replaces the .node method in ReplaceSource with a version which performs all replacements in a single pass.

Does this PR introduce a breaking change?
No

Other information
This PR can produce significantly faster build times when processing large files with devtool: 'source-map'

The existing .node method walks the SourceNode tree for every replace each time, twice splitting the SourceNode tree at the replace start and end. In large source maps this can take significant time as the SourceNode constructor will individually .add each child node on each split.

The new method processes all replaces inline, turning on and off an emit flag depending on if you are in the middle of a replace or not.
The PR emits identical module and source map output as webpack 3.4.1 / webpack-sources 1.0.1. (Including use with ModuleConcatenationPlugin which chains ReplaceSource's)

For our current Angular prod build using devtool: 'source-map' and 2481 modules, this PR reduces the chunk asset optimization time (average over 3 runs) from ~314 seconds to ~127 seconds, and reduces total webpack build time from ~485 seconds to ~314 seconds. (38.5% or 187 seconds faster)

Update ReplaceSource .node to process all replacements in a single pass

codecov · 2017-07-21T01:15:03Z

Codecov Report

Merging #23 into master will increase coverage by 4.04%.
The diff coverage is 96.15%.

@@            Coverage Diff             @@
##           master      #23      +/-   ##
==========================================
+ Coverage      71%   75.04%   +4.04%     
==========================================
  Files          11       11              
  Lines         438      505      +67     
  Branches       66       83      +17     
==========================================
+ Hits          311      379      +68     
+ Misses        127      126       -1

Impacted Files	Coverage Δ
lib/ReplaceSource.js	`95.85% <96.15%> (+2.99%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b643bfb...cf11460. Read the comment docs.

brentwilton · 2017-08-14T21:23:38Z

@sokra @TheLarkInn please let me know if there is anything additional I can provide to help clarify the issue or to explain how the updated ReplaceSource.node() method works.

We are keen to get this PR merged as it makes our CI webpack builds 3 minutes faster.

TheLarkInn · 2017-08-29T19:32:51Z

Hey @brentwilton I'm sorry for not seeing this right away. I'm slowly migrating to Octobox for tracking all of these issues and PR's across our orgs' 100+ repos. :-D

Let me see if I can get this prioritized.

alan-agius4 · 2017-11-16T18:05:16Z

@sokra Can we have this merged this?

#29

TheLarkInn · 2017-11-16T23:10:42Z

This is on my weekend hotlist to merge. Again so sorry for the delay. We want to make sure this doesn't break upstream.

alan-agius4 · 2017-11-25T16:57:31Z

Reminder!

brentwilton · 2017-11-30T20:50:32Z

@alan-agius4 We have been using a postinstall script in the package.json to manually patch ReplaceSource.js for the last few months while we have been waiting for this PR to be merged.
(I highly recommend implementing this workaround if you have not already. The performance benefits outweigh the hackiness)

With webpack 3.8.1 and the 1.1.0 version of ReplaceSource our CI build takes 47 minutes. With the PR version, it takes 17 minutes. (the patch version was temporarily not being applied on our CI build yesterday and everyone thought that the CI server had broken due to builds taking forever)

alan-agius4 · 2017-11-30T20:54:13Z

I dont understand why this pr is not being prioritized and it’s frustrating! I have the same problem. My CI build it takes approx 40mins because of this method.

sokra · 2017-12-01T10:47:14Z

This is difficult to review, but I take a look soon.

brentwilton · 2017-12-02T10:10:01Z

Thanks for the update @sokra. I know you guys have been busy the last few months with progressing webpack 4.

To give a bit more information about the cause of the slowdown and the solution:

The current algorithm does the following:
For each replace we:

Traverse the entire source map tree until we find the replace end point
Split the source map tree at that point
Traverse the entire source map tree until we find the replace start point
Split the source map tree at that point
Add the new replace text and the end section
Move onto the next replacement with the remainder of the tree

There are some performance issues with this approach

We traverse the entire remaining tree twice per replace
The tree is split twice per replace, at the start and end point of the replace
When the tree is split, it adds the split sections as children to 2 new SourceNodes
When you create a new SourceNode, internally it will type check and array.push each child individually to its children array
On large source maps with a lot of replaces this can take minutes and it thrashes memory while doing so.

The new algorithm changes this to only traverse the source node tree a single time making a copy of it to a new tree.

We first sort the list of replacements by start location, and find the first location where we want to replace.
When traversing the tree we look for the first replacements start point, and when we hit that point, we turn an emit flag off and stop copying across to the new tree.
We add the replacement text as the next node in the copy.
We then continue the traversal and look for the replacement end point. On hitting the end point we turn back on the emit flag and start copying the tree again.
When we hit the end point and start emitting again, we then move onto the next replacement in the list and repeat the above until the entire tree is copied.

The ReplacementEnumerator class handles the state of the emit flag and current replacement index.

There are some special cases we also deal with:

Some of the replacements occur outside of the original source map. This is usually webpack adding the imports or require wrappers. We run the _prependStartNodes and _appendRemainingNodes steps to handle this and make the main traversal copy code less complex.
Replaces can only occur on leaf nodes, and multiple replaces can occur on the same leaf node. Sometimes the start/end points will even overlap so we actually keep recursing into replaceInStringNode after we finish a replace to handle this.
If you look in the header() method I included some code which tweaks the output to generate identical source maps to the previous version. Identical source map output is good to show we didn't break anything in the change.

We had a situation where a single ReplaceSource.node call was taking 15 minutes on a complex build. The new algorithm reduced that call to 0.12 seconds.

vladimir-tikhonov · 2017-12-08T13:57:02Z

Hello, @brentwilton! I've tried use your branch on our project, but unfortunately found a bug. We use redux-action package and now webpack produces invalid js code when devtool is set to "source-map". Seems like this file is causing issues: https://github.com/reduxactions/redux-actions/blob/master/src/camelCase.js. I've created a repro repository for you here: https://github.com/vladimir-tikhonov/webpack-sources-bug. Let me know if you need something else.

brentwilton · 2017-12-10T21:26:11Z

Hi @vladimir-tikhonov. Thanks for trying this PR out and submitting a repro of your issue.
I will pull it down and isolate the cause.

…rlapping replaces

brentwilton · 2017-12-11T03:30:41Z

@vladimir-tikhonov I have pushed a new version. Can you check to see if it resolves your issue ?

The code was double emitting the ); if it reached the end of the source map while in the middle of a replace.

vladimir-tikhonov · 2017-12-11T11:57:43Z

@brentwilton yes, the issue is resolved now. Great job!

alan-agius4 · 2018-01-07T20:30:47Z

Can we have this re-reviewed please?

TheLarkInn · 2018-01-10T23:40:38Z

Yes cc @sokra and @ooflorent also.

…

On Sun, Jan 7, 2018, 12:30 PM Alan Agius ***@***.***> wrote: Can we have this re-reviewed please? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADQBMAoKsgJ51a4d4QVMv4ra-hFecZjDks5tISl3gaJpZM4Oe2IV> .

ooflorent

Looks good to me. It also passes all webpack's integration tests.

ooflorent · 2018-01-11T08:22:21Z

lib/ReplaceSource.js

+	}
+
+	_sortReplacementsAscending() {
+		this.replacements.sort(function(a, b) {


Could you define the sort function outside of the class?

@brentwilton bump

@ooflorent Moved sort function outside of class as requested.

alan-agius4 · 2018-01-25T20:21:20Z

Can we merge this obe? Super imporant for me as I have a huge app

TheLarkInn · 2018-01-25T22:42:56Z

I think we are waiting on @ooflorent's request, and I'd like @sokra to review as well for this. Given we just released [email protected] we need to make sure this doesn't cause any breaking changes (even if tests passing).

abhinavsingi · 2018-03-31T13:35:19Z

Any idea when will it be merged?
@TheLarkInn @sokra

ghost · 2018-04-12T23:30:12Z

Is there any update on getting this issue merged?

@TheLarkInn @sokra

Thanks!

alan-agius4 · 2018-04-24T21:00:53Z

Ping!

ghost · 2018-04-24T21:10:22Z

:tumbleweed:

vohzd · 2018-04-26T13:50:22Z

Is this getting merged? I mean who doesn't want faster compilation times :D

rpellerin · 2018-05-02T15:01:07Z

Would love to see this merged too!

montogeek · 2018-07-04T00:43:02Z

@ooflorent @sokra Can we merge this one? It seems it works :)

alan-agius4 · 2018-07-10T18:52:25Z

1yr later and this PR is not merged yet! This is just ridiculous.

TheAlexLichter · 2018-07-24T07:31:55Z

Would be awesome to see this merged! 🙏

mlavina · 2018-08-27T15:17:39Z

Hey @sokra @ooflorent

Just checking in as well if this can be merged in and released soon?

improve tests

sokra · 2018-08-29T12:07:22Z

🎉 finally merged

Sorry for the delay, this was difficult to review

sokra · 2018-08-29T12:10:06Z

and released as 1.2.0

alan-agius4 · 2018-08-29T12:10:50Z

thank you :)

…

On Wed, 29 Aug 2018 at 14:10, Tobias Koppers ***@***.***> wrote: and released as 1.2.0 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#23 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQv-WteNxcCLROvWIsoYIWXlsaC3wC99ks5uVoShgaJpZM4Oe2IV> .

eranimo · 2018-08-29T14:37:43Z

Is it possible to create a beta release version of webpack with this new version?

alexander-akait · 2018-08-29T14:40:39Z

@eranimo why? It is not major release, just update your lock file. Look on https://github.com/webpack/webpack/blob/master/package.json#L32

edmorley · 2018-08-29T16:41:43Z

This decreased the time taken for our warm (babel-loader and uglify-webpack-plugin caches enabled) webpack 4 --mode production devtool: 'source-map' builds by 30%. Much appreciated - thank you :-)

Single pass ReplaceSource node

b2cbbe4

Update ReplaceSource .node to process all replacements in a single pass

Add unit test coverage for ReplaceSource

6ac5075

TheLarkInn requested a review from sokra August 29, 2017 19:32

Merge branch 'master' into single-pass-replacesource-node

dc35c1a

Fix double emit on in progress replace and adjust node column for ove…

d178f18

…rlapping replaces

ooflorent reviewed Jan 11, 2018

View reviewed changes

Move ReplaceSource replacement sort method outside of class

441ef8b

ooflorent approved these changes Mar 2, 2018

View reviewed changes

sokra mentioned this pull request Mar 21, 2018

Webpack 4 irregularly spikes CPU at 93% "after chunk asset optimization" during hot reload rebuild, CPU loads ~1.25 cores, sometimes Node dies with out-of-memory error webpack/webpack#6812

Closed

dotneet mentioned this pull request Apr 5, 2018

Why does the build process take 15sec+? nuxt/nuxt#2433

Closed

edmorley mentioned this pull request Apr 25, 2018

Update to webpack 4 neutrinojs/neutrino#809

Merged

4 tasks

alan-agius4 mentioned this pull request Jun 14, 2018

Improve performance of node method in ReplaceSources #29

Closed

mlavina mentioned this pull request Aug 27, 2018

Please upgrade source-map to 0.7.2 to get speed improvements webpack-contrib/uglifyjs-webpack-plugin#248

Open

sokra added 2 commits August 29, 2018 12:15

Simplify and inline methods

89e8ebf

improve tests

Improve the quality of the source maps in some cases

cf11460

sokra approved these changes Aug 29, 2018

View reviewed changes

sokra merged commit b141232 into webpack:master Aug 29, 2018

Single pass ReplaceSource .node performance improvement #23

Single pass ReplaceSource .node performance improvement #23

Conversation

brentwilton commented Jul 21, 2017 • edited Loading

codecov bot commented Jul 21, 2017 • edited Loading

Codecov Report

brentwilton commented Aug 14, 2017

TheLarkInn commented Aug 29, 2017

alan-agius4 commented Nov 16, 2017 • edited Loading

TheLarkInn commented Nov 16, 2017

alan-agius4 commented Nov 25, 2017

brentwilton commented Nov 30, 2017

alan-agius4 commented Nov 30, 2017 • edited Loading

sokra commented Dec 1, 2017

brentwilton commented Dec 2, 2017

vladimir-tikhonov commented Dec 8, 2017

brentwilton commented Dec 10, 2017

brentwilton commented Dec 11, 2017

vladimir-tikhonov commented Dec 11, 2017

alan-agius4 commented Jan 7, 2018

TheLarkInn commented Jan 10, 2018 via email

ooflorent left a comment

Choose a reason for hiding this comment

ooflorent Jan 11, 2018

Choose a reason for hiding this comment

alan-agius4 Jan 18, 2018

Choose a reason for hiding this comment

brentwilton Mar 2, 2018 • edited Loading

Choose a reason for hiding this comment

alan-agius4 commented Jan 25, 2018

TheLarkInn commented Jan 25, 2018

abhinavsingi commented Mar 31, 2018 • edited Loading

ghost commented Apr 12, 2018 • edited by ghost Loading

alan-agius4 commented Apr 24, 2018

ghost commented Apr 24, 2018 • edited by ghost Loading

vohzd commented Apr 26, 2018

rpellerin commented May 2, 2018

montogeek commented Jul 4, 2018

alan-agius4 commented Jul 10, 2018

TheAlexLichter commented Jul 24, 2018

mlavina commented Aug 27, 2018

sokra commented Aug 29, 2018

sokra commented Aug 29, 2018

alan-agius4 commented Aug 29, 2018 via email

eranimo commented Aug 29, 2018

alexander-akait commented Aug 29, 2018

edmorley commented Aug 29, 2018

brentwilton commented Jul 21, 2017 •

edited

Loading

codecov bot commented Jul 21, 2017 •

edited

Loading

alan-agius4 commented Nov 16, 2017 •

edited

Loading

alan-agius4 commented Nov 30, 2017 •

edited

Loading

brentwilton Mar 2, 2018 •

edited

Loading

abhinavsingi commented Mar 31, 2018 •

edited

Loading

ghost commented Apr 12, 2018 •

edited by ghost

Loading

ghost commented Apr 24, 2018 •

edited by ghost

Loading