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)
| moduleLayerCache = new ModuleLayerCache(); | ||
| cachedModuleLayerCaches.set(associatedObjectForCache, moduleLayerCache); | ||
| } | ||
| if (moduleLayerCache.has(this.userRequest)) { |
There was a problem hiding this comment.
| if (moduleLayerCache.has(this.userRequest)) { | |
| if (moduleLayerCache.has(this.request)) { |
| @@ -780,6 +784,31 @@ class NormalModule extends Module { | |||
| * @returns {Source} the created source | |||
| */ | |||
| createSource(context, content, sourceMap, associatedObjectForCache) { | |||
| // If this module exists in a layer, try to reuse the value for the | |||
| // source string so it is not duplicated in multiple modules. | |||
| if (this.layer && this.userRequest) { | |||
There was a problem hiding this comment.
| if (this.layer && this.userRequest) { | |
| if (this.layer && this.request) { |
There was a problem hiding this comment.
You need to use userRequest since at least in Next.js App Router, the values of request will be different for the same file across the different layers.
There was a problem hiding this comment.
Could you use resource or resourcePath?
| cachedModuleLayerCaches.set(associatedObjectForCache, moduleLayerCache); | ||
| } | ||
| if (moduleLayerCache.has(this.userRequest)) { | ||
| const cachedSource = moduleLayerCache.get(this.userRequest); |
There was a problem hiding this comment.
| const cachedSource = moduleLayerCache.get(this.userRequest); | |
| const cachedSource = moduleLayerCache.get(this.request); |
| /** @type {Map<string, WeakMapKey>} */ | ||
| this.weakMapKeys = new Map(); | ||
| /** @type {WeakMap<WeakMapKey, string | Buffer>} */ | ||
| this.cachedValues = new WeakMap(); |
There was a problem hiding this comment.
This is not "weak". this.weakMapKeys holds a strong reference to the WeakMapKey, so it can't be collected while ModuleLayerCache is active. So the whole class is equal to a single Map<string, string | Buffer>
There was a problem hiding this comment.
Is there another key that you could use? If so, just let me know. You need to use userRequest (a string) and therefore it has to be able to be a key in the WeakMap somehow. This holds on to the values during the lifetime of the Compilation object but then is thrown away.
There was a problem hiding this comment.
I think you can't make it "weak" on the values. That's won't work with string keys. But I think that might not be needed. But you need to make it "weak" on associatedObjectForCache that should be enough.
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.