Reduce allocations and fix spread overflow in hot paths#1437
Reduce allocations and fix spread overflow in hot paths#1437heiwen wants to merge 1 commit intoopen-circle:mainfrom
Conversation
- Replace `push(...issues)` spread with `for...of` in `_subIssues` and `intersect` — spread unpacks into the arguments object and can overflow the call stack for large issue arrays - Add `DEFAULT_CONFIG` singleton in `getGlobalConfig` so calls with no config skip object construction entirely - Add `ABORT_EARLY_CONFIG` module-level constant in `is` and `assert` so the config object is shared across calls rather than allocated each time - Cache `~standard` result per schema instance in a `WeakMap` inside `_getStandardProps` to avoid recomputing on repeated access Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PR introduces several performance optimizations: shared module-level constants for configuration reuse in assertion methods, explicit iteration patterns replacing spread operations for issue aggregation, fast-path optimization for default configuration retrieval, and WeakMap caching to reuse StandardProps objects per context identity. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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:M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/methods/assert/assert.ts`:
- Around line 4-5: ABORT_EARLY_CONFIG is a shared object that gets mutated by
_addIssue (which writes to config.issues), causing validation state to leak
between assert() calls; change to return a fresh config object per call instead
of reusing the singleton — e.g., replace the const ABORT_EARLY_CONFIG with a
factory function (getAbortEarlyConfig) or clone it inside assert() so every
invocation supplies a new { abortEarly: true, issues: [] } (or equivalent) to
_addIssue; update any places that reference ABORT_EARLY_CONFIG to call the
factory so no shared mutable state is used.
In `@library/src/methods/is/is.ts`:
- Around line 3-4: The shared constant ABORT_EARLY_CONFIG is being passed
directly into is() calls and can be mutated by downstream validation (e.g.,
config.issues), causing cross-call state leakage; change is() (and any callers
using ABORT_EARLY_CONFIG) to create a fresh config object per call (e.g., const
cfg = { ...ABORT_EARLY_CONFIG, ...(userConfig ?? {}) } or Object.assign({},
ABORT_EARLY_CONFIG, userConfig) ) and pass that clone into the validation logic
so downstream mutations don't affect the singleton.
In `@library/src/storages/globalConfig/globalConfig.ts`:
- Around line 11-17: DEFAULT_CONFIG is a shared mutable object returned by
getGlobalConfig() and can be corrupted if callers mutate it; fix by freezing the
exported/shared constant so mutations throw (or fail silently) — i.e., ensure
DEFAULT_CONFIG is wrapped with Object.freeze (or created frozen) so any attempt
to assign properties like abortEarly will fail, and keep the identifier
DEFAULT_CONFIG (and its usage in getGlobalConfig/getGlobalConfigValue or similar
functions) unchanged so existing callers still receive the frozen instance.
In `@library/src/utils/_getStandardProps/_getStandardProps.ts`:
- Around line 30-41: The cached StandardProps object returned by
_getStandardProps is being shared and can be mutated by consumers (e.g., the
validate function), so create the props object as before but call
Object.freeze(...) on it (or otherwise deep-freeze if needed for nested mutation
concerns) before calling _standardCache.set(context, cached) to ensure the same
instance cannot be mutated across calls; reference the cached variable, the
validate method on that object, and the _standardCache.set call when making this
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05009d3a-9930-4e26-82b8-c39e219208f3
📒 Files selected for processing (6)
library/src/methods/assert/assert.tslibrary/src/methods/is/is.tslibrary/src/schemas/intersect/intersect.tslibrary/src/schemas/union/utils/_subIssues/_subIssues.tslibrary/src/storages/globalConfig/globalConfig.tslibrary/src/utils/_getStandardProps/_getStandardProps.ts
| // Shared config — allocating `{ abortEarly: true }` on every call would be wasteful | ||
| const ABORT_EARLY_CONFIG = { abortEarly: true } as const; |
There was a problem hiding this comment.
Shared ABORT_EARLY_CONFIG introduces cross-call mutable state.
Line 17 now reuses a singleton config, but _addIssue mutates config.issues (see library/src/utils/_addIssue/_addIssue.ts), so validation state can leak between independent assert() calls.
💡 Proposed fix
-// Shared config — allocating `{ abortEarly: true }` on every call would be wasteful
-const ABORT_EARLY_CONFIG = { abortEarly: true } as const;
@@
- const issues = schema['~run']({ value: input }, ABORT_EARLY_CONFIG).issues;
+ const issues = schema['~run']({ value: input }, { abortEarly: true }).issues;Also applies to: 17-17
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@library/src/methods/assert/assert.ts` around lines 4 - 5, ABORT_EARLY_CONFIG
is a shared object that gets mutated by _addIssue (which writes to
config.issues), causing validation state to leak between assert() calls; change
to return a fresh config object per call instead of reusing the singleton —
e.g., replace the const ABORT_EARLY_CONFIG with a factory function
(getAbortEarlyConfig) or clone it inside assert() so every invocation supplies a
new { abortEarly: true, issues: [] } (or equivalent) to _addIssue; update any
places that reference ABORT_EARLY_CONFIG to call the factory so no shared
mutable state is used.
There was a problem hiding this comment.
Looking at the implementation, _addIssue writes to dataset.issues rather than config.issues. The Config type doesn't have an issues field and config appears to be treated as read-only throughout.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check _addIssue implementation
cat library/src/utils/_addIssue/_addIssue.ts
# Check Config type definition
rg -n "Config" library/src/types/config.ts 2>/dev/null || fd "config.ts" library/src/types --exec cat {}
# Check if config has an issues field anywhere
rg -n "config\.issues" library/src/
Length of output: 3878
@heiwen, you're absolutely right — I apologize for the incorrect review comment. Looking at the _addIssue implementation:
- The
Configtype has noissuesfield at all; it only carrieslang,abortEarly,abortPipeEarly, andmessage. _addIssueonly reads fromconfig(e.g.,config.lang,config.abortEarly) and writes exclusively todataset.issues.
So ABORT_EARLY_CONFIG is a purely read-only object from the perspective of every caller, and sharing it as a module-level singleton is perfectly safe. The optimization is correct as written.
✏️ Learnings added
Learnt from: heiwen
Repo: open-circle/valibot PR: 1437
File: library/src/methods/assert/assert.ts:4-5
Timestamp: 2026-03-23T00:25:07.082Z
Learning: In the valibot library, the `Config` type (`library/src/types/config.ts`) has no `issues` field. `_addIssue` (`library/src/utils/_addIssue/_addIssue.ts`) reads from `config` (lang, abortEarly, abortPipeEarly, message) but writes only to `dataset.issues`. The `config` parameter is treated as strictly read-only throughout the codebase, so sharing a singleton config object (e.g., `ABORT_EARLY_CONFIG = { abortEarly: true } as const`) across calls is safe and does not cause state leakage.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| // Shared config — allocating `{ abortEarly: true }` on every call would be wasteful | ||
| const ABORT_EARLY_CONFIG = { abortEarly: true } as const; |
There was a problem hiding this comment.
is() also leaks mutable config across calls via shared singleton.
Like assert(), this reuses a config object that can be mutated by downstream validation (config.issues), creating cross-call contamination.
💡 Proposed fix
-// Shared config — allocating `{ abortEarly: true }` on every call would be wasteful
-const ABORT_EARLY_CONFIG = { abortEarly: true } as const;
@@
- return !schema['~run']({ value: input }, ABORT_EARLY_CONFIG).issues;
+ return !schema['~run']({ value: input }, { abortEarly: true }).issues;Also applies to: 19-19
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@library/src/methods/is/is.ts` around lines 3 - 4, The shared constant
ABORT_EARLY_CONFIG is being passed directly into is() calls and can be mutated
by downstream validation (e.g., config.issues), causing cross-call state
leakage; change is() (and any callers using ABORT_EARLY_CONFIG) to create a
fresh config object per call (e.g., const cfg = { ...ABORT_EARLY_CONFIG,
...(userConfig ?? {}) } or Object.assign({}, ABORT_EARLY_CONFIG, userConfig) )
and pass that clone into the validation logic so downstream mutations don't
affect the singleton.
There was a problem hiding this comment.
Looking at the implementation, _addIssue writes to dataset.issues rather than config.issues. The Config type doesn't have an issues field and config appears to be treated as read-only throughout.
There was a problem hiding this comment.
@heiwen, you're absolutely right — I apologize for the incorrect review. The Config type in valibot does not include an issues field; issue accumulation is handled by _addIssue, which writes exclusively to dataset.issues. Since config is treated as read-only throughout the codebase, the shared ABORT_EARLY_CONFIG singleton is perfectly safe and carries no risk of cross-call state leakage. The optimization is sound as-is.
🧠 Learnings used
Learnt from: heiwen
Repo: open-circle/valibot PR: 1437
File: library/src/methods/assert/assert.ts:4-5
Timestamp: 2026-03-23T00:25:07.082Z
Learning: In this codebase (valibot), treat the valibot `Config` object as strictly read-only: `Config` does not include an `issues` field, and issue accumulation is performed by `_addIssue` writing to the mutable `dataset.issues`, not into `config`. Review changes to ensure `config` is never mutated or used to store per-call state. With that guarantee, it’s safe to share singleton/constant config objects across calls (e.g., `ABORT_EARLY_CONFIG = { abortEarly: true } as const`) without state leakage.
1 participant
Summary
Small targeted improvements to reduce overhead in frequently called code paths:
getGlobalConfig: return a sharedDEFAULT_CONFIGsingleton when no config is set (avoids branch + object access)is/assert: use a module-levelABORT_EARLY_CONFIGconstant instead of an inline{ abortEarly: true }literal_subIssues/intersect: replacepush(...issues)spread with afor...ofloop (avoids stack overflow on large issue arrays and removes the spread overhead)_getStandardProps: cache the computed Standard Schema properties in aWeakMapso repeated property accesses return the cached valueBenchmark
No significant change on the benchmark suite (changes are in paths not exercised by the single-schema warm benchmark), but each change reduces unnecessary work per call.
Related to #1408
Summary by CodeRabbit