On Tue, 28 Apr 2026 07:16:50 GMT, Jaikiran Pai <[email protected]> wrote:
>> 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. > > test/jdk/java/net/httpclient/http2/HeaderEncodingBufferReuseTest.java line 68: > >> 66: Object conn = getHttp2Connection(client); >> 67: Asserts.assertEquals(send(client, uri, 2).statusCode(), 200); >> 68: ByteBuffer cached = (ByteBuffer) getField(conn, >> "cachedHeaderBuffer"); > > I think this will need some slight adjustment to the test to assert that the > connection that was used for the request was indeed the same one. There are > ways to verify it, but I think we can get to that part once the rest of the > review comments are addressed. The `HttpResponse` has a `connectionLabel()` method that can be used to check whether two requests were made on the same connection. Though that method returns an Optional, our implementation should never return Optional.empty(). ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/30931#discussion_r3172906195
