chore: Add more QPACK encoding vectors - #3904
Conversation
In preparation of more work on QPACK. Includes RFC 9204 Appendix B test vectors and exaples from tp6 traces. Also clean up some existing tests by using new helpers.
There was a problem hiding this comment.
Pull request overview
Adds broader QPACK encoding coverage using RFC 9204 Appendix B and Firefox tp6-derived vectors.
Changes:
- Adds RFC instruction and header-block vectors.
- Adds real-world encoding edge-case tests.
- Introduces reusable encoder test helpers.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
neqo-qpack/src/qpack_send_buf.rs |
Tests Huffman inflation. |
neqo-qpack/src/header_block.rs |
Adds RFC header-block vectors. |
neqo-qpack/src/encoder.rs |
Adds helpers and extensive encoding tests. |
neqo-qpack/src/encoder_instructions.rs |
Adds RFC instruction vectors. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3904 +/- ##
==========================================
- Coverage 97.05% 96.76% -0.30%
==========================================
Files 115 118 +3
Lines 41097 39943 -1154
Branches 41097 39943 -1154
==========================================
- Hits 39886 38650 -1236
- Misses 1194 1268 +74
- Partials 17 25 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Test-only change; no production code touched, so no API/security/perf surface. I verified the new vectors by hand against RFC 9204 rather than by running them (the sandbox here blocks cargo test):
- RFC 9204 Appendix B: B.1 block
0000 510b /index.html, B.23fbd01+c00f/c10c+ block0381 1011, B.34a…0c…, B.40500 80c1 81, B.5810d…, andB_MAX_ENTRIES = 220/32 = 6all check out, including the Required Insert Count and Base Delta encodings (§4.5.1). - Builders:
push_prefixed_intmatches RFC 7541 §5.1 including theprefix_len == 0case; the prefix/prefix_lenpairs (0xc0/2,0x50/4,0x20/4,0x40/2,0x00/1) matchprefix.rsand the H-bit placement is right. - Static indices: every claimed index in
FIREFOX_REQUEST/FIREFOX_RESPONSE/REPEATED_VARYmatchesstatic_table.rs, andexpires,pragma,x-amzn-requestid,akamai-grn,sec-fetch-*,content-security-policy-report-onlyare genuinely absent. - Byte-count assertions: the
7f 69,5f 04,27 03,4a…00,2 + 3*3 + 6 + 8 + 7, and 4-vs-11 arithmetic are all correct — including that index 59 needs a two-byte name reference. - Feature gating: the
#[cfg(test)]justification for testing B.4/B.5 at the instruction layer instead of end-to-end is accurate.
Two architectural notes:
-
The
expected_block/assert_round_tripsscaffolding is the valuable part, but it's load-bearing on an assumption that isn't asserted. Everyencode_fieldscaller silently requires Required Insert Count == 0 (assert_round_tripshardcodes an unrelatedMAX_ENTRIES, andexpected_blockhardcodes the00 00prefix). The day someone adds aFieldsvector that does touch the dynamic table, the failure will be a byte diff in the prefix rather than "this helper doesn't cover that". Adebug_assert_eq!(&block[..2], [0x00, 0x00])inexpected_block's contract, or a doc line onFields, would make the boundary explicit.Repralso can't express dynamic/post-base references, which is whyinsert_while_base_is_non_zeroandfirefox_response_is_all_literalsfall back to hand-written bytes or a weaker encoder config — extendingReprwithPostBase(u64)/Dynamic(u64)would let the helper cover the insert path too, which is the path this PR is preparing to change. -
Comment density. The prose is unusually essayistic for this tree ("Decoding is what keeps
expected_blockhonest", "Guards the guard", "The block first: when a change breaks both, that is the more telling failure"). The factual comments — static-table index rationale, whyOriginmisses entry 60 on case, why B.3 differs from neqo's behaviour, the recording provenance — are exactly right and worth keeping. The meta-commentary about test design could come down by half without losing anything for this audience.
Also: the tp6 provenance link and Firefox pref names are genuinely useful, but FIREFOX_MAX_TABLE_SIZE/FIREFOX_MAX_BLOCKED_STREAMS will drift from the prefs silently. Worth a one-line note that these are a 2026-03 snapshot rather than a live contract.
There was a problem hiding this comment.
Test-only change; no production code touched. cargo test is blocked in this sandbox, so I verified the vectors by hand again against RFC 9204 and the tables in-tree.
All nine inline comments from my previous pass are addressed — encode_header_block_returning rename, firefox_request_makes_no_inserts / firefox_response_makes_no_inserts renames, the connect_firefox counterpart for the response vector (firefox_response_inserts_names_absent_from_static_table) plus the Repr::Literal if inserting branch, the issue #49 link, the send_instructions drain loop, hoisting DECODE_MAX_ENTRIES, and instruction round-tripping via test_encoding_decoding in assert_marshals_to. The hand-rolled push_prefixed_int/push_literal builders are gone entirely, which resolves the prefix-masking inconsistency, and the duplicated Huffman-inflation test at the encoder layer is gone too.
Re-verified on this revision: B.1 0000 510b, B.2 3fbd01 / c00f / c10c / block 0381 1011, B.3 4a…0c…, B.4 02 and 0500 80c1 81, B.5 810d…; B_MAX_ENTRIES = 220/32 = 6 per §3.2.2; Required Insert Count / Base Delta per §4.5.1 for the [0x06, 0x84, …] and [0x07, 0x80, 0x10] blocks; 7f 69, 5f 04, 27 03, 4a…00, 2 + 3*3 + 6 + 8 + 7; and HUFFMAN_TABLE lengths 23/28/28 for 0x01/0x02/0x06 → 79 bits → 10 bytes, so 4-vs-11 is right. Static indices all match static_table.rs.
One architectural consequence worth stating explicitly, since the comment at expected_block gets close but not all the way: rendering the expectation through HeaderEncoder means encode_fields no longer pins bytes at all — it pins the representation the encoder chose per field, and the byte encoding of those representations is asserted only by the four rfc9204_* tests and builders_match_vectors. That is a defensible trade (the hand-rolled builders were themselves a second implementation to keep correct), but it means a change to HeaderEncoder that is wrong in the same way for both sides is caught only by assert_round_trips — i.e. by the decoder. Worth keeping in mind when the insert path changes: if Repr grows Dynamic/PostBase variants, the Firefox vectors will pin even less on their own, so the rfc9204_* byte anchors are the part that has to grow with them.
Otherwise this is a good base for the upcoming QPACK work. firefox_request_repeated_is_not_shorter documenting #49 as an assertion rather than a #[should_panic] or an #[ignore] is the right call.
Performance profiles for profiler.firefox.comBenchmarks (14)
|
There was a problem hiding this comment.
Test-only change; no production code, no unsafe, no public API surface touched. I re-verified the new vectors against the specs and they hold up:
- RFC 9204 B.1–B.5 byte strings, prefix arithmetic (
RIC % 2·max_entries + 1, sign bit, delta) and theB_MAX_ENTRIES = 220/32 = 6modulus all match the RFC. - Every static-table index in
FIREFOX_REQUEST/FIREFOX_RESPONSE/REPEATED_VARY/LONG_PATHmatches Appendix A, including the name-only cases the comments call out (31gzip, deflate, br, 76get, 59 vs. 60 on case). huffman_table.rsconfirmsLONG_CODES = [0x01, 0x02, 0x06]are 23/28/28 bits → 79 bits → 10 bytes, so the 4-vs-11 assertion is right.
Previous round resolved: the encode_header_block_returning rename, firefox_request_makes_no_inserts, the connect_firefox response counterpart, the lossless two-direction send_instructions drain, hoisting TEST_MAX_ENTRIES onto ADDITIONAL_TABLE_ENTRY_SIZE, expected_inserts + FIRST: Fields, assert!(decoded.is_err()), and folding test_encoding_decoding into assert_marshals_to. Dropping the hand-rolled prefix encoder in favour of EncoderInstruction::marshal is a clear improvement — the reference encoder is now the real one.
Two PR-wide observations:
-
expected_blockis a second encoder. It reimplements representation selection overHeaderEncoder, so a change toencode_header_block's choice logic fails ~10 tests at once with a byte diff rather than one test naming the field that changed.assert_round_tripsmitigates the "both writers agree on wrong bytes" risk, but not the diagnosability one. An alternative worth weighing: keepFields/Reprpurely as the assertion (walk the block and check each field's representation tag) instead of re-rendering it. Not blocking — the current shape is fine given the RFC vectors anchor the byte level. -
send_instructionsnow drains to quiescence rather than doing one round trip, which silently strengthens ~30 pre-existing tests (several call sites previously never flushed at all and now assert "no instructions emitted"). That's a good change, but worth a line in the commit message so a future bisect doesn't attribute a failure there to the vectors.
Minor, outside the diff: encoder.rs:350 still divides by a literal 32; now that the tests import ADDITIONAL_TABLE_ENTRY_SIZE for the same computation, using it there too would keep the two in step.
Benchmark resultsNo significant performance differences relative to 7fd585b. All resultsstreams/walltime/1-streams/each-1000-bytes: Change within noise threshold. time: [549.34 µs 551.39 µs 553.77 µs]
thrpt: [1.7222 MiB/s 1.7296 MiB/s 1.7361 MiB/s]
change:
time: [+0.6831% +1.2192% +1.7654] (p = 0.00 < 0.05)
thrpt: [-1.7348% -1.2046% -0.6785]
Change within noise threshold.
Found 13 outliers among 100 measurements (13.00%)
1 (1.00%) high mild
12 (12.00%) high severestreams/walltime/1000-streams/each-1-bytes: Change within noise threshold. time: [10.928 ms 10.944 ms 10.961 ms]
thrpt: [89.095 KiB/s 89.232 KiB/s 89.360 KiB/s]
change:
time: [-0.5234% -0.3185% -0.1232] (p = 0.00 < 0.05)
thrpt: [+0.1233% +0.3195% +0.5262]
Change within noise threshold.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mildstreams/walltime/1000-streams/each-1000-bytes: Change within noise threshold. time: [30.848 ms 30.885 ms 30.924 ms]
thrpt: [30.840 MiB/s 30.878 MiB/s 30.916 MiB/s]
change:
time: [+0.2634% +0.4390% +0.6203] (p = 0.00 < 0.05)
thrpt: [-0.6164% -0.4371% -0.2627]
Change within noise threshold.streams-flow-controlled/walltime/1-streams/each-4194304-bytes: Change within noise threshold. time: [25.648 ms 25.682 ms 25.718 ms]
thrpt: [155.53 MiB/s 155.75 MiB/s 155.96 MiB/s]
change:
time: [-0.7914% -0.6136% -0.4450] (p = 0.00 < 0.05)
thrpt: [+0.4470% +0.6174% +0.7977]
Change within noise threshold.streams-flow-controlled/walltime/10-streams/each-1048576-bytes: Change within noise threshold. time: [71.248 ms 71.332 ms 71.419 ms]
thrpt: [140.02 MiB/s 140.19 MiB/s 140.36 MiB/s]
change:
time: [+0.1601% +0.3244% +0.4972] (p = 0.00 < 0.05)
thrpt: [-0.4948% -0.3234% -0.1598]
Change within noise threshold.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mildtransfer/walltime/pacing-false/varying-seeds: Change within noise threshold. time: [17.730 ms 17.742 ms 17.754 ms]
thrpt: [225.30 MiB/s 225.46 MiB/s 225.60 MiB/s]
change:
time: [-0.7822% -0.6973% -0.6101] (p = 0.00 < 0.05)
thrpt: [+0.6138% +0.7022% +0.7884]
Change within noise threshold.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high severetransfer/walltime/pacing-true/varying-seeds: Change within noise threshold. time: [18.216 ms 18.225 ms 18.234 ms]
thrpt: [219.37 MiB/s 219.48 MiB/s 219.59 MiB/s]
change:
time: [-0.3176% -0.2392% -0.1605] (p = 0.00 < 0.05)
thrpt: [+0.1608% +0.2398% +0.3186]
Change within noise threshold.
Found 2 outliers among 100 measurements (2.00%)
1 (1.00%) low mild
1 (1.00%) high mildtransfer/walltime/pacing-false/same-seed: Change within noise threshold. time: [17.741 ms 17.751 ms 17.761 ms]
thrpt: [225.22 MiB/s 225.34 MiB/s 225.47 MiB/s]
change:
time: [+0.0121% +0.0987% +0.1845] (p = 0.02 < 0.05)
thrpt: [-0.1841% -0.0986% -0.0121]
Change within noise threshold.
Found 1 outliers among 100 measurements (1.00%)
1 (1.00%) high mildtransfer/walltime/pacing-true/same-seed: Change within noise threshold. time: [18.275 ms 18.285 ms 18.296 ms]
thrpt: [218.63 MiB/s 218.75 MiB/s 218.88 MiB/s]
change:
time: [-1.1586% -1.0706% -0.9764] (p = 0.00 < 0.05)
thrpt: [+0.9861% +1.0822% +1.1721]
Change within noise threshold.Download data for |
Client/server transfer resultsPerformance differences relative to 7fd585b. Transfer of 33554432 bytes over loopback, min. 100 runs. All unit-less numbers are in milliseconds.
Table above only shows statistically significant changes. See all results below. All resultsTransfer of 33554432 bytes over loopback, min. 100 runs. All unit-less numbers are in milliseconds.
Download data for |
Failed Interop TestsQUIC Interop Runner, client vs. server, differences relative to
All resultsSucceeded Interop TestsQUIC Interop Runner, client vs. server neqo-pr as client
neqo-pr as server
Unsupported Interop TestsQUIC Interop Runner, client vs. server neqo-pr as client
neqo-pr as server
|
In preparation of more work on QPACK.
Includes RFC 9204 Appendix B test vectors and exaples from tp6 traces.
Also clean up some existing tests by using new helpers.