response: say what the segment coalescing actually does - #234
Merged
Conversation
`coalesce_small` merges only runs of consecutive sub-threshold segments.
Its name, its doc ("merge segments below `min_segment` into their
neighbour") and its test name all claimed it merges an isolated fragment
into an adjacent one, which it does not: in the alternating
fragment/capture shape a multi-field view produces, nothing merges at
all. The test asserted a length of 5 under the name
`tiny_segments_are_merged_into_their_neighbours`, proving the opposite
of what it was called.
The behaviour is right and stays. A `Bytes` is an immutable view with no
spare capacity, so folding a fragment into an adjacent capture means
allocating and copying that capture — destroying the refcount aliasing
this path exists to create. The residual cost is one 9-byte frame header
per captured field.
So this renames the helper to `coalesce_small_runs`, rewrites the doc to
describe runs and to record why an isolated fragment is left as its own
segment, and renames the test to match what it verifies. It also adds a
test for the merging branch itself: in the alternating shape `pending`
never holds more than one fragment, so the run-accumulation path the
function is named for had no direct coverage.
Signed-off-by: Iain McGinniss <309153+iainmcgin@users.noreply.github.com>
iainmcgin
marked this pull request as ready for review
July 20, 2026 22:10
iainmcgin
enabled auto-merge
July 20, 2026 22:10
azdagron
approved these changes
Jul 20, 2026
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.
Summary
coalesce_small, added in #232, merges only runs of consecutive sub-threshold segments. Its name, its doc — "Merge segments belowmin_segmentinto their neighbour" — and its test's name all claimed it merges an isolated fragment into an adjacent one. It does not.The shape that matters is the one the doc itself describes: a rope flushes its pending tail before each capture, so a view with several large fields yields alternating tag/length fragments and captured payloads. In that shape no fragment has a small neighbour, so nothing merges. The test made this concrete and then mislabelled it —
tiny_segments_are_merged_into_their_neighboursassertedmerged.len() == 5on a five-element alternating input, which is the assertion that nothing merged.The behaviour is correct and unchanged
A
Bytesis an immutable{ptr, len, refcount}view with no spare capacity and no interior mutability. Every zero-copy operation it offers narrows the window; none widens it.BytesMut::unsplitcan rejoin without copying, but only for adjacent slices of one originating buffer, and a freshly built tag/length fragment is not contiguous with a payload captured from the request's backing buffer.So folding a 2-byte fragment onto a neighbouring 32 KiB capture — in either direction — forces a fresh allocation and a full copy of that capture, destroying the refcount aliasing this whole path exists to create. Leaving the fragment as its own segment is the right trade, and the residual cost is one 9-byte HTTP/2 frame header per captured field.
What changes
The helper becomes
coalesce_small_runs. The doc describes runs, and records why an isolated fragment is kept separate — including that the fragment itself is still re-copied into a run of its own, so what survives untouched is the capture, not the fragment. The test is renamed toisolated_fragments_stay_their_own_segments.A second test covers the merging branch. In the alternating shape,
pendingnever accumulates more than one fragment before it is flushed, so the run-accumulation path the function is named for had no direct coverage:consecutive_fragments_merge_into_one_segmentdrives three adjacent fragments into one segment and pins that the capture following them is not copied.No behaviour change, no public API change — the helper is private.
Note on #232's description
#232's merged description lists "tiny rope fragments becoming their own frames" among four defects it says are fixed. That claim was wrong; the frames remain, deliberately, for the reason above. The changelog fragment does not repeat the claim, so the release notes are unaffected.
Testing
Clippy on the pinned 1.95 toolchain,
cargo test -p connectrpc --no-default-features, 56 test suites, lint, and fmt all clean.