Decode a lone response body part in place - #2303
Conversation
Response#getResponseBody went through getResponseBodyAsBytes, which concatenates every body part into a freshly allocated array. A body that arrived in a single read was therefore copied twice: once to concatenate a single part with nothing, and once to decode. That is the common case for responses small enough to land in one socket read, and it is the path the default AsyncCompletionHandlerBase puts every caller of executeRequest(request) on. Decode straight from the part when there is exactly one. A rough probe on JDK 17 over a single-part ASCII body measured 496 -> 47 ns at 512 B, 2277 -> 373 ns at 4 KB, 2963 -> 1450 ns at 16 KB and 24913 -> 12248 ns at 128 KB, alongside one fewer whole-body allocation. Several parts are still concatenated before decoding, never decoded one at a time, because a multi-byte character can straddle a part boundary. The new test pins that by splitting a two-byte UTF-8 character across two parts and asserting both shapes decode alike. getResponseBodyAsBytes and getResponseBodyAsByteBuffer are deliberately left alone: they hand the array to the caller, so they keep copying rather than expose a part's own array. A second test pins that too. Claude Code on behalf of Pavel Ptashyts Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| // Decode a lone body part straight from its own bytes. getResponseBodyAsBytes concatenates every part | ||
| // into a fresh array first, so a body that arrived in a single read was copied twice, once to | ||
| // concatenate and once to decode. The array does not escape this method, so decoding the part's own | ||
| // one is safe. Several parts are still concatenated before decoding rather than decoded one at a | ||
| // time, because a multi-byte character can straddle a part boundary. |
There was a problem hiding this comment.
Five lines of comment in a file that has none. The middle sentence is about what the old code did, which is commit message material. Two lines carry everything a reader needs:
// A lone part's array does not escape this method, so it can be decoded in place. Several parts are
// concatenated first because a multi-byte character can straddle a part boundary.
There was a problem hiding this comment.
Cut to two lines. The sentences about what the old code did went into the commit message, and what remains sits on the private helper, since that is where the branch and the constraint on it now live.
| // concatenate and once to decode. The array does not escape this method, so decoding the part's own | ||
| // one is safe. Several parts are still concatenated before decoding rather than decoded one at a | ||
| // time, because a multi-byte character can straddle a part boundary. | ||
| if (bodyParts.size() == 1) { |
There was a problem hiding this comment.
getResponseBodyAsStream a few lines below hands the same array to a ByteArrayInputStream, which never exposes what it wraps, so it can take this shortcut too. Fine to leave for a follow up. If you do both, put the guard in a small private helper instead of repeating it.
There was a problem hiding this comment.
Did both. The guard is in a private sharedBodyBytes(), named to carry the constraint to the call sites: the array it returns may be a part's own, so only callers that keep it to themselves may use it. getResponseBodyAsBytes and getResponseBodyAsByteBuffer still copy.
| int split = 4; | ||
|
|
||
| List<HttpResponseBodyPart> onePart = new LinkedList<>(); | ||
| onePart.add(new EagerResponseBodyPart(Unpooled.wrappedBuffer(utf8), true)); |
There was a problem hiding this comment.
Both new tests use Eager parts. Lazy is the case where this is least obvious, since its getBodyPartBytes returns only the readable region rather than the whole backing array. Worth one more case with a Lazy part over a slice, say Unpooled.wrappedBuffer(backing, 3, 11), so that a later change to getBodyByteBuf().array() cannot slip through. testGetResponseBodyAsByteBuffer above already uses Lazy parts, so it fits the file.
There was a problem hiding this comment.
Added, with your wrappedBuffer(backing, 3, 11). Good catch. A version reaching for getBodyByteBuf().array() would read the surrounding bytes too, and nothing else in the suite would have noticed. The case asserts the stream path as well, now that it shares the shortcut.
|
|
||
| // getResponseBody may decode a lone part in place, but getResponseBodyAsBytes hands the array to the | ||
| // caller, so it must keep copying rather than expose the part's own array. | ||
| assertNotSame(response.getResponseBodyAsBytes(), response.getResponseBodyAsBytes()); |
There was a problem hiding this comment.
Good one to have. This is what catches the tempting wrong version of this change, pushing the fast path down into getResponseBodyAsBytes, because Eager hands back the same array every time.
There was a problem hiding this comment.
Kept for exactly that reason. Eager handing back the same array every time makes the wrong version of this look free.
Review feedback on AsyncHttpClient#2303. getResponseBodyAsStream wraps the bytes in a ByteArrayInputStream, which never exposes the array it holds, so it can read a lone part's own array for the same reason getResponseBody can. Both now go through one private sharedBodyBytes(), which keeps the guard and the constraint on it in a single place. Cut the comment down. Two of its five lines described what the previous version did, which belongs in a commit message, and the file carries no other comments. Add a case for a Lazy part over a slice. Lazy is where the shortcut is least obvious, because its getBodyPartBytes returns only the readable region rather than the whole backing array, so a later change reaching for getBodyByteBuf().array() would read the surrounding bytes instead. The case covers both the decode and the stream path. Claude Code on behalf of Pavel Ptashyts Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 participants
Problem
For the default handler (
executeRequest(request)->AsyncCompletionHandlerBase->
Response) the body is copied three times:HttpHandler.handleChunk->EagerResponseBodyPartcopies each chunk out ofthe network buffer into a heap
byte[](needed:channelReadreleases themessage afterwards);
NettyResponse.getResponseBodyAsByteBufferconcatenates every part into afreshly allocated array;
getResponseBody(charset)decodes that array.Step 2 is pure waste when there is only one part, which is the case for any body
that lands in a single socket read: it copies a single part into a new array with
nothing to concatenate it with.
Change
getResponseBody(Charset)decodes straight from the part when there is exactlyone of them. The array does not escape the method, so the part's own array can be
decoded in place.
Several parts are still concatenated before decoding, never decoded one at a
time, because a multi-byte character can straddle a part boundary.
getResponseBodyAsBytesandgetResponseBodyAsByteBufferare deliberately leftuntouched: they hand the array to the caller, so they keep making a defensive
copy rather than expose a part's own array. There is no aliasing change anywhere
in this PR.
Measurements
Rough probe on JDK 17 over a single-part ASCII body, concatenate-then-decode
versus decode-in-place. Not JMH, so read the shape rather than the digits:
Plus one fewer whole-body allocation per response. The percentages look large
partly because a pure-ASCII body decodes through a JDK intrinsic, which makes the
removed copy a big share of what is left.
Tests
Two added to
NettyAsyncResponseTest:testGetResponseBodyDecodesOnePartAndSplitPartsIdenticallysplits the two-byteUTF-8 encoding of U+00E9 across two parts and asserts one-part and split-part
bodies decode alike. This pins the constraint the comment states: it fails if
anyone later makes the multi-part path decode part by part.
testGetResponseBodyAsBytesDoesNotShareTheBodyPartArraypins thatgetResponseBodyAsBytesstill returns a fresh array and never the part's own.The body bytes are built as an explicit
byte[]rather than a string literal tokeep the source ASCII per
AGENTS.md.Verification
mvnw clean verify- BUILD SUCCESS, 1373 tests (1371 before, plus these two),0 failures, 0 errors, 19 skipped. Error Prone, NullAway and Revapi clean.
LargeResponseTest,NoNullResponseTest,BodyDeferringAsyncHandlerTestandRedirectBodyTest, which exercise the multi-part path, are green.Caveat on the testing gate:
AGENTS.mdrequires the build to run on JDK 11 andno JDK 11 is installed on this machine, so it was run on JDK 17 (also in the
CI matrix). The JDK 11 leg of CI on this PR is the real gate.
Not in scope
The multi-part case still concatenates. A
CompositeByteBuf.toString(charset)variant measured faster there (Netty decodes a multi-component buffer through a
recycled, un-zeroed thread-local array instead of a fresh
byte[]), but itregressed at high part counts in the same probe, so it needs proper benchmarking
before it becomes a change. Removing copy 1 would mean retaining network buffers
and giving
Responsea lifecycle, which is public API and wants a designdiscussion first.
No public API change here. Same review pass as #2300, #2301 and #2302.
Claude Code on behalf of @pavel-ptashyts
🤖 Generated with Claude Code