perf(json): index large replacer key sets - #4157
Open
bobzhang wants to merge 1 commit into
Open
Conversation
`Replacer::keep` and `Replacer::exclude` closed over the key array and ran a
linear `contains` for every object property, so stringifying an object with
many properties against a large key list cost O(properties * keys).
Make the replacer carry its key set as data rather than as a closure, so it can
be indexed. Key sets below a small threshold keep scanning linearly, which is
cheaper than hashing. Larger ones scan linearly too until the replacer has
actually been consulted enough times to amortize an index, at which point a
`Map[StringView, Unit]` is built once and reused for the life of the replacer.
That bounds the wasted work to threshold * keys and leaves one-shot uses
allocation-free, so no usage pattern regresses.
Native, `Json::stringify` with 2,500 keep keys:
| object fields | before | after |
| ---: | ---: | ---: |
| 5 | 7.42 us | 402.54 ns |
| 50 | 92.91 us | 3.19 us |
| 5,000 | 24.31 ms | 391.73 us |
Building a fresh replacer per call stays at 7.5 us, and a 3-key replacer over
5,000 fields goes from 91.4 us to 95.1 us -- the enum dispatch costs ~4% on
that path, in exchange for the numbers above.
The generated interface is unchanged. `Replacer`'s `@debug.Debug` output
becomes `{ kind: Custom(<function: ...>) }` instead of `{ f: <function: ...> }`,
reflecting the new private representation.
Supersedes #4146, whose diagnosis and benchmark this builds on.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MWKp8X7A5VmoV1PxkT8zaS
Contributor
There was a problem hiding this comment.
Pull request overview
This PR optimizes @json.Replacer::keep / exclude by enabling large key sets to be indexed and cached on the Replacer instance, avoiding O(properties × keys) behavior when stringifying large objects against large key lists. It also documents the nested-object filtering behavior and adjusts debug output expectations for the updated internal representation.
Changes:
- Refactors
Replacerto carry a data representation (ReplacerKind) and adds an adaptive key-lookup path that lazily builds and caches aMap[StringView, Unit]after sufficient repeated lookups. - Adds JSON tests to pin behavior across the indexing threshold, verify reuse stability (including
transformparity), and document nested filtering semantics. - Adds benchmark tests covering large-key scenarios, small-key scenarios, and “fresh replacer per call” cases.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| json/json.mbt | Refactors Replacer internals and introduces adaptive indexed lookup to improve stringify/transform performance for large key sets; updates docs to clarify nested filtering and mutation observability. |
| json/json_test.mbt | Adds correctness and behavioral tests for threshold agreement, reuse after indexing, and nested filtering semantics. |
| json/types_test.mbt | Updates the @debug.Debug snapshot expectation for the new Replacer representation. |
| json/replacer_bench_test.mbt | Adds benchmark coverage for large keep-key sets, small-key sets, and fresh-per-call replacer behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Collaborator
Coverage Report for CI Build 6352Coverage increased (+0.009%) to 90.934%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
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.
Supersedes #4146 — the diagnosis and the original benchmark are @mizchi's; this changes where the index lives so no usage pattern regresses.
Problem
Replacer::keepandReplacer::excludeclosed over the key array and ran a linearcontainsfor every object property, so stringifying an object with many properties against a large key list cost O(properties × keys).Approach
The replacer now carries its key set as data rather than as a closure, which is what makes indexing possible at all. On top of that:
Map[StringView, Unit]once the replacer has actually been consulted enough times to amortize it. The index then lives on theReplacerand is reused for its lifetime.Deferring the build bounds the wasted work to
threshold × keysand keeps one-shot uses allocation-free; caching it on the replacer means a reused replacer pays for the index once rather than once per call.Map[StringView, Unit]avoids copying every key withto_owned().Results
Native,
Json::stringifywith 2,500 keep keys:And the two cases that keep the design honest:
That last row is a real regression and it reproduces: matching on the replacer enum costs a little more than the old direct closure call on the small-key hot path. I tried reordering the variants to recover it and it made no difference, so it looks like the indirection itself. It seems a fair trade for the rows above, but flagging it rather than burying it.
Behaviour
Two things changed that are worth calling out:
The key array is no longer re-read on every call. Previously the replacer closed over the
ArrayView, so mutating it betweenstringifycalls was observed. Once a large key set has been indexed that no longer holds. This was an accident of closing over the view rather than an intended contract, sokeep/excludenow document the key list as being read while the replacer is in use, with mutation afterwards not guaranteed to be observed.Replacer's@debug.Debugoutput is now{ kind: Custom(<function: ...>) }instead of{ f: <function: ...> }, reflecting the new private representation.deriveis kept, sopkg.generated.mbtiis byte-identical to main — no public API change.Also here
While documenting the above I noticed
keep/excludefilter at every nesting level —keep(["a"])turns{"a": {"b": 1}, "c": 2}into{"a": {}}. That matches JavaScript'sJSON.stringify(value, keys)and is not a change, but every doc comment and README example showed a flat object, so it was easy to be surprised by. Now documented and pinned by a test.Tests: the new ones check that the indexed path agrees with an equivalent custom replacer for both
keepandexcludeacross the threshold, that a replacer stays correct when reused after its index is built, and thattransformagrees withstringify.Validation: 218 tests pass on wasm, wasm-gc, JS and native;
moon check --target all,moon infoandmoon fmtare clean;pkg.generated.mbtimatches main exactly.🤖 Generated with Claude Code
https://claude.ai/code/session_01MWKp8X7A5VmoV1PxkT8zaS