Cache last-successful union branch index per schema instance#1439
Open
heiwen wants to merge 3 commits intoopen-circle:mainfrom
Open
Cache last-successful union branch index per schema instance#1439heiwen wants to merge 3 commits intoopen-circle:mainfrom
heiwen wants to merge 3 commits intoopen-circle:mainfrom
Conversation
Add a module-level `WeakMap` that records, for each `union` schema instance, the index of the option that matched on the most recent successful parse. On the next call the cached index is tried first; if it matches, the remaining options are skipped entirely. Falls back to the linear scan when the hint misses or the schema has never matched. Schemas derived via `omit`/`pick`/`partial`/`required` share the same object identity so the hint naturally tracks per instance. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughA module-level Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
dosubot
Bot
added
size:S
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@library/src/schemas/union/union.ts`:
- Around line 129-130: The early-return that returns lastDataset as
SuccessDataset<unknown> (when lastDataset.typed && !lastDataset.issues) breaks
left-to-right winner semantics for overlapping unions; change the fast-path so
it only triggers for unions proven disjoint (e.g., gate on a disjoint
flag/property or add an isDisjoint check) or remove the early return entirely so
earlier branches are still attempted, and apply the same fix at the analogous
check around lines 154-155; add a regression test in
library/src/schemas/union/union.test.ts that exercises union([literal('foo'),
string()]) (parse 'bar' then 'foo') to assert the first branch wins and
transforms/defaults behave correctly.
- Around line 124-128: The cached lastIndex is being dereferenced without
verifying it still points into the current this.options array (UnionOptions is
stored by reference); update the union() logic around lastIndex/lastDataset so
you first verify lastIndex is a valid index (e.g., lastIndex >= 0 && lastIndex <
this.options.length && this.options[lastIndex] && typeof
this.options[lastIndex]['~run'] === 'function') before calling
this.options[lastIndex]['~run'](...); if the guard fails, clear or ignore
lastIndex and fall back to the existing linear scan over this.options to compute
lastDataset as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f75f1f66-48b7-4398-9570-fc39179d53fa
📒 Files selected for processing (1)
library/src/schemas/union/union.ts
- Guard lastIndex with < this.options.length in case options is mutated - Only early-return on cache hit when lastIndex === 0; for higher indexes the full loop still runs (reusing the cached dataset) so earlier branches with higher priority are not skipped Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@heiwen is attempting to deploy a commit to the Open Circle Team on Vercel. A member of the Team first needs to authorize it. |
dosubot
Bot
added
size:M
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@library/src/schemas/union/union.ts`:
- Around line 119-143: The current fast-path reuses a cached branch result from
_lastSuccessIndex even when lastIndex > 0, causing speculative execution of
later branches; modify the logic so the cached optionDataset is only reused when
lastIndex === 0 (i.e., only allow the short-circuit fast path for index 0) —
remove/guard the reuse for any other lastIndex to avoid running non-priority
branches, update the conditional around this.options[i]['~run'] to reflect this
change, and add a regression test in library/src/schemas/union/union.test.ts
that demonstrates union([literal('foo'), pipe(string(), transform(sideEffect))])
does not invoke the transform when branch 0 matches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d0a068d3-148a-4f84-82d3-5ab0c0a72c0f
📒 Files selected for processing (1)
library/src/schemas/union/union.ts
Restricts the early-return to lastIndex === 0, eliminating pre-computation for higher indexes which could run transform side effects out of order. The full loop still updates the cache so index 0 is reached naturally. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
dosubot
Bot
added
size:S
There was a problem hiding this comment.
♻️ Duplicate comments (2)
library/src/schemas/union/union.ts (2)
122-124:⚠️ Potential issue | 🟡 MinorRestore the empty-array guard on the fast path.
Line 123 now dereferences
this.options[0]whenever the cache holds0. Becauseunion()keeps the caller'soptionsarray by reference, shrinking it to[]after a successful parse turns this intoundefined['~run'](...)instead of falling back to the normal scan.🛡️ Minimal guard
- if (_lastSuccessIndex.get(this) === 0) { + if (_lastSuccessIndex.get(this) === 0 && this.options.length > 0) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@library/src/schemas/union/union.ts` around lines 122 - 124, The fast-path currently dereferences this.options[0] when _lastSuccessIndex.get(this) === 0 which breaks if the caller mutated options to an empty array; restore an empty-array guard before calling this.options[0]['~run'] (e.g., check this.options && this.options.length > 0 or similar) and fall back to the normal scan path when options is empty so union() does not attempt to call '~run' on undefined; ensure this change touches the block that computes fastDataset and retains existing behavior when options is non-empty.
122-128:⚠️ Potential issue | 🔴 CriticalReuse the warmed option-0 result on fallback.
If Lines 127-128 do not return, Line 134 executes option
0again. After the cache is warmed, a failing first branch now runs any transforms or user callbacks twice, which is an observable behavior change from the pre-cache implementation.🔧 Reuse the first dataset instead of re-running branch 0
+ let firstOptionDataset: + | ReturnType<(typeof this.options)[number]['~run']> + | undefined; + // Fast path: if the first option succeeded last time, try it first. // Restricted to index 0 to preserve left-to-right priority semantics and // avoid running side-effecting branches (transforms) out of order. if (_lastSuccessIndex.get(this) === 0) { - const fastDataset = this.options[0]['~run']( + firstOptionDataset = this.options[0]['~run']( { value: dataset.value }, config ); - if (fastDataset.typed && !fastDataset.issues) { - return fastDataset as SuccessDataset<unknown>; + if (firstOptionDataset.typed && !firstOptionDataset.issues) { + return firstOptionDataset as SuccessDataset<unknown>; } } // Parse schema of each option and collect datasets for (let i = 0; i < this.options.length; i++) { - const optionDataset = this.options[i]['~run']( - { value: dataset.value }, - config - ); + const optionDataset = + i === 0 && firstOptionDataset + ? firstOptionDataset + : this.options[i]['~run']({ value: dataset.value }, config);Also applies to: 133-137
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@library/src/schemas/union/union.ts` around lines 122 - 128, The current logic re-invokes options[0]['~run'] when falling back after warming the cache, causing branch-0 transforms/callbacks to run twice; modify the fallback path in the Union implementation to reuse the previously computed fastDataset (the variable produced when _lastSuccessIndex.get(this) === 0) instead of calling this.options[0]['~run'] again — i.e., keep and reuse fastDataset (typed/issue-checked) for the fallback return/handling (also apply the same reuse pattern to the symmetric case around the other fallback at lines 133-137) so option 0 is not executed twice and you still return the cached SuccessDataset<unknown> when appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@library/src/schemas/union/union.ts`:
- Around line 122-124: The fast-path currently dereferences this.options[0] when
_lastSuccessIndex.get(this) === 0 which breaks if the caller mutated options to
an empty array; restore an empty-array guard before calling
this.options[0]['~run'] (e.g., check this.options && this.options.length > 0 or
similar) and fall back to the normal scan path when options is empty so union()
does not attempt to call '~run' on undefined; ensure this change touches the
block that computes fastDataset and retains existing behavior when options is
non-empty.
- Around line 122-128: The current logic re-invokes options[0]['~run'] when
falling back after warming the cache, causing branch-0 transforms/callbacks to
run twice; modify the fallback path in the Union implementation to reuse the
previously computed fastDataset (the variable produced when
_lastSuccessIndex.get(this) === 0) instead of calling this.options[0]['~run']
again — i.e., keep and reuse fastDataset (typed/issue-checked) for the fallback
return/handling (also apply the same reuse pattern to the symmetric case around
the other fallback at lines 133-137) so option 0 is not executed twice and you
still return the cached SuccessDataset<unknown> when appropriate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 374dee6f-4a55-48d1-aed9-5c5edd6769df
📒 Files selected for processing (1)
library/src/schemas/union/union.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
In typical usage, repeated calls to
unionwith valid input match the same branch every time (e.g. a discriminated union where the input type is stable). This caches the last-successful branch index in a module-levelWeakMap<object, number>keyed on the schema instance and tries it first on the next call.Benchmark
~3.0% improvement on
safeParsevalid (allErrors) path.Related to #1408
Summary by CodeRabbit