perf: singly-linked StackEntry for resolver stack with Set-like API#526
perf: singly-linked StackEntry for resolver stack with Set-like API#526alexander-akait merged 1 commit intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 27f13f1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Merging this PR will improve performance by 48.65%
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #526 +/- ##
==========================================
- Coverage 96.75% 96.52% -0.23%
==========================================
Files 50 50
Lines 2589 2622 +33
Branches 788 794 +6
==========================================
+ Hits 2505 2531 +26
- Misses 69 76 +7
Partials 15 15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
alexander-akait
left a comment
There was a problem hiding this comment.
Let's merge, I don't think anyone uses add method for stack, perf improvements are looking very good,
The StackEntry refactor (#526) replaced the internal Set-based stack with a singly-linked list. However, the public API still allows callers to pass a Set<string> as resolveContext.stack. When doResolve receives a Set as parent, Set.has(stackEntry) compares by reference and never matches the new StackEntry object, so recursion detection silently fails. Fix: check `parent instanceof StackEntry` and fall back to `parent.has(stackEntry.toString())` for Set compatibility. https://claude.ai/code/session_01TBuEDZmTzjKYzebY26ZkL1
… set `doResolve` runs once per plugin step (~10-15 per resolve). When `resolveContext.log` is unset — the production hot path — the inner context is structurally identical to the parent save for `stack`, so we can mutate `stack` in place before the hook call and restore it after, avoiding one options-literal + one `createInnerContext` return object per step. `forEachBail` and tapable's `AsyncSeriesBailHook` drive sibling calls strictly sequentially, so no two call sites read `resolveContext.stack` concurrently. The logging branch remains unchanged, since it still needs the deferred header wrapper from `createInnerContext`. Benchmark on top of main (which already has the singly-linked stack from #526), median of 5 runs under the repo's `--no-opt --predictable` flags: concurrent-batch (15 parallel resolves): 1.075 -> 1.045 ms (+2.8%) All 970 tests pass, including the recursion detection suite.
Summary
Replace
Set<string>-based stack with a singly-linkedStackEntryclass that exposes a Set-compatible API (has, iteration,size,toString). EachdoResolvecall prepends a node instead of cloning a Set.Comparison
claude/pr-443-...){ name, path, request, ..., parent }linked list{ entry, parent }linked list, hidden behind aSet<string>getterStackEntryclass linked listdoResolvehot pathhasStackEntryfield-compareinstanceof Setbranch + possiblesetToStackconversion + eager string buildnew StackEntry(..., parent)+ field-comparehasstack.has(entry)(public API).hasstack instanceof Setstack.add(x)ResolverCachePluginCodSpeed CI benchmarks (vs
mainBASE)PR #523 regresses
cache-predicateby 45% because itsdoResolvehot path adds aninstanceof Setbranch +_stack \|\| stackfallback + lazy Set getter materialization. For paths that hit UnsafeCachePlugin and skip the rest of the plugin chain (already microsecond-scale), that per-call overhead dominates.This PR avoids the regression by keeping a single representation (the linked list) with no branching or materialization on the hot path — every benchmark improves, none regress.
What kind of change does this PR introduce?
perf
Did you add tests for your changes?
Does this PR introduce a breaking change?
If relevant, what needs to be documented once your changes are merged or what have you already documented?
Use of AI