Decode quoted-printable bodies a run at a time (#229) - #245
Conversation
Quoted-printable is the transfer encoding of most HTML newsletters and transactional mail, and it was the last transfer decoder here still running byte-at-a-time. decode_quoted_printable handed the whole body to the quoted_printable crate, which makes three passes over it -- a char-by-char copy into a String through a filter_map, a second walk with lines() and trim_end(), then a decode loop with one bounds-checked push per byte into a Vec that was never given a capacity. Bodies now go through vendor/mailparse/src/qp.rs: one allocation sized to the input, memchr to find line breaks and escapes, extend_from_slice for everything between them, so a line with no '=' in it is a single copy. Plain std plus the memchr already vendored here; no unsafe, no new dependency. The RFC 2047 encoded-word path in header.rs still uses the crate -- header-sized inputs, and different semantics. Nothing in the suite could see this path before. Every benchmark fixture is base64 or 8bit, which is also why the maintainer's profile of a full parse (53% base64 decode, 27% whitespace strip) describes base64 mail only: a quoted-printable message goes through none of it, and the gate would not have noticed this getting slower. So the benchmark comes with the change -- parse_qp_message over tests/data/valid_message.eml (89,932 bytes of quoted-printable HTML, 8,122 of text), plus a metadata-mode control on the same message, which is what shows the gain is the decode and not the parse around it. Apple M4 (10 vCPU), 5 interleaved rounds, pure-Python controls within 3.2%: parse_qp_message 0.219 -> 0.134 ms -39% parse_qp_message_metadata 0.022 -> 0.022 ms flat Every other benchmark landed inside the noise floor. Output is byte-identical to quoted_printable 0.5.1's Robust mode, which is the whole safety argument, so it is asserted three ways rather than claimed. The in-crate differential test compares the two over a generated corpus at every length to 96, over every (x, y) pair after '=' in the four positions where the crate's escape handling branches, and over hand-written cases for each of the six rules -- soft breaks, literal '=X' at end of line, non-hex octets resuming after Y, the trimmed set, bare CR, dropped bytes, empty lines, trailing newline. It also decodes the real fixture. The qp_agreement fuzz target asserts the same equality on arbitrary input through the public entry point: 22,934,494 executions in 601 s, zero findings. Writing that test is what turned up the reason quoted_printable is now pinned to =0.5.1. 0.5.2 changed what Robust mode emits for a body whose last line ends in a soft break -- b"abc=\n" decodes to b"abc\r\n" on 0.5.1 and b"abc" on 0.5.2 -- and the requirement here was a caret. Two consequences: cargo update could have changed body and encoded-word decoding with nothing in the suite to catch it, and this crate's own cargo test resolves its own lockfile, so it was pulling 0.5.2 while the extension links 0.5.1, which meant the differential test was comparing against a version that does not ship. The first run failed for exactly that reason. Cargo.lock is unchanged by the pin; the root already resolved to 0.5.1. The PR fuzz budget stays at a minute: three targets at 20 s rather than two at 30 s. deep-fuzz.yml gains the target in its matrix. PATCH.md describes the third changed function, the pin and its reasoning, and the sync checklist and diff -r claim name src/qp.rs; the root Cargo.toml comment, which described only the memmem change, is brought up to date. Signed-off-by: kurok <22548029+kurok@users.noreply.github.com>
519756e to
0e66c75
Compare
|
Rebased onto Two things the rebase turned up that are worth flagging. 1. #244 left a small gap in 2. The metadata benchmarks read ~23% faster on this branch, and that is not this PR's doing. Re-measured on the M4 against the new base:
Those metadata numbers are the M4 code-placement artifact I documented on #244 — master sits at 0.037 ms there, where it was 0.030 ms before base64-simd was linked, and this change happens to shift the layout back. It is not a quoted-printable win: metadata mode never transfer-decodes. The x86 gate measured those same benchmarks as flat on #244, so I expect it to show them flat here too, and the gate is the number to quote, not this table. Reporting it because a 23% swing on an unrelated benchmark should be explained rather than pocketed. The claim this PR actually makes is the first row and its control: same fixture, same headers, decode removed — 0.221 → 0.137 ms. Re-verified after the rebase: 811 passed / 3 skipped, the vendored suite 57 + 25 (all four QP differential tests among them), fmt for both manifests, clippy, mypy --strict, ruff. |
1 participant
Closes #229.
Branched from
master(abb31b8). Independent of #244, but both touchvendor/mailparse/PATCH.mdand the fuzz target list, so whichever lands second will want a small rebase — noted at the bottom.What changed
Quoted-printable is the transfer encoding of most HTML newsletters and transactional mail, and it was the last transfer decoder here still running byte-at-a-time.
decode_quoted_printablehanded the whole body to thequoted_printablecrate, which makes three passes over it: a char-by-char copy into aStringthrough afilter_map, a second walk withlines()+trim_end(), then a decode loop with one bounds-checkedpushper byte into aVecthat was never given a capacity.Bodies now go through
vendor/mailparse/src/qp.rs: one allocation sized to the input,memchrto find line breaks and escapes,extend_from_slicefor everything between them — so a line with no=in it is a single copy. Plainstdplus thememchralready vendored here. Nounsafe, no new dependency,Cargo.lockunchanged. The RFC 2047 encoded-word path inheader.rsstill uses the crate: header-sized inputs, different semantics.The benchmark had to come first
Nothing in the suite could see this path. Every fixture in
tests/benchmark/is base64 or 8bit — which is also why the profile everyone quotes (53% base64 decode, 27% whitespace strip) describes base64 mail only. A quoted-printable message goes through none of those fast paths, and the gate would not have noticed this getting slower.So per the issue's step 1, I added the benchmark and measured on master before writing a line of the decoder: 218 µs, against 22 µs for metadata mode on the same message. ~90% of that parse was the QP decode. That is what justified touching the vendored surface at all.
Numbers
Apple M4 (10 vCPU), rustc 1.98.0, CPython 3.12, 5 interleaved rounds (
ab_median.py). Pure-Python controls within 3.2%.parse_qp_message(new)parse_qp_message_metadata(new control)The control is the point: same fixture, same headers, same structure, no transfer decode. It does not move, so the gain is the decode and not the parse around it. Every other benchmark landed inside the noise floor.
One honest caveat on the size of the win. The issue estimated the decoder itself at 4–8× and end-to-end at 1.5–2.5×. End-to-end came in at 1.64×, inside that range, but the decode component went 197 µs → 112 µs, i.e. ~1.76× rather than 4–8×. The likely reason is named in the issue's own Out of scope:
quoted_printable_invalid_escapeinsrc/mail_parser.rsstill makes a full per-byte pass over every QP body, and it is now the largest remaining one. That is a separate issue, and this PR does not touch it.Full A/B report
Measured on
Apple M4, 10 vCPU.Median of 5 interleaved rounds per side; each value is a benchmark's minimum. Positive delta =
masteris slower.test__fast_mail_parser___attachment_rereadtest__fast_mail_parser___full_readtest__fast_mail_parser___parse_lazy_all_attachmentstest__fast_mail_parser___parse_lazy_untouchedtest__fast_mail_parser___parse_manytest__fast_mail_parser___parse_many_metadatatest__fast_mail_parser___parse_messagetest__fast_mail_parser___parse_message_stricttest__fast_mail_parser___parse_metadatatest__fast_mail_parser___parse_metadata_strtest__fast_mail_parser___parse_qp_messagetest__fast_mail_parser___parse_qp_message_metadatatest__fast_mail_parser___parse_treetest__fast_mail_parser___parse_tree_lazy_untouchedtest__fast_mail_parser___parse_tree_metadatatest__mail_parser___parse_messagetest__mailparser_lib___full_readtest__stdlib_email___full_readtest__threaded___parse_manytest__threaded___parse_many_metadata_smalltest__threaded___parse_many_smalltest__threaded___threadpool_parse_emailtest__threaded___threadpool_parse_email_smallByte-identical, asserted three ways
The entire safety argument is that output does not change, so it is tested rather than claimed:
qp.rs), againstquoted_printable::decode(.., Robust)as oracle: a generated corpus at every length to 96 over an alphabet that reaches every rule; every(x, y)pair after=in the four positions where the crate's escape handling branches (=x,=x\n,a=x,=xy,=xy\n— 65,536 pairs); hand-written cases for each of the six rules (soft break, literal=Xat end of line, non-hex octet resuming afterY, the trimmed set, bare CR, dropped bytes, empty lines, trailing newline); and the real fixture.qp_agreementfuzz target, same equality on arbitrary input through the public entry point: 22,934,494 executions in 601 s, zero findings. Canary included and verified to fire.test_quoted_printable_content_encoding, and the Pythontest_parse_warnings.py/test_stdlib_parity.pycontracts, all unchanged and green.Why
quoted_printableis now pinned to=0.5.1Writing that differential test is what found this, and it is a latent bug independent of this PR.
0.5.1 and 0.5.2 do not agree. For a body whose last line ends in a soft break,
b"abc=\n"decodes tob"abc\r\n"on 0.5.1 andb"abc"on 0.5.2 —if filtered.ends_with('\n')gained&& add_line_break == Some(true). The requirement here was0.5.0, a caret, so:cargo updatecould have changed body and encoded-word decoding, with nothing in the suite to catch it; andcargo testresolves its own lockfile, independent of the root one — so it was pulling 0.5.2 while the extension links 0.5.1. My first test run failed for exactly that reason, and a differential test that compares against a version you do not ship is worse than no test.The pin closes both.
Cargo.lockis unchanged: the root already resolved to 0.5.1.src/qp.rsreproduces 0.5.1, because that is what this library ships.Checks run locally
pytest tests --ignore=tests/benchmark(808 passed, 3 skipped), the vendored suite (55 + 25 passed) via--target-diroutside the tree,cargo fmt --all --checkandcargo fmt --manifest-path vendor/mailparse/Cargo.toml --check,cargo clippy --all-targets -- -D warnings -W clippy::cast_possible_truncation,mypy --strict,ruff check .— all green. Notarget/undervendor/mailparse/.__init__.pyi,tests/test_contract.pyanddocs/compatibility.mdare untouched, as the issue requires — there is no API or behaviour change.The M4 is not a proxy for x86; the Benchmark quality gate is the verdict.
Rebase note
#244 (base64 SIMD) also edits
vendor/mailparse/PATCH.md, the rootCargo.toml[patch.crates-io]comment,fuzz/Cargo.tomland theFUZZ_TARGETSlist — each adding a third fuzz target and a third changed function. Whichever merges second needs those merged by hand rather than taken wholesale; the conflicts are additive and small. #244 also adds.gitignoreentries for whatcargo fuzzleaves behind, which I deliberately did not duplicate here.