perf: Benchmarks and optimizations#1527
Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. WalkthroughThe PR adds runtime performance changes in Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@BENCHMARKING.md`:
- Around line 14-16: Update the BENCHMARKING.md description to remove the “runs
every file once” wording, since the benchmark runner executes each case across
warmup and measurement iterations before reporting hz and latency percentiles.
Rewrite the sentence around the benchmark behavior so it accurately describes
repeated iterations, and keep the surrounding guidance in the benchmarking
section consistent with the semantics shown by the bench runner.
In `@library/src/schemas/picklist/picklist.ts`:
- Around line 104-111: The cached _optionsSet in PicklistSchema is holding a
stale snapshot of mutable options, so later mutations to this.options are not
reflected during validation. Update the logic around the optionsSet creation/use
in PicklistSchema (and its parse path) so membership checks always reflect the
current options array, either by rebuilding the Set from this.options each time
or by invalidating the cache whenever options changes. Keep the SameValueZero
behavior consistent with the existing includes-based check.
In `@library/src/schemas/variant/variant.ts`:
- Around line 198-201: The fast dispatch in the variant discriminator lookup is
collapsing an absent key and a present undefined value because it reads
input[this.key] directly. Update the discriminator handling in variant.ts so the
fast path first checks whether the key exists on input the same way the slow
path does (using currentKey in input or equivalent) before consulting
discriminatorMap in the relevant branch, and only use the fast map lookup when
the discriminator is actually present.
- Line 70: The NaN guard in variant validation uses self-comparison, which
triggers Biome. Update the check in the variant schema logic (the `variant.ts`
code path that validates `value`) to use `Number.isNaN(value)` instead of `value
!== value`, preserving the same behavior while keeping lint clean.
In `@library/src/schemas/variant/variantAsync.ts`:
- Line 71: The NaN check in the variant async schema currently uses
self-comparison in the code path around the value guard, which Biome flags as a
lint issue. Update the check in the relevant validation logic within the
variantAsync handling to use Number.isNaN instead, keeping the same behavior
while satisfying the linter.
- Around line 199-203: The async discriminator fast path in variantAsync should
match the sync/slow-path behavior by checking that the discriminator key
actually exists on input before using input[this.key]. Update the
discriminatorMap lookup branch in the variantAsync execution logic so it only
runs when the key is present (for example via a currentKey in input-style
guard), and keep the existing discriminatorMap/option flow otherwise.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dad508fc-247c-4a71-a241-5a51a71119f0
📒 Files selected for processing (18)
BENCHMARKING.mdCONTRIBUTING.mdlibrary/bench/array.bench.tslibrary/bench/enum.bench.tslibrary/bench/object.bench.tslibrary/bench/picklist.bench.tslibrary/bench/string-pipe.bench.tslibrary/bench/variant-async.bench.tslibrary/bench/variant.bench.tslibrary/package.jsonlibrary/src/schemas/enum/enum.tslibrary/src/schemas/looseObject/looseObject.tslibrary/src/schemas/object/object.tslibrary/src/schemas/objectWithRest/objectWithRest.tslibrary/src/schemas/picklist/picklist.tslibrary/src/schemas/strictObject/strictObject.tslibrary/src/schemas/variant/variant.tslibrary/src/schemas/variant/variantAsync.ts
There was a problem hiding this comment.
6 issues found across 18 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
@fabian-hiller any chance you could take a look at this? |
|
Thank you for this PR. I will try to take a closer look and get this merged before v1.5. |
commit: |
This change adds a runtime benchmark suite and three hot-path optimizations, each proven by before/after measurements. All 4486 tests pass with no behavior or type changes.
Why these paths
Profiling the schema-execution hot paths and comparing against zod showed three places where valibot did linear work that could be made constant-time, without touching its functional, tree-shakeable design or resorting to
eval/JIT.1.
variant/variantAsyncdiscriminator-map dispatchBefore, validating a discriminated union scanned the options one by one, running each option's discriminator sub-schema against the input until one matched. Cost grew with the number of options: the last option of ten was ~13x slower than the first.
Now a
Map<discriminatorValue, option>is built lazily on the first run, so dispatch is a singleMap.getregardless of option count. The map is disabled (falling back to the original scan) for nested variants, non-enumerable discriminators, colliding values, orNaN, so error reporting and every edge case stay identical. Result: dispatch is position-independent (up to ~13x on the worstcase, ~8x for the async schema), and even the first-option case is faster because the fast path skips the per-call closure and
Setallocations of the scan.2.
enum/picklistset membershipBefore, membership used
Array.includes, an O(n) scan, so checking a value near the end of a large allow-list was ~3x slower than one near the start. Now a lazily-cachedSetmakeshasO(1) and position-independent.Set.hasuses the same SameValueZero comparison asArray.includes, so behavior (includingNaN) is unchanged.3.
objectfamily loopThe entry loop evaluated
key in inputtwice per present key (once in the condition, once in the value ternary). Hoisting it to a single check gives a small, consistent win on the object happy path with no behavior change. Applied toobject,strictObject,looseObject, andobjectWithRest.Design notes
Benchmarks
variantandvariantAsyncdiscriminator-map fast path (O(1) dispatch instead of scanning every option).objectfamily: hoist the redundantkey in inputre-check in the entry loop.enum/picklistlazily-cachedSetmembership instead ofArray.includes.Baseline vs. optimized (speedup = optimized / baseline)
* The
variantmiss case is unchanged by design: a non-matching discriminatorfalls through to the original slow path so the discriminator issue and message
are byte-identical to before.
Summary by CodeRabbit
benchcommand to run the benchmark suite locally.