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

feat: monomorphic ResolveRequest object #445

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dmichon-msft
Copy link
Contributor

@dmichon-msft dmichon-msft commented Jan 10, 2025

Improves performance all-up (especially in ParsePlugin, JoinRequestPlugin, JoinRequestPartPlugin) by ensuring that the ResolveRequest object 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 ResolveRequest object to pass them between plugins and out of the resolver, instead of using the context property.

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:
image
image

After:
image
image

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

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,

lib/ParsePlugin.js Show resolved Hide resolved
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

@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

  1. Implement special place to store custom values for authors (maybe just Map?)
  2. Make internal helper for request generation and implement an option to back compat
  3. Rewrite it in webpack and test it

Alternative solution, maybe even better, but need to check perf

  1. Check request in factory (we can use Symbol isPure) - if no unknown properties it is pure (we known them), otherwise false
  2. Make internal helper for creating request - if it isPure request we can use your logic and get perf improvement, if no just use olg logic
  3. (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

@dmichon-msft
Copy link
Contributor Author

@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

  1. Implement special place to store custom values for authors (maybe just Map?)
  2. Make internal helper for request generation and implement an option to back compat
  3. Rewrite it in webpack and test it

Alternative solution, maybe even better, but need to check perf

  1. Check request in factory (we can use Symbol isPure) - if no unknown properties it is pure (we known them), otherwise false
  2. Make internal helper for creating request - if it isPure request we can use your logic and get perf improvement, if no just use olg logic
  3. (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 context property is already a completely opaque object that is forwarded through the request chain without being inspected or modified in any way, so we ought to be able to just use that for the ResolverCachePlugin scenario.

@dmichon-msft
Copy link
Contributor Author

I have some thoughts on ways to make this behavior be a resolver option, will refactor.

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.

3 participants