perf: reuse HPACK encode buffer across frames - #929
Merged
seanmonstar merged 1 commit intoAug 3, 2026
Conversation
`into_encoding` allocated a fresh `BytesMut` for every HEADERS and PUSH_PROMISE frame, grew it through several capacity doublings while HPACK encoded into it, then called `.freeze()` — allocating again for the `Bytes` refcount header. That is at least two allocations per outbound header block, on every request and every response. The block does not need to outlive the call: on the common path it is copied into the connection write buffer and dropped immediately. So `hpack::Encoder` now keeps one scratch buffer that `into_encoding` takes and `encode` hands back once the block has been written. `EncodingHeaderBlock::hpack` becomes `BytesMut`, which removes the `.freeze()` as well. The CONTINUATION path keeps its remainder instead of returning the buffer, and splitting that remainder is now `split_to` on a uniquely-owned buffer rather than a copy. Same bytes on the wire; no public API change. `cargo bench --bench main`, 5 interleaved rounds per arm: multi-thread 100k requests improves 3.8 % (faster in 24 of 25 pairwise comparisons, exact permutation test p = 0.016), current-thread 1.5 %, and the write-contention benchmark is unchanged as expected.
dorianverlaine
added a commit
to dorianverlaine/pingclair
that referenced
this pull request
Aug 3, 2026
The fork made the H1 `BodyReader` reuse its 64 KiB read buffer across keepalive requests instead of allocating one per message body, and stopped `finish_body_buf` splitting when there was nothing overread. Both are reverted, along with 35,496 lines of vendored crate. The mechanism was real and the allocation win was real — cumulative allocations over a 300k-request profile fell from 13.4 GB to 1.9 GB, and the profile went from "one function is 91 % of bytes" to "nothing above 3.4 %". What never appeared was a reason to care. Three independent attempts now agree: - the original local profile: CPU per request 51.6 vs 49.3 µs, inside noise; - an OrbStack A/B on this commit's binaries, differing by exactly the one `[patch.crates-io]` line: +0.9 % throughput, and RSS ranges that overlap almost entirely (fork 27.3-31.4 MB, base 28.3-31.3 MB); - the published c7i-flex table, still 55 % behind nginx on H1 proxy, which allocation cleanup did not move. The A/B recorded proxy, backend and client CPU every round and discarded any round where pingclair was not the saturated tier. That check rejected 5 of 10 runs — the first harness had nginx pegged at 203 % of a 200 % quota while pingclair had headroom, so it would have measured nginx. Only two paired rounds survived, at +2.0 % and -0.4 %. The standing hypothesis that the payoff appears on memory-constrained instances (t3/t4g) is untested and stays untested; it is not worth an AWS run, 35k lines of vendored code, a hand-applied upgrade path, and a `cargo audit` blind spot for an effect that has now failed to appear three times. Removing the patch restores `source` and `checksum` on the lockfile entry, so pingora-core is covered by the nightly audit again. `[patch.crates-io]` now holds only h2, which is the one fork measured against its own project's benchmark and is upstream as hyperium/h2#929. Gates: fmt, clippy, and 637 workspace tests all pass.
dorianverlaine
added a commit
to dorianverlaine/pingclair
that referenced
this pull request
Aug 3, 2026
`[patch.crates-io]` strips `source` and `checksum` from a lockfile entry, and cargo-audit only reports on crates it can trace back to crates.io. So every patched crate was silently exempt from the nightly audit — verified directly rather than inferred: a project depending on `atty 0.2.14` reports RUSTSEC-2021-0145 and RUSTSEC-2024-0375, and the same project with `atty` path-patched reports nothing and exits clean. That exemption pointed the wrong way for the one crate still patched here. h2 is where the HPACK denial-of-service (RUSTSEC-2023-0034) and the 2024 CONTINUATION-flood class landed, and the Pingclair patch is inside the HPACK encoder. The audit now runs a second time against a lockfile resolved with the patch section stripped. Both preconditions fail the job loudly rather than passing vacuously: that the section was actually removed, and that h2 actually resolves to the registry afterwards. The step becomes unnecessary when hyperium/h2#929 lands and `vendor/h2` goes away. GUARDRAILS gains three entries from this week, each one a failure that already happened: measure a fork where the patched thing is the saturated resource before vendoring it, `[patch]` hides crates from the audit, and `target/` grows unbounded because cargo never collects old artifacts (77 GB against 12 GiB of free disk; `cargo clean` reclaimed 113 GB; routine handling is now `cargo sweep --time 7`).
dorianverlaine
added a commit
to dorianverlaine/pingclair
that referenced
this pull request
Aug 3, 2026
Brings in the H3, static-file and header-path work from `perf/request-cost`, plus the fork audit that ran alongside it. Performance, all measured: - H3 gets GSO-backed batching (the per-connection out buffer was 1350 bytes, so every QUIC packet became its own syscall), a bounded per-stream chunk queue in place of a byte ring, and `max_ack_delay = 0` so a body the server is draining does not trickle at one packet per 25 ms. - The static file server prebuilds per-file response metadata behind an `ArcSwap`, so a hit clones a few `HeaderValue`s instead of reformatting dates, ETags and lengths per request. - The proxy path stops rebuilding `Via`, request-id and forwarding-header values that only depend on things known before the request arrived. Correctness, found while reviewing the above: - A repeated header name reused the first value, so a route with `header +Vary Accept-Encoding` merged with CORS emitted `Vary: Accept-Encoding` twice and dropped `Vary: Origin` — a shared cache could then serve one origin's response to another. HTTP/3 was never affected, which made it a parity gap too. Forks: three were vendored on this branch and one survives. - `h2` stays: +3.8 % on h2's own multi-threaded benchmark, faster in 24 of 25 pairwise comparisons (p = 0.016). Submitted as hyperium/h2#929; when it lands, `vendor/h2` and the `[patch]` section go away. - `pingora-core` and `pingora-http` are gone (38,532 lines). Both had a sound mechanism and neither ever produced a number from a run where the thing it patched was the saturated resource. The rule that came out of it, now in GUARDRAILS: measure a fork where the patched component is the bottleneck, or do not vendor it. The first harness built for exactly that question was itself invalid — nginx sat at 203 % of a 200 % quota while pingclair had headroom — so the A/B now records all three tiers' CPU per round and discards any round pingclair did not saturate. Also closes the audit gap those forks opened: `[patch.crates-io]` strips `source` and `checksum` from the lockfile, which makes a crate invisible to `cargo audit`, so the nightly job now re-audits a lockfile resolved without the patch section. Gates: fmt, clippy, and 640 workspace tests all pass.
seanmonstar
approved these changes
Aug 3, 2026
seanmonstar
left a comment
Member
There was a problem hiding this comment.
Nice! hyper's h1 internals do a similar thing, reusing the header encoding buffer over and over on a single connection. Thanks!
dorianverlaine
added a commit
to dorianverlaine/pingclair
that referenced
this pull request
Aug 3, 2026
hyperium/h2#929 was merged on 2026-08-03 as 27b730ee, verbatim — diffing the submitted commit against h2 master shows no change to either patched file. seanmonstar's review noted hyper's HTTP/1 internals already reuse the header encoding buffer the same way, so this closed the equivalent HTTP/2 gap. The distinction that matters for us is merged versus released: the newest h2 release is still 0.4.15 (2026-06-15), which predates the merge, so deleting `vendor/h2` now would hand back the measured 3.8 % until h2 cuts its next release. The note therefore records the exact commit to wait for, and lists everything that gets deleted together when it arrives — the patch entry, the check script and its lint step, and the strip-the-patch audit pass all exist solely to hold this one fork safely. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
into_encodingallocates a freshBytesMutfor every HEADERS/PUSH_PROMISEframe, lets it grow through several capacity doublings while HPACK encodes
into it, and then calls
.freeze()— which allocates again for theBytesrefcount header. That is at least two allocations per outbound header block,
on every request and every response.
The block does not need to outlive the call: on the common path it is copied
into the connection write buffer (
dst.put_slice) and dropped immediately.So the buffer can simply be reused across frames on the same connection.
Solution
hpack::Encoderkeeps one scratchBytesMut.into_encodingtakes it,clears it, and encodes into it; once
EncodingHeaderBlock::encodehascopied the block into the write buffer, it hands the buffer back.
EncodingHeaderBlock::hpackbecomesBytesMutinstead ofBytes, so the.freeze()goes away too. The CONTINUATION path keeps its remainder anddoes not return the buffer — the next encode just starts from a fresh one —
and splitting that remainder is now
split_toon a uniquely-ownedBytesMutrather than a copy.Nothing else changes: same bytes on the wire, no public API change, both new
methods are
pub(crate).Memory
Worth stating explicitly given #923: this retains one buffer per connection,
sized to the largest header block that connection has encoded, for the
connection's lifetime. For ordinary headers that is well under 1 KiB; a
connection that once sent a header block near
max_frame_sizewill holdthat much until it closes. If that tradeoff is unwanted, the buffer could be
capped or released on idle — happy to add either.
Measurements
cargo bench --bench mainon this repo, 5 interleaved rounds per arm(master vs this branch, rebuilt each round), Apple M-series,
Overallin ms,lower is better:
The multi-thread row separates cleanly — the patched arm is faster in 24 of
25 pairwise comparisons (exact permutation test, two-sided p = 0.016). The
write-contention benchmark is DATA-frame throughput and is unaffected, which
is what you would expect.
It is a small win, and I would not argue it is more than that. It is worth
having mainly because it is on every request in both directions and costs
nothing to keep.
Testing
cargo test --allpasses, including the full HPACK fixture suite andwrite_continuation_frames/read_continuation_frames/too_many_continuation_frames_sends_goaway, which cover the split path thischange touches.