mknichel/dedupe strings in module layers #18340
Conversation
|
|
For maintainers only:
|
| } else { | ||
| moduleLayerCache.set(module.userRequest, data); | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we move it to method, to avoid duplicate logic (for example to the compilation class)
There was a problem hiding this comment.
Done. Sorry for the delay. Let me know if this is what you had in mind.
There was a problem hiding this comment.
You need to put that behind and experiments flag.
Currently this is not a plain improvement. It will leak memory as all module sources stay in the cache even if you remove the import to the module. I think it would be better to remove modules that are not longer in the compilation from the cache.
|
FYI @alexander-akait, @sokra and I met to discuss a path forward. Instead of this approach, we want to add a generic string and Buffer interning logic inside https://github.com/webpack/webpack-sources. In Webpack, we can enable + disable interning via an experiment and at the start and end of a compilation so that users can opt in to this behavior and no memory is leaked beyond the compilation. I'm going to start prototyping that solution. @sokra will be unavailable starting in 2 days, so let me know if this plans sounds good to you and whether you might be able to review that while @sokra is OOO. |
What kind of change does this PR introduce?
This pull request adds a layer of caching for strings that could be duplicated in modules across multiple layers. This change was motivated by an observation that there were many strings in the heap duplicated in a Next.js application that uses App Router, which uses layers in the implementation. After this pull request, these strings were deduplicated saving the memory for any file that appears in multiple layers.
The cache here is associated with the compilation in a weak map so it can be garbage collected after the compilation finishes.
There should be no visible behavior difference after this pull request.
Did you add tests for your changes?
New tests were not added since the behavior should be covered by existing integration tests that use layers.
Does this PR introduce a breaking change?
No, this PR is not a breaking change. No user visible behavior is affected.
What needs to be documented once your changes are merged?
No changes would need to be documented after this pull request. This pull request only affects internal implementation details.