You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Thanks for this, the buffer reuse is a clean win and the before/after allocation flamegraphs make the impact easy to see. I traced the in-place merge against the old return-value version and the behavior looks preserved, including the non-commutative operand order and the memory accounting via heap_size. Two small suggestions, neither a blocker.
1. Defensive clear() for consistency
compress_immut defensively clears its output buffer before filling it, but with_head_buffer_inserted and merge take sampled_buffer and only reserve before pushing:
This is correct today because sampled_buffer is always empty on entry. It does leave a bit of a trap though. If that invariant ever slips in a future change, this path would silently append the stale elements (it pushes after reserve) and corrupt the result rather than fail loudly. A new_samples.clear() right after the take is essentially free and keeps this consistent with compress_immut. Same thought for merged_sampled in merge.
2. A multi-merge test
The new flush_and_merge_reuse_sampled_buffers test locks in the pointer swap, which is great. merge_is_within_bound only does a single merge though, and merge_batch folds many digests through the reused buffers in sequence. Would you consider adding a test that merges three or more summaries in place and checks the query result against the exact percentile? That would exercise the buffer reuse across repeated merges, which is exactly where a reuse bug would surface.
Review assisted by an LLM.
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
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.
Which issue does this PR close?
Closes #4874.
Rationale for this change
The native QuantileSummaries port behind approx_percentile / percentile_approx allocates more than necessary on its hot paths:
QuantileSummaries::mergetakes&self/&otherand allocates a fresh summary, somerge_batchreallocates on every incoming digest.with_head_buffer_insertedrebuilds the wholesampledvector on every flush.What changes are included in this PR?
How are these changes tested?
benchmark
result:
merge_batch: (15GB -> 11GB)with_head_buffer_inserted: (10GB -> 1GB)