Reuse the element scratch buffer across rows in json_get_array - #142
Merged
Merged
Conversation
The fast path added in #139 collected each row's element slices in a SmallVec<[&str; 8]>, which allocates once per row for arrays longer than eight elements. The buffer is now a Vec<Range<usize>> created once per batch and cleared per row, so it allocates at most once per batch regardless of array length. Storing byte ranges instead of borrowed slices is what lets one buffer outlive the per-row closure call. The row's elements are sliced from the input &str by range, which is an O(1) char-boundary check, in place of the previous std::str::from_utf8 pass over each element. jiter stops on ASCII structural bytes, so the boundaries are always valid. invoke_array_scalars_direct takes FnMut so the closure can capture the buffer. The smallvec dependency is removed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L2MYrU7T15KLX6ASKAMu7Y
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #142 +/- ##
==========================================
+ Coverage 93.05% 93.09% +0.03%
==========================================
Files 18 18
Lines 1815 1824 +9
Branches 1815 1824 +9
==========================================
+ Hits 1689 1698 +9
Misses 70 70
Partials 56 56 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The existing json_get_array_array bench uses eight elements, which never spilled the SmallVec inline buffer, so it does not exercise the case the scratch-buffer reuse targets. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L2MYrU7T15KLX6ASKAMu7Y
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation preserves behavior and is adequately tested, with only a minor allocation-claim discrepancy.
Review effort: Balanced
Findings: 1
What changed in this PR
Reuses a batch-scoped scratch buffer in json_get_array to reduce parsing allocations.
Changes:
- Stores element byte ranges in a reusable
Vec. - Allows mutable direct-path callbacks.
- Removes
smallvecand adds a wide-array benchmark.
| File | Description |
|---|---|
src/json_get_array.rs |
Implements reusable range buffering. |
src/common.rs |
Accepts FnMut callbacks. |
Cargo.toml |
Removes smallvec. |
benches/main.rs |
Adds a 32-element benchmark. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Merged
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.

Follow-up to #139.
The
json_get_arrayfast path buffered each row's element slices in aSmallVec<[&str; 8]>, which allocates once per row for arrays longer than eight elements.This replaces it with a
Vec<Range<usize>>created once per batch and cleared per row, so its capacity is reused across rows. The buffer grows only when a row has more elements than any earlier row in the batch, so the allocation count per batch depends on the widest row, not on the number of rows.Storing byte ranges rather than borrowed
&stris what lets one buffer outlive each per-row closure call.Elements are now sliced from the input
&strby range, an O(1) char-boundary check, instead of astd::str::from_utf8pass over each element.jiter stops on ASCII structural bytes, so every range boundary is a char boundary.
invoke_array_scalars_directtakesFnMutso the closure can capture the buffer.The
smallvecdependency is removed.Null-on-error behaviour is unchanged: ranges are only sliced after the whole array parses, and the existing
malformed_late_element_does_not_append_partial_valuestest still covers that.The equivalence test runs rows of 5, 9, 2, 0 and 1 elements in sequence through the shared buffer.
Benchmarks
The existing
json_get_array_arraybench uses eight elements, which fit the old inline buffer, so this addsjson_get_array_array_widewith 32 elements.Both are 1,024-row Utf8View batches,
cargo bench --bench mainrelease builds on an Apple M3 Max, criterion--save-baselineon main's source and--baselineon this branch, same bench file for both.Two before/after pairs, reported as criterion's mean estimate:
The eight-element gain comes from dropping the per-element
from_utf8pass; the extra gain at 32 elements is the per-row heap allocation the old buffer spilled into.A first baseline run measured 487 µs and 1.876 ms, well outside the other two, and was discarded as noise.
cargo testandcargo clippy --all-targets -- -D warningspass.🤖 Generated with Claude Code
https://claude.ai/code/session_01L2MYrU7T15KLX6ASKAMu7Y