Skip to content

fix: don't re-walk an already-dereferenced $ref target - #85

Open
BrianWillows wants to merge 1 commit into
cvent:masterfrom
BrianWillows:fix/exponential-ref-resolution
Open

BrianWillows wants to merge 1 commit into
cvent:masterfrom
BrianWillows:fix/exponential-ref-resolution

Conversation

@BrianWillows

Copy link
Copy Markdown

Summary

When a $ref is replaced, this.update(newValue) lets traverse descend into the
substituted value:

this.update(newValue)

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:

definitions input before after
10 1.1 KB 13.5 ms 4.1 ms
18 1.9 KB 579.6 ms 0.8 ms
22 2.3 KB 9,556 ms 1.5 ms
24 2.6 KB 38,250 ms 2.8 ms

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/... refs
were 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/d at two sites yields the same object today
(result.properties.a === result.properties.b is true on the current release), so
once it has been dereferenced there is nothing left to do on the next substitution.

Verification

  • Existing test suite passes: 41/41.
  • Output is byte-for-byte identical to the current implementation across 400
    generated schemas covering nested refs, refs shared between several properties,
    repeated refs, and missing refs (failOnMissing paths).
  • The pathological case is now flat: 40 levels resolves in ~1 ms, where 24 levels
    previously took 38 s.

Notes

Circular refs are unaffected — that path (checkLocalCircular / the history
check) 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.

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>
@BrianWillows

Copy link
Copy Markdown
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 $ref target isn't re-walked at each substitution site. No signature or option changes.

If you'd prefer the set threaded through explicitly rather than held in the closure, say the word and I'll rework it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants