fix: don't re-walk an already-dereferenced $ref target - #85
Open
BrianWillows wants to merge 1 commit into
Open
BrianWillows wants to merge 1 commit into
BrianWillows wants to merge 1 commit into
Conversation
When a $ref is replaced, this.update(newValue) lets traverse descend into
the substituted value. The same resolved object is substituted at every
site that points at that ref, so its subtree is walked again on each
substitution - and when refs nest, every level is re-walked once per path
that reaches it. A schema where each definition references the one below
it twice is a DAG of n definitions but is traversed as 2^n nodes.
deref() is synchronous, so this blocks the event loop:
definitions input before after
10 1.1KB 13.5ms 4.1ms
18 1.9KB 579.6ms 0.8ms
22 2.3KB 9,556ms 1.5ms
24 2.6KB 38,250ms 2.8ms
Track which resolved values have already been walked and skip descending
into them again. Resolved refs are already shared (the same object is
substituted everywhere), so this changes nothing observable: verified
byte-for-byte identical output across 400 generated schemas covering
nested, shared, repeated and missing refs, and the existing 41 tests pass.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author
|
Nudge on this one - checked against master today, still merges cleanly. The change is confined to one file and adds a visited-set so an already-resolved If you'd prefer the set threaded through explicitly rather than held in the closure, say the word and I'll rework it. |
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
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
When a
$refis replaced,this.update(newValue)letstraversedescend into thesubstituted value:
The same resolved object is substituted at every site pointing at that ref, so
its subtree is walked again on each substitution. When refs nest, every level gets
re-walked once per path that reaches it — a schema where each definition references
the one below it twice is a DAG of n definitions but is traversed as 2ⁿ nodes.
deref()is synchronous, so this blocks the event loop:A ~2.6 KB schema was enough to occupy a core for 38 seconds. The module's existing
cache only covers
refType === 'file'loads, so internal#/definitions/...refswere re-resolved every time.
Fix
Track which resolved values have already been walked, and skip descending into one
that has been seen before (
this.update(newValue, true)).This is safe because resolved refs are already shared — substituting
#/definitions/dat two sites yields the same object today(
result.properties.a === result.properties.bistrueon the current release), soonce it has been dereferenced there is nothing left to do on the next substitution.
Verification
generated schemas covering nested refs, refs shared between several properties,
repeated refs, and missing refs (
failOnMissingpaths).previously took 38 s.
Notes
Circular refs are unaffected — that path (
checkLocalCircular/ thehistorycheck) is untouched and its tests still pass.
Found and fixed with AI assistance (Claude). Happy to add a regression test that
asserts a deep shared-ref schema resolves in bounded time.