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

Use shorter identifier for ConcatenatedModules #5997

Merged
merged 1 commit into from
Nov 22, 2017
Merged

Use shorter identifier for ConcatenatedModules #5997

merged 1 commit into from
Nov 22, 2017

Conversation

filipesilva
Copy link
Contributor

What kind of change does this PR introduce?
Memory usage optimization.

Did you add tests for your changes?
I couldn't find a test for module names, it doesn't seem like it is tested currently.

If relevant, link to documentation update:

Summary
See #5992 (comment) for rationale.

Fixes #5992

Does this PR introduce a breaking change?
No.

Other information

Copy link
Contributor

@lencioni lencioni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for digging into this! I'm excited about this PR 💃

case "concatenated":
return info.module.identifier();
}
}).filter(Boolean).join(" ");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of mapping, filtering, and joining, I wonder if this would be faster and use less memory if you call hash.update while iterating in a forEach:

const hash = crypto.createHash("md5");
this._orderedConcatenationList.forEach(info => {
  if (info.type === "concatenated") {
    hash.update(info.module.identifier() + " ");
  }
});
return this.rootModule.identifier() + " " + hash.digest("hex");

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, but the difference is probably negligible. Profiling should tell us either way.

If we want raw speed, md4 might be worth considering as well. Not sure about collisions though.

Copy link
Contributor

@lencioni lencioni Nov 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I've also had good success using string-hash in client code, but that isn't set up to accept input in chunks like crypto stuff. https://github.com/darkskyapp/string-hash

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good to me. I think in that case, this PR should stick with md5 and then can be upgraded with whatever you land on after that testing is complete.

I'd love to try this out at Airbnb ASAP--is there any chance we can get this in and get a new webpack 3 release out?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only reasoning for picking this algorithm was consistency really. It is what NormalModule is using. Happy to change it to whatever.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleaned out the comments that really have no bearing on this particular PR.

@lencioni - I do believe that's the plan though webpack/webpack isn't my domain. I'm going to work on the hash testing now. Assuming the results are positive, i'll tag you in that PR as well given it's probably going to be applicable to airbnb's perf issues as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@filipesilva - Leave it the way it is, we kind of went off on a tangent that is likely to lead to more perf improvements but needs to be done across the entire project.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleanup: #6001

@webpack-bot
Copy link
Contributor

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

@sokra sokra merged commit 20759bb into webpack:master Nov 22, 2017
@sokra
Copy link
Member

sokra commented Nov 22, 2017

Thanks

lencioni added a commit to lencioni/webpack that referenced this pull request Nov 22, 2017
This was implemented in webpack#5997 to fix some memory issues, but I think we
can make this a little more efficient. By using a single forEach instead
of a map, filter, and join, we avoid unnecessary array creations and
iterations.
lencioni added a commit to lencioni/webpack that referenced this pull request Nov 23, 2017
This was implemented in webpack#5997 to fix some memory issues, but I think we
can make this a little more efficient. By using a single forEach instead
of a map, filter, and join, we avoid unnecessary array creations and
iterations.
@sokra
Copy link
Member

sokra commented Nov 25, 2017

#5718

@lencioni
Copy link
Contributor

I just tried out v3.9.1 at Airbnb, and the memory problems were were seeing seem to be resolved now! Thanks so much for this!

elliottsj added a commit to elliottsj/rushstack that referenced this pull request Apr 4, 2018
webpack@~3.11.0 includes a fix for an out-of-memory issue; see:
webpack/webpack#5992
webpack/webpack#5997
nickpape pushed a commit to microsoft/rushstack that referenced this pull request Apr 5, 2018
* Upgrade webpack to ~3.11.0

webpack@~3.11.0 includes a fix for an out-of-memory issue; see:
webpack/webpack#5992
webpack/webpack#5997

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

Successfully merging this pull request may close these issues.

ModuleConcatenationPlugin memory usage increases dramatically with chunk module count
5 participants