okhttp: avoid unnecessary Header allocation when indexing already-lowercase names in Hpack.Writer - #2
Draft
jaipilot[bot] wants to merge 1 commit into
Draft
Conversation
…ercase names in Hpack.Writer
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Builds on grpc#12907 ("okhttp: optimize HPACK to index :path and fix dynamic table eviction bugs"), which fixed a bug where headers were inserted into the HPACK dynamic table using their original-case name instead of the lowercased name used for indexed lookups, by unconditionally allocating
new Header(name, value)at both dynamic-table-insertion call sites inWriter.writeHeaders.This change avoids that allocation in the common case:
ByteString.toAsciiLowercase()returnsthis(same reference) when the name has no uppercase ASCII bytes, which is true for gRPC's generated header names and the newly-indexable:path/:authoritypseudo headers. A smallcanonicalHeader(header, name, value)helper reuses the originalHeaderinstance whenname == header.name, and otherwise allocates exactly as before (preserving the original PR's case-folding fix for mixed-case names).Why
Header.equals/hashCode/hpackSize are purely value-based (over
name/valuecontent), andHeaderis immutable, so reusing the original instance when its name is already the indexing name is behavior-preserving: identical bytes are written, identical dynamic table state (size, order, byte accounting) results, and identical case-insensitive matching semantics apply.Evidence
HpackTest#mixedCaseHeaderNameIndexedOnRepeat, verifying a mixed-case header is stored lowercase and recognized as an indexed reference on repeat. Passes identically before and after the change (59/59 HpackTest tests both times).writeHeadersallocating 5Headerobjects (one per header) on the original code for a representative gRPC header block, versus 0 on the optimized code, reproduced across multiple runs. A ThreadMXBean allocated-bytes benchmark (5 runs x 200k iterations each side) showed a consistent 8488 -> 8368 bytes/iteration reduction.grpc-okhttpmodule build (compile, test, checkstyle) passes: 360 tests, 0 failures/errors.Limitations
Writer.writeHeaderscall sites and the eviction/indexing logic already touched by the evaluated PR.Generated by JAIPilot Cloud for #1 from Anthropic session
sesn_01758uGvBonuiwxT2TZEkHEP.