On Thu, 30 Apr 2026 12:53:33 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...
>
> Hello Ashay,
>
>> As for the JMH benchmark in the comment, yes, I did use an LLM to generate
>> the initial version (so that I can get some rough performance estimates),
>> but I was under the impression that that is okay since the benchmark is not
>> being added to the OpenJDK repo. Let me know if you'd prefer that I delete
>> the comment or write a new JMH benchmark from scratch.
>
> I asked about this and the input I have received from the relevant person is
> that GitHub pull request comments are components of the pull request and they
> must not contain AI-generated content. So please remove your comment which
> has the LLM generated JMH benchmark. You don't have to repost a benchmark.
Thanks @jaikiran and @dfuch! I've incorporated your suggestions except for
one, which was to use the connection label for checking whether the request was
made using the same connection as before. My rationale is for calling
`assertSame()` on the underlying connection object is that that seems like a
stronger check than checking the string label, but I'm happy to switch it to
the connection label check if that still seems like the best approach. Let me
know what you think.
Also, I ran through all tier 1, 2, and 3 tests in tests/jdk when built in the
FastDebug configuration, and they all passed:
==============================
Test summary
==============================
TEST TOTAL PASS FAIL ERROR
SKIP
jtreg:test/jdk:tier1 2584 2542 0 0
42
jtreg:test/jdk:tier2 4556 4309 0 0
247
jtreg:test/jdk:tier3 1680 1588 0 0
92
==============================
TEST SUCCESS
-------------
PR Comment: https://git.openjdk.org/jdk/pull/30931#issuecomment-4384088837
