Repository navigation
Reuse the chunk size header of full chunks in checksum trailers (6,441 → 39 allocations per 100 MiB) - #3490
Open
czj wants to merge 11 commits into
Open
Reuse the chunk size header of full chunks in checksum trailers (6,441 → 39 allocations per 100 MiB)#3490czj wants to merge 11 commits into
czj wants to merge 11 commits into
Conversation
Reuse the input buffer, append chunk and trailer framing directly, and avoid temporary substrings when draining the encoded buffer. Cover binary payloads, partial reads, EOF, and rewind behavior.
A reader that ignores the given buffer and returns a frozen String made it the next read's buffer, raising FrozenError. Read into a local instead, consume buffer_enabled like the other options, and skip the trailer compatibility examples on JRuby, where trailer checksums are disabled. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every chunk but the last one has the same size, so its hex size is built once instead of allocating a String per chunk: streaming 100 MiB with 16 KiB reads allocates 39 objects instead of 6,441. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The spec now reads 4 bytes first, then full chunks, so both the cached header and the formatted one are exercised. `size` derives the full chunk overhead from the same cached header the body writes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Nice! I’ll take a look. This will ship separately from the earlier two PRs. I want to release those first and make sure everything goes smoothly before shipping this one. |
Contributor
Author
|
That's a wise approach. Thanks for your work on this :) |
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.
Following up on @jterapin's suggestion in #3448:
AwsChunkedTrailerDigestIOstill allocates one String per chunk for its hex size. Every chunk but the last has the same size, so that header is now built once ininitialize, and only a shorter chunk still formats its own.The gain is allocations only, not time: since #3448 no GC runs on this path anyway, so this mostly matters for apps that watch allocations per request or job.
Streaming 100 MiB through it with 16 KiB reads into a reused buffer, as Net::HTTP does (Ruby 4.0.9, arm64 macOS, median of 9 alternating runs):
read(length, buffer)read(length, buffer)read(length)onlyread(length)onlyA 3,000-case differential fuzz against #3465 (random payloads, chunk sizes, read lengths, sources returning short reads, rewinds, both read paths) gives byte-identical output, and the new spec covers a source returning short reads, where chunks before the last one are not full either.
This branch sits on top of #3465 (itself on #3448), since #3465 changes the same line, so only the last two commits are new here: compare with #3465. I'll rebase it as they land. Does stacking it this way work for you, or would you rather have it based on #3448 alone?