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: single-linked list for resolver stack #443

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

Conversation

dmichon-msft
Copy link

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

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.

@dmichon-msft dmichon-msft changed the title feat: Use single-linked list for resolver stack feat: single-linked list for resolver stack Jan 10, 2025
@TheLarkInn TheLarkInn requested a review from Copilot January 13, 2025 21:18
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 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)

lib/Resolver.js Outdated Show resolved Hide resolved
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.

2 participants