On Tue, 28 Apr 2026 07:33:21 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. > > Thank you for running the additional JMH benchmarks yourself. Those do show > good improvements with the proposed changes in this PR. > > Looking at the JMH benchmark that is made availbale in a comment in this PR > https://github.com/openjdk/jdk/pull/30931#issuecomment-4330459981, was a LLM > tool used to generate it? Some parts of it give me that impression, but it's > hard to detect these details and I might be wrong, so please do correct me if > that's not the case. > > If an LLM was indeed used to generate it, then I'll have to check with others > if it still follows the OpenJDK guidelines https://openjdk.org/legal/ai. The > JMH benchmark isn't being proposed as a contribution in this PR, so I don't > know if it still complies with the FAQ#5 in those guidelines and whether it's > OK to go ahead with this PR review and subsequent integration. Thanks @jaikiran for reviewing the changes and for your suggestions. I've addresses the changes in a separate commit with the commit message listing the key changes, but the major ones are: 1. The test now uses junit (and junit's assertions), `HttpTestServer`, and `URIBuilder`. 2. Replaced some instances of `assertEquals()` with `assertSame()` to ensure that the connection object and the cached buffer don't change after calling `sned()`. 3. Swapped out the lambda for a private static method. 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. ------------- PR Comment: https://git.openjdk.org/jdk/pull/30931#issuecomment-4345800335
