Conversation
Motivation: HuffmanDecoder collected the decoded symbols in a ByteArrayOutputStream, one synchronized write(int) per symbol, growing it from 32 bytes by doubling, and copied it out with toByteArray; Decoder then copied the result again into a String. The raw literal path allocated and copied once through InputStream.readNBytes and again into the String. This runs for every string literal of every inbound header block, which browsers Huffman code almost entirely. Modification: Decoder keeps two scratch arrays, reused across header blocks: the coded literal is read into one with readNBytes(byte[], int, int), and HuffmanDecoder.decode(buf, length, out) writes the symbols by index into the other, which the caller sizes from the static bound that the shortest code in the HPACK Huffman table is 5 bits (maxDecodedLength). The String is built straight from the scratch array with the ISO-8859-1 charset, the single copy left in either path. The arrays grow to the longest literal seen on the connection, which the max header list size and the dynamic table capacity bound. A MiMa exclude covers the package-private HuffmanDecoder signature. Result: No lock, no stream growth and one copy instead of two or three per string literal: decoding a typical browser request's header block is about 20-25% faster Huffman coded and 35-40% faster raw on JDK 17 (see the PR for the JMH figures, taken on a loaded laptop). Tests: - sbt "http-core/Test/testOnly org.apache.pekko.http.shaded.com.twitter.hpack.HuffmanDecoderSpec org.apache.pekko.http.impl.engine.http2.hpack.HpackDecoderSpec" (Scala 2.13.18 and 3.3.8) - sbt "http2-tests/Test/testOnly org.apache.pekko.http.impl.engine.http2.Http2ServerSpec org.apache.pekko.http.impl.engine.http2.Http2ClientSpec org.apache.pekko.http.impl.engine.http2.RequestParsingSpec" - sbt "http-bench-jmh/Jmh/run -f 1 -wi 3 -i 5 -w 1 -r 1 HpackDecoderBenchmark" on main and on this branch - sbt http-core/mimaReportBinaryIssues; sbt headerCheckAll; sbt javafmtCheckAll (JDK 17) - scalafmt --mode diff-ref=upstream/main References: Refs apache#1301 - the same change for the encoder; Refs apache#1251 - the InputStream decoding this builds on
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.
Motivation
The inbound mirror of #1301. For every string literal in every inbound HEADERS frame — and browsers Huffman-code nearly all of them —
HuffmanDecoder.decodecollected the symbols in aByteArrayOutputStream: onesynchronizedwrite(int)per decoded symbol, growth from the default 32 bytes by doubling (3–4 regrowths for a typicaluser-agent), then atoByteArraycopy;Decoder.readStringLiteralthen copied the result again into aString. The raw (non-Huffman) path was one allocation + copy inInputStream.readNBytes(int)and a second copy into theString.Modification
Decoderkeeps two scratch arrays, reused across header blocks (aDecoderis per-connection and single-threaded, like theEncoderin perf: encode HPACK header blocks from byte arrays into an unsynchronized ByteStringOutputStream #1301): the coded literal is read intoliteralBufwithreadNBytes(byte[], int, int), and for a Huffman literalHuffmanDecoder.decode(buf, length, out)writes the symbols by index intodecodedBuf. TheStringis built straight from the scratch array with ISO-8859-1 (every octet → the char of the same value, asStringTools.asciiStringFromBytesdid), the one copy left in either path.decodedBufis sized from a static bound rather than grown while decoding: the shortest code in the HPACK Huffman table is 5 bits, soncoded bytes decode to at mostn * 8 / 5symbols (HuffmanDecoder.maxDecodedLength). No growth logic in the decoder, no stream.HuffmanDecoder's EOS / invalid-padding checks and their preallocated exceptions are unchanged.HuffmanDecoder.decodesignature (MiMa 1.2.0 reports it, as it did for the encoder in perf: encode HPACK header blocks from byte arrays into an unsynchronized ByteStringOutputStream #1301).StringTools.asciiStringFromBytesno longer has a caller in main code; left in place with its spec since #1297 just added it — happy to drop it here if you prefer.Tests: new
HuffmanDecoderSpec(RFC 7541 Appendix C.4 vectors decoded directly, all-256-octet round trip against an encoder written from the HPACK table in the test itself so it does not depend onHuffmanEncoder's API, a literal of 5-bit codes that fills themaxDecodedLengthbound exactly, thelengthargument being honoured, EOS-in-stream and bad-padding rejection).HpackDecoderSpecgains: literals longer than the 128-byte initial scratch (Huffman and raw), consecutive blocks of varying literal lengths through oneDecoder(a shorter literal must not pick up the previous one's tail from the reused arrays), and a hand-built raw literal of octets 0x80–0xFF decoding to the chars of the same value.HpackDecoderBenchmarkadded tohttp-bench-jmh.Not touched: the twitter decoder does not detect padding longer than 7 bits (
a8eb10649cbf+ffdecodes asno-cache); that is pre-existing and out of scope here.Result
HpackDecoderBenchmark.decodeHeadersdecodes the same 10-header browser request block asHpackEncoderBenchmark(~430 octets of values, all string literals), JDK 17.0.19, Apple Silicon. Short runs (-f 1 -wi 3 -i 5 -w 1 -r 1) on a laptop that was under heavy unrelated load (1-min load average 5–20), so the error bars are wide — treat these as indicative and worth a quiet re-run:mainhuffman(forced on; what a browser sends)raw(forced off)defaultSo roughly −20–25% Huffman and −35–40% raw; the
defaultcolumn is too noisy to read. The remaining Huffman cost is the per-byte state machine (in.read()+ tree walk), which the lock removal does not touch.Tests
sbt "http-core/Test/testOnly org.apache.pekko.http.shaded.com.twitter.hpack.HuffmanDecoderSpec org.apache.pekko.http.impl.engine.http2.hpack.HpackDecoderSpec"— 18/18 on Scala 2.13.18 and 3.3.8sbt "http2-tests/Test/testOnly ...Http2ServerSpec ...Http2ClientSpec ...RequestParsingSpec"— 212 pass, 20 pending (pre-existing)sbt "http-bench-jmh/Jmh/run -f 1 -wi 3 -i 5 -w 1 -r 1 HpackDecoderBenchmark"onmainand on this branch — table abovesbt http-core/mimaReportBinaryIssues,sbt headerCheckAll,sbt javafmtCheckAll(JDK 17),scalafmt --mode diff-ref=upstream/main,git diff --check— cleanReferences
Refs #1301 - the same change for the encoder; Refs #1251 - the
InputStreamdecoding this builds on