Skip to content
Please note that GitHub no longer supports Internet Explorer.

We recommend upgrading to the latest Microsoft Edge, Google Chrome, or Firefox.

Learn more
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

improve performance of ModuleConcatenationPlugin for loop #6613

Conversation

@brentwilton
Copy link
Contributor

brentwilton commented Feb 28, 2018

What kind of change does this PR introduce?

bugfix / performance improvement

Did you add tests for your changes?

No

If relevant, link to documentation update:

N/A

Summary

In our large Angular 5 project (~3400 modules) ModuleConcatenationPlugin is now taking 60% of the build time in webpack 4.

Webpack is spending 869499ms in 'chunk modules optimization' within ModuleConcatenationPlugin.

Most of this time is consumed in 2 nested for-of loops in the ModuleConcatenationPlugin. By changing the for-of loops to standard for loops we have reduced the time we spend in ModuleConcatenationPlugin from 14 minutes 29 seconds to 2 minutes 35 seconds.

Does this PR introduce a breaking change?

No

Other information

Node: 8.9.3
Webpack: 4.0.1 and 4.0.0
Windows 10

During our prod build the inner for-of loop is visited a total of 7,309,580,444 (7.3 billion) times.

When debugging most of the time was spent in the following 2 for loops in ModuleConcatenationPlugin.js. Original time taken: 14m 29s (869499ms)

for (const dep of newModule.dependencies) {
if (dep.module) {
for (const reason of dep.module.reasons) {
if (reason.dependency === dep) reason.module = newModule;
}
}
}

Changing the above for of loop to a standard for loop reduces the time taken to 6m 8s (368453ms)

for (let i = 0; i < newModule.dependencies.length; i++) {
    let dep = newModule.dependencies[i];
    if (dep.module) {
        for (let j = 0; j < dep.module.reasons.length; j++) {
            let reason = dep.module.reasons[j];
            if (reason.dependency === dep) reason.module = newModule;
        }
    }
}

Making a further change to let reasons = dep.module.reasons; reduces the time taken to 2m 35s (141326ms)

for (let i = 0; i < newModule.dependencies.length; i++) {
    let dep = newModule.dependencies[i];
    if (dep.module) {
        let reasons = dep.module.reasons;
        for (let j = 0; j < reasons.length; j++) {
            let reason = reasons[j];
            if (reason.dependency === dep) reason.module = newModule;
        }
    }
}
@ooflorent

This comment has been minimized.

Copy link
Member

ooflorent commented Feb 28, 2018

Such difference is really odd. When I made these changes and benchmarked them, the results were on par. It looks like a bug in v8. The escape analysis should produce optimal code for this hot path. @bmeurer, can you advice us on this?

@phenomnomnominal

This comment has been minimized.

Copy link

phenomnomnominal commented Feb 28, 2018

I'm working on a minimal repro for this situation, not having a heap of luck. I can see some difference, but not the massive drop. I'll check with @brentwilton at work tomorrow and see what we can figure out.

@bmeurer

This comment has been minimized.

Copy link

bmeurer commented Feb 28, 2018

As mentioned in the Twitter thread, I'm pretty sure this is a bug in TurboFan where something odd prevents the array iterator inlining, which in turn defeats escape analysis and all other optimizations. We'll need some repro for this, ideally something that doesn't involve the rest of webpack. 😀

Maybe as a starting point: Are the shapes involved monomorphic? I.e. do you have different dep or dep.module.reasons?

@phenomnomnominal

This comment has been minimized.

Copy link

phenomnomnominal commented Feb 28, 2018

@bmeurer

This comment has been minimized.

Copy link

bmeurer commented Feb 28, 2018

@psmarshall will take a look when you have a repro. If it's a small fix we might be able to patch this in Node 8 as well.

@ooflorent

This comment has been minimized.

Copy link
Member

ooflorent commented Feb 28, 2018

@bmeurer no, it's not monomorphic. dep instances may be created from different classes that share the same base class (Dependency).

@clydin

This comment has been minimized.

Copy link

clydin commented Feb 28, 2018

While this appears to be a v8 defect, this is causing major performance problems with large Angular CLI projects; essentially wiping out and reversing the performance benefits of Webpack 4. Please considering merging this as soon as possible.

@@ -281,9 +281,12 @@ class ModuleConcatenationPlugin {
for (const reason of newModule.reasons) {
reason.dependency.module = newModule;
}
for (const dep of newModule.dependencies) {
for (let i = 0; i < newModule.dependencies.length; i++) {

This comment has been minimized.

Copy link
@sokra

sokra Mar 1, 2018

Member

Could you add a comment explaining why this isn't a for-of loop pointing to this PR and adding // TODO: remove when LTS node version contains fixed v8 version

@bmeurer

This comment has been minimized.

Copy link

bmeurer commented Mar 1, 2018

Ok, I have a repro for V8. It's because the inlining of Array.prototype[Symbol.iterator] and ArrayIterator.prototype.next get's confused by polymorphic input arrays. Simple test case to illustrate the problem:

const N = 1e7;

function forOf(a) {
  for (const x of a) {}
}

function test(f, a) {
  let result;
  for (let i = 0; i < N; ++i) result = f(a);
  return result;
}

const a = [1,2,3,4];
const b = ['a', 'b', 'c', 'd'];

test(forOf, a);
for (let i = 0; i < 100; ++i) test(x => x, a);

console.time('mono');
test(forOf, a);
console.timeEnd('mono');

test(forOf, b);

console.time('poly');
test(forOf, a);
console.timeEnd('poly');

Filed v8:7510 upstream.

@webpack-bot

This comment has been minimized.

Copy link
Contributor

webpack-bot commented Mar 2, 2018

@brentwilton Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@sokra Please review the new changes.

@webpack-bot

This comment has been minimized.

Copy link
Contributor

webpack-bot commented Mar 2, 2018

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

@sokra
sokra approved these changes Mar 2, 2018
@sokra sokra merged commit 2e687d0 into webpack:master Mar 3, 2018
11 of 12 checks passed
11 of 12 checks passed
codecov/changes/integration 1 file has unexpected coverage changes not visible in diff.
Details
Codacy/PR Quality Review Good work! A positive pull request.
Details
codecov/changes/unit No unexpected coverage changes found.
Details
codecov/patch/integration 100% of diff hit (target 90%)
Details
codecov/patch/unit 0% of diff hit (target 0%)
Details
codecov/project/integration Absolute coverage decreased by -<.01% but relative coverage increased by +9.19% compared to 6a5d081
Details
codecov/project/unit 23.22% (target 0%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.002%) to 92.499%
Details
licence/cla Contributor License Agreement is signed.
Details
security/snyk No dependency changes
Details
@sokra

This comment has been minimized.

Copy link
Member

sokra commented Mar 3, 2018

Thanks

@bmeurer

This comment has been minimized.

Copy link

bmeurer commented Mar 6, 2018

Thanks for the heads-up. The bug (v8:7510) was just fixed upstream. Unfortunately just missed 6.6, so it might not make it into Node 10, unless we can float this patch on top.

@ronkorving ronkorving mentioned this pull request Mar 6, 2018
@bmeurer bmeurer mentioned this pull request Mar 27, 2018
2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.