feat: single-linked list for resolver stack

[dmichon-msft]

Improve performance of doResolve by representing the resolver stack via a singly-linked list instead of a Set<string>. Profiling indicated that a significant amount of time was spent cloning resolveContext.stack during doResolve; replacing with a singly-linked list reduces the total memory footprint and makes adding a new entry be O(1) in both time and memory, in exchange for making the circularity check be O(n).

Additionally, this avoids allocating strings for the formatted stack until required when logging it.

Local profiling showed a reduction from 2400ms of self time in doResolve to 88ms of self time plus 73ms for the cost of hasStackEntry, a net reduction of 2239ms.
Measured on NodeJS 18.19.1.

Note that this is a breaking change that will require an update to webpack's ResolverCachePlugin.

[Copilot]

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

lib/Resolver.js:748

• [nitpick] The error message string might be unclear. Consider updating it to provide more context about the recursion.

"Recursion in resolving\nStack:" + formatStack(stackEntry)

[alexander-akait]

@dmichon-msft Great work!

I would like to see this improvement for webpack,

Note that this is a breaking change that will require an update to webpack's ResolverCachePlugin.

Which places we need to rewrite?

Maybe we can implement this under an option (or internal value for resolver like resolver.optimizaStack = true)? So we will mark old logic as deprecated (and put todo for removing it in the next major release), then rewrite some things in webpack and use it

What do you think?

[dmichon-msft]

@dmichon-msft Great work!

I would like to see this improvement for webpack,

Note that this is a breaking change that will require an update to webpack's ResolverCachePlugin.

Which places we need to rewrite?

Maybe we can implement this under an option (or internal value for resolver like resolver.optimizaStack = true)? So we will mark old logic as deprecated (and put todo for removing it in the next major release), then rewrite some things in webpack and use it

What do you think?

The only change to ResolverCachePlugin (which does still function without the change, it just does unnecessary work), is the removal of the new Set() here and replacing with undefined:
https://github.com/webpack/webpack/blob/a5ad13c092158d203f799bc0d12a8556e8876f20/lib/cache/ResolverCachePlugin.js#L154-L164

We could fork the logic here, though it only impacts plugins that inspect or instantiate ResolverContext.

[alexander-akait]

@dmichon-msft

We could fork the logic here, though it only impacts plugins that inspect or instantiate ResolverContext.

Can you explain?

I would prefer a solution that will allow to avoid critical changes now and get perf improvements right now, That's is why I thought about a new option, my concern is also related to the fact that we don’t know how other developers use our code, we have very many downloads every week.

[dmichon-msft]

I have some thoughts on how we can make this be a resolver option and keep the public API almost the same even when enabled (e.g. make stack still have the same shape if read by a plugin)

[alexander-akait]

@dmichon-msft Will be great to look at this ⭐

Also how you do profiling/benches, can you provide a code?

I think will be great to add it in our repo for future improvement and checks (if you have time feel free to send a separate PR with it)