chore: update ChunkSegmenter to optionally allow a limit on the number of bytes it should consume #3279
Conversation
| for (int i = offset; i < length; i++) { | ||
| ByteBuffer buffer = bbs[i]; | ||
| int remaining; | ||
| while ((remaining = buffer.remaining()) > 0) { | ||
| long overallRemaining = totalRemaining - consumedSoFar; | ||
| if (overallRemaining < blockSize && currentBlockPending == blockSize) { | ||
| break; | ||
| break outerloop; |
| } | ||
| if (numBytesConsumable <= 0) { | ||
| continue; | ||
| break outerloop; |
There was a problem hiding this comment.
K...now I'm starting to worry a bit about cyclomatic complexity
There was a problem hiding this comment.
Prior to the appendable upload stuff we generally had a pretty firm bulkhead on the number of buffers that would be passed into these methods outside of tests. With the appendable addition we place less emphasis on early buffering in favor of passing things through wherever possible, so if multiple buffers are passed in here, logically the conditions would prevent consuming any bytes once a break from the while takes place, but by breaking the for as well we avoid the cycles performing work that isn't productive.
And, refactoring everything to nested method calls to allow early returns instead of break to label didn't seem worth it to me.
There was a problem hiding this comment.
What's the follow up here?
There was a problem hiding this comment.
Nothing from my perspective. There isn't a functional difference here between the label break and a separate method with early returns.
…r of bytes it should consume Update BidiAppendableUnbufferedWritableByteChannel to only attempt to consume as many bytes as are available according to the stream -- this prevents over packing of segments we will for sure never be able to use.
0bbbc42 to
e07f2a4
Compare
"Child" change after #3278
Update BidiAppendableUnbufferedWritableByteChannel to only attempt to consume as many bytes as are available according to the stream -- this prevents over packing of segments we will for sure never be able to use.
With both this PR and the contents of #3278 our latency now looks like the following, where the
v2.56.0line represents the workload running using version 2.56.0 andv2.56.1+bw.2025_09_04_1715represents the same workload bu using a version build from this PR.