On Thu, 30 Apr 2026 16:14:35 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 four additional
>> commits since the last revision:
>>
>> - Address PR suggestions
>>
>> 1. Added an assertion to check whether sendlock is held by the current
>> thread.
>>
>> 2. Replaced lambda (`captureAndAddtoBuffers`) with a private static
>> method (`copyBuffer`) since the lambda doesn't really save us any
>> significant lines of code.
>>
>> 3. Used `${test.main.class}` instead of explicitly spelling out the
>> class name.
>>
>> 4. Used junit instead of JDK test library assertions, updated the rest
>> of the test accordingly.
>>
>> 5. Used `HttpTestServer` and `URIBuilder` in the test, dropped the
>> initial warmup since the HTTP version is "2" by construction.
>>
>> 6. Used junit's `assertSame` method for ensuring that the connection
>> object and the cache header buffer are the same across various
>> `send()` invocations.
>>
>> 7. Minor fixes (updated copyright year and comments)
>> - Merge branch 'master' into JDK-8383248-reuse-buffer-in-header-encoding
>> - 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...
>
> test/jdk/java/net/httpclient/http2/HeaderEncodingBufferReuseTest.java line 70:
>
>> 68: httpUri = URIBuilder.newBuilder()
>> 69: .scheme("http")
>> 70: .host("localhost")
>
> I haven't looked at the updated PR (will do in the coming days), but a quick
> note about the usage `localhost` here. `localhost` may resolve to a
> non-loopback address. The server that is created a few lines above is bound
> to loopback address. Trying to issue a request to localhost can thus result
> in failures. Instead, please use
> `.host(testServer.getAddress().getAddress())` here.
>
> That brings me to the next point. GitHub actions job which gets run against
> these PRs only runs `tier1` tests. The networking tests are in `tier2`, so
> please run tier2 tests locally to verify that existing and this new test
> continues to pass with the proposed changes. `doc/testing.md` has additional
> details on how to run these tests.
In fact since `HttpTestServer.create(HTTP_2);` creates a server on the loopback
you could just replace ` .host("localhost")` with `.loopback()`: that's what we
do in most tests. It will take care of the issue mentioned by @jaikiran .
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30931#discussion_r3172888175
