Repository navigation
perf: reuse flattened anyOf and oneOf alternatives - #300
Kaileshwar16 wants to merge 4 commits into
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
jdesrosiers
left a comment
There was a problem hiding this comment.
LGTM, about 20–30% faster than main on oneOf-heavy schemas, with identical output.
I added three small commits:
- Fixed oneOf treating alternatives with no output as matches (an older bug on a line this PR rewrote)
- Gave flattened output its own type so unflattened output can't be passed by mistake
- Reused first-pass errors in the oneOf fallback
Could you try a follow-up that caches flattenOutput instead? Store each flattened result in a WeakMap keyed on the NormalizedOutput object. That would cover every caller that flattens the same output more than once, not just anyOf/oneOf. For example, contains.js calls getValidity three times per item, and the evaluator calls it on every simple-applicator suboutput. It would also let us remove the *FromFlattenedOutput variants and the FlattenedOutput type. A few things to watch:
- flatten runs in two modes (isApplied for errors and validity, isRequired for getSuccesses), so each mode needs its own cache.
- The cache is only safe if nothing modifies a normalized output after it's first flattened.
- Please benchmark it against this PR with a schema that exercises contains and nested allOf/$ref, since a cache lookup on every call isn't free.
|
@jdesrosiers thanks for the review, Would you prefer I open a separate PR for that or push it to this one once I have the numbers |
|
I think it's really more of an alternative approach than a follow up. So, if the alternate approach ends up being better, it probably makes sense for it to be a separate PR and we choose which one we like better and close the other. |
|
@jdesrosiers Thanks for the WeakMap suggestion. I tried it on a separate local branch and compared it with your latest version of this PR. Given that, I think current PR is the better option. I’ve kept the WeakMap version local rather than opening a competing PR. attached the benchmark for reference
|
Refs #295.
Ihave reused the flattened alternative results across filtering, validity checks, and error generation
The change is kept small:
Performance and validation:
oneOfcase improved by roughly 5 to 8%.Tradeoffs:
The change adds a little helper indirection and potentially more temporary memory during multiple-match handling. Smaller cases showed no consistent performance improvement or regression, while the larger nested case improved by roughly 5–8%. so this isn't a general speedup. I haven't measured the memory impact yet.
your thoughts on whether the gain is worth those tradeoffs