On Fri, 24 Apr 2026 22:02:15 GMT, Ashay Rane <[email protected]> wrote:
>> Prior to this patch, every HTTP request created a new 16KB buffer for >> encoding the header, which is typically only a few hundred bytes long. >> This increased pressure on the garbage collector when the client created >> lots of requests. This patch instead makes the header encoder reuse the >> buffer that is created during the handling of the first request. >> >> The caveat, however, is that the downstream consumers of the header are >> asynchronous, so the encoder needs to take special care to ensure that >> it doesn't modify or invalidate the buffer after it hands the buffer >> over to the downstream asynchronous pipeline. To resolve this, this >> patch snapshots the buffer data into compact copies sized to the actual >> encoded length. Doing so makes the buffer immediately available for >> reuse via `clear()` and `limit()`. >> >> For typical requests, this reduces per-request allocation from 16KB to >> a few hundred bytes (i.e. the size of the compact copy of the encoded >> headers), with the 16KB encoding buffer allocated once per connection >> instead of once per request. >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > Ashay Rane has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains two additional commits since > the last revision: > > - Merge branch 'master' of https://github.com/openjdk/jdk into > JDK-8383248-reuse-buffer-in-header-encoding > - Reuse buffer for encoding headers instead of allocating one per request > > Prior to this patch, every HTTP request created a new 16KB buffer for > encoding the header, which are typically only a few hundred bytes long. > This increased pressure on the garbage collector when the client created > lots of requests. This patch instead makes the header encoder reuse the > buffer that is created during the handling of the first request. > > The caveat, however, is that the downstream consumers of the header are > asynchronous, so the encoder needs to take special care to ensure that > it doesn't modify or invalidate the buffer after it hands the buffer > over to the downstream asynchronous pipeline. To resolve this, this > patch snapshots the buffer data into compact copies sized to the actual > encoded length. Doing so makes the buffer immediately available for > reuse via `clear()` and `limit()`. > > For typical requests, this reduces per-request allocation from ~16KB to > a few hundred bytes (i.e. the size of the compact copy of the encoded > headers), with the 16KB encoding buffer allocated once per connection > instead of once per request. src/java.net.http/share/classes/jdk/internal/net/http/Http2Connection.java line 1642: > 1640: private List<ByteBuffer> encodeHeadersImpl(int bufferSize, > HttpHeaders... headers) { > 1641: List<ByteBuffer> buffers = new ArrayList<>(); > 1642: Consumer<ByteBuffer> captureAndAddToBuffers = (this_buffer) -> { It feels a bit odd to be creating a lambda/Consumer style function here. Can this instead be changed to a private static method which takes a `ByteBuffer`, allocates a copy of that `ByteBuffer` and returns that copy? The callsite(s) can then call this private method and add that returned copy to the `buffers` `List`. test/jdk/java/net/httpclient/http2/HeaderEncodingBufferReuseTest.java line 32: > 30: * @run main/othervm > 31: * --add-opens java.net.http/jdk.internal.net.http=ALL-UNNAMED > 32: * HeaderEncodingBufferReuseTest Nit - for new tests, it would be good to use `${test.main.class}` to refer to the test class. So: * @run main/othervm * --add-opens java.net.http/jdk.internal.net.http=ALL-UNNAMED * ${test.main.class} test/jdk/java/net/httpclient/http2/HeaderEncodingBufferReuseTest.java line 33: > 31: * --add-opens java.net.http/jdk.internal.net.http=ALL-UNNAMED > 32: * HeaderEncodingBufferReuseTest > 33: */ Nit - it might be better to move this entire test declaration right above the class declaration. I find it easier to read/spot when reviewing tests. test/jdk/java/net/httpclient/http2/HeaderEncodingBufferReuseTest.java line 49: > 47: import jdk.httpclient.test.lib.http2.Http2TestExchange; > 48: import jdk.httpclient.test.lib.http2.Http2TestServer; > 49: import jdk.test.lib.Asserts; We should instead convert this test to a `junit` test. That way instead of launching it as `main/othervm` you launch it as `junit/othervm` and that then brings in the JUnit test library and you can then use `org.junit.jupiter.api.Assertions` instead of this JDK specific test library. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/30931#discussion_r3152162938 PR Review Comment: https://git.openjdk.org/jdk/pull/30931#discussion_r3152178876 PR Review Comment: https://git.openjdk.org/jdk/pull/30931#discussion_r3152169264 PR Review Comment: https://git.openjdk.org/jdk/pull/30931#discussion_r3152188675
