feat: monomorphic ResolveRequest object#445
feat: monomorphic ResolveRequest object#445dmichon-msft wants to merge 3 commits intowebpack:mainfrom
Conversation
There was a problem hiding this comment.
Copilot reviewed 5 out of 20 changed files in this pull request and generated 1 comment.
Files not reviewed (15)
- lib/ResolverFactory.js: Evaluated as low risk
- lib/ModulesInHierarchicalDirectoriesPlugin.js: Evaluated as low risk
- lib/SymlinkPlugin.js: Evaluated as low risk
- lib/ModulesInRootPlugin.js: Evaluated as low risk
- lib/AppendPlugin.js: Evaluated as low risk
- lib/DescriptionFilePlugin.js: Evaluated as low risk
- lib/JoinRequestPartPlugin.js: Evaluated as low risk
- lib/ExportsFieldPlugin.js: Evaluated as low risk
- lib/AliasFieldPlugin.js: Evaluated as low risk
- lib/ResultPlugin.js: Evaluated as low risk
- lib/AliasPlugin.js: Evaluated as low risk
- lib/RootsPlugin.js: Evaluated as low risk
- lib/ImportsFieldPlugin.js: Evaluated as low risk
- lib/CloneBasenamePlugin.js: Evaluated as low risk
- lib/JoinRequestPlugin.js: Evaluated as low risk
Comments suppressed due to low confidence (1)
lib/ParsePlugin.js:55
- The logic for setting the 'fragment' property might not correctly handle cases where both 'parsed.fragment' and 'request.fragment' are falsy. Consider using 'parsed.fragment ?? request.fragment' to handle these cases correctly.
fragment: parsed.fragment || request.fragment || parsed.fragment,
alexander-akait
left a comment
alexander-akait
left a comment
There was a problem hiding this comment.
@dmichon-msft Yeah, I am afraid it is a breaking change...
In webpack we have such things - https://github.com/webpack/webpack/blob/main/lib/cache/ResolverCachePlugin.js#L147 and https://github.com/webpack/webpack/blob/main/lib/cache/ResolverCachePlugin.js#L150
Technically we can solve this and add a special place to store such things.
I have only remembered one place where we use it, maybe somewhere else, but it is quite possible that this is the only place
So let's try to solve it step by step
- Implement special place to store custom values for authors (maybe just Map?)
- Make internal helper for request generation and implement an option to back compat
- Rewrite it in webpack and test it
Alternative solution, maybe even better, but need to check perf
- Check request in factory (we can use Symbol
isPure) - if no unknown properties it is pure (we known them), otherwisefalse - Make internal helper for creating request - if it
isPurerequest we can use your logic and get perf improvement, if no just use olg logic - (Optional) And also provide a place to store custom values for authors, because to bring such improvements to webpack
So we will keep old behaviour and have perf improvement for some places
The |
|
I have some thoughts on ways to make this behavior be a resolver option, will refactor. |
|
Yeah, an option is a good way here, also maybe we should add a place where we can store custom values, so after merge this we can update logic inside webpack and get perf (we need a place where to store custom values inside webpack for caches) |
Rewrites the `{ ...request, ... }` spread pattern used across every
request-producing plugin into a shared `cloneRequest` helper that writes
the canonical ResolveRequest fields as a fixed-order object literal.
V8 can then cache a single hidden class for the result, matching the
perf win from #445 (ParsePlugin, JoinRequestPlugin, JoinRequestPartPlugin)
without dropping unknown own properties — after the literal, any extra
string or symbol keys on the original request are copied over, so
plugin authors (e.g. webpack's ResolverCachePlugin) that attach custom
state to the request keep working unchanged.
…atible) Re-implements the perf wins from PR #445 (constant-shape ResolveRequest objects so V8 keeps a single hidden class on the resolver hot path) without the breaking change for plugin authors that attach custom properties directly to a request. Each plugin now builds the new ResolveRequest as an explicit object literal in a fixed property order. A new `lib/util/cloneRequest.js` helper exports `copyExtras(target, source)` which copies any non-canonical own string keys plus any symbol keys from the source request, preserving the historical behavior of `{ ...request, ... }` for callers that pass through values via custom keys (e.g. webpack's ResolverCachePlugin). ParsePlugin keeps its public `(source, requestOptions, target)` constructor; the partial overrides are applied via `Object.assign` after the canonical literal so the `{ fullySpecified }` use case used by ResolverFactory does not transition the hidden class. MainFieldPlugin keeps its `alreadyTriedMainField` Symbol-based recursion guard, set after `copyExtras` so the current attempt's value always wins. Adds a regression test in `test/plugins.test.js` that taps the resolve hook to attach both a string-keyed and a Symbol-keyed property and asserts both survive through the plugin chain to `hooks.result`.
Improves performance all-up (especially in
ParsePlugin,JoinRequestPlugin,JoinRequestPartPlugin) by ensuring that theResolveRequestobject always has a constant shape.Note that this is technically a breaking change to plugin authors, if they took a dependency on the ability to attach arbitrary properties to the
ResolveRequestobject to pass them between plugins and out of the resolver, instead of using thecontextproperty.Additionally, creating the object via a direct object literal is significantly faster than creating it via object spread.
Testing on NodeJS 18.19.1, this had the following impact for a local webpack build.
Before:
After:
