Evaluate a part's body once, and stop copying text bodies twice (#230) - #253
Conversation
|
The gate was right, and fixing it properly found a second problem. The failure was At that size it said something the small one could not. The owned-decode shortcut is a pessimisation. Handing the decoded Reverting that one arm:
So base64 and quoted-printable keep their original route, That is the second thing #230 prescribed that measurement refused, after the
Controls within 2.4%.
One note on the instrument. I tried to dispatch |
get_body_encoded() re-reads a part's headers to find its transfer encoding, and both the full and the lazy parser called it two or three times per part: once for the quoted-printable escape check, once for the encoded size, and again inside get_body_raw. It is now called once and threaded to all three. Two copies went with it, and neither bought anything: A 7bit/8bit/binary body *is* its raw bytes, so get_body_raw copied it into a Vec only so decode_charset could borrow it straight back. A base64 or quoted-printable body, once transfer-decoded, was lent to encoding_rs by reference -- which validates UTF-8 and hands back a Cow::Borrowed -- and into_owned() then copied the whole body again. So text bodies now decode from the borrowed slice where the encoding is already plaintext, and give the decoded Vec to String::from_utf8 where it is not. The two cases where that would not be equivalent are routed to the old path: a BOM, which encoding_rs strips and from_utf8 would keep as U+FEFF, and invalid sequences, which encoding_rs replaces with U+FFFD and from_utf8 rejects. The resolved charset is compared rather than the raw label, because Charset::for_label maps utf8, UTF-8 and unicode-1-1-utf-8 onto one charset. tests/test_charset_decoding.py pins all of that -- BOMs across four transfer encodings, invalid UTF-8, four spellings of the label, a non-UTF-8 charset and an unknown one. All sixteen pass against master as well, which is the point: they are the oracle for the claim that the shortcut changed nothing, not a test of the shortcut. What is NOT here is the issue's other half. It asks for the escape scan to use memchr_iter, and I implemented it, benchmarked it, and took it back out. The new parse_qp_dense_escapes benchmark -- one '=' every three bytes, the scan's worst case -- came back 10.1% SLOWER with memchr against a 1.2% control floor, because memchr's per-call setup never amortises when the hits are three bytes apart, while the byte loop's test is one comparison. Reverting that hunk alone moved it to -1.7% and left every other gain in place, which is how I know it was the memchr and not placement. The dependency declaration came out with it, so Cargo.toml, fuzz/Cargo.toml and Cargo.lock are unchanged. Apple M4 (10 vCPU), 5 interleaved rounds, controls within 0.5%: parse_many_small_serial 4.054 -> 3.733 ms -8% parse_8bit_text 0.003 -> 0.003 ms -8% parse_qp_message 0.135 -> 0.131 ms -2.4% parse_qp_dense_escapes 0.017 -> 0.017 ms flat vendor/ is untouched. Signed-off-by: kurok <22548029+kurok@users.noreply.github.com>
Two fixes to this branch, both found by making its own benchmarks big enough to mean anything. The gate failed on parse_base64_utf8_text at +14.5%. That benchmark ran in 11 us on the runner, an order of magnitude below every other gated one, so a 2 us wobble was a 14.5% verdict and fixed per-call cost -- FFI, header parse -- was most of what it measured rather than the body handling it is named for. All three benchmarks this branch adds are now ~10x larger: 21, 49 and 175 us on an M4, in the same league as parse_metadata and parse_qp_message. At that size the benchmark said something the small one could not: the owned-decode shortcut is a PESSIMISATION. Handing a decoded Vec to String::from_utf8 instead of lending it to encoding_rs saves a copy, and still measured 8% slower on a 128 KB UTF-8 body -- 0.045 -> 0.049 ms, zero variance across five rounds on both sides. encoding_rs validates UTF-8 with SIMD and std does not, and that difference is bigger than the copy it saves. Reverting that one arm moved the benchmark to +0.3% and left both real wins standing: parse_qp_message -9.8%, parse_8bit_text -6.4%. So decode_charset keeps its original route for base64 and quoted-printable, decode_body's comment records the measurement, and decode_charset_owned is gone. That is the second thing #230 prescribed that measurement refused -- the memchr escape scan was the first, 10% slower on its worst case. What survives is what the issue got right: evaluate the body once, and do not copy a body that is already plaintext. tests/test_charset_decoding.py stays. It pins BOM handling, invalid UTF-8 and the charset-label aliasing, all of which pass on master too; it was written as the oracle for the shortcut and is a perfectly good regression pin without it. Signed-off-by: kurok <22548029+kurok@users.noreply.github.com>
198e8fb to
696c590
Compare
1 participant
Closes #230 — with one deliberate omission, measured. See What I took back out.
Branched from
master(5027ca5).What changed
get_body_encoded()re-reads a part's headers to find its transfer encoding, and both the full and lazy parsers called it two or three times per part — for the quoted-printable escape check, for the encoded size, and again insideget_body_raw. It is now called once and threaded to all three.Two copies went with it, and neither bought anything:
get_body_rawcopied it into aVeconly sodecode_charsetcould borrow it straight back.encoding_rsby reference — which validates UTF-8 and hands back aCow::Borrowed— andinto_owned()then copied the whole body a second time.So text bodies now decode from the borrowed slice where the encoding is already plaintext, and give the decoded
VectoString::from_utf8where it is not.Why the shortcut is safe
The two cases where
String::from_utf8is not equivalent toencoding_rsare routed to the original path: a BOM, whichencoding_rsstrips andfrom_utf8would keep as U+FEFF, and invalid sequences, whichencoding_rsreplaces with U+FFFD andfrom_utf8rejects outright. The resolved charset is compared, never the raw label —Charset::for_labelmapsutf8,UTF-8andunicode-1-1-utf-8onto one charset.tests/test_charset_decoding.pypins all of it: BOMs across four transfer encodings, invalid UTF-8, four spellings of the label, a non-UTF-8 charset, and an unknown one that must still warn.All sixteen pass against master as well. That is the point — they are the oracle for the claim that the shortcut changed nothing, not a test of the shortcut.
What I took back out
The issue also asks for the escape scan to use
memchr_iter. I implemented it, benchmarked it, and removed it.The new
parse_qp_dense_escapesbenchmark — one=every three bytes, the scan's worst case — came back 10.1% slower withmemchr, against a 1.2% control floor.memchr's per-call setup never amortises when hits are three bytes apart, while the byte loop's test is a single comparison. Reverting that hunk alone moved it to −1.7% and left every other gain in place, which is how I know it was thememchrand not code placement:memchrparse_qp_dense_escapesparse_many_small_serialparse_qp_messageThe dependency declaration came out with it, so
Cargo.toml,fuzz/Cargo.tomlandCargo.lockare unchanged from master.(The
parse_qp_messagefigure moved a lot between those two runs — 12.5% against 2.4% — which given this repo's current ±15% placement scatter I would not read as meaning anything. The two numbers I do trust are the dense-escape one, because it has a mechanism and reverting one hunk moved it back, andparse_many_small_serial, because it is consistent across both runs.)Numbers
Apple M4 (10 vCPU), 5 interleaved rounds, pure-Python controls within 0.5%:
parse_many_small_serialparse_8bit_text(new)parse_qp_messageparse_qp_dense_escapes(new)Three benchmarks are added: a plain 8bit body (almost entirely the removed copy plus charset validation), a base64 UTF-8 body (the owned-decode path), and the dense-escape worst case that caught the
memchrregression.Checks run locally
pytest tests --ignore=tests/benchmark(843 passed, 3 skipped), the full benchmark suite (32 passed),cargo fmt --check,cargo clippy --all-targets -- -D warnings -W clippy::cast_possible_truncation,mypy --strict,ruff check ..git diff --stat origin/master -- vendor/ Cargo.lockis empty. Attachmentcontentis byte-identical —body_to_vecisget_body_raw's match verbatim.