Stop allocating header keys twice, and test for a header without decoding it (#238) - #248
Conversation
942989b to
3c4a698
Compare
|
Rebased onto The new baseline settles the caveat in the PR body. I wrote that Re-measured, 5 interleaved rounds, pure-Python controls within 2.1%:
The two Worth noting the quoted-printable rows, which did not exist when this branch was first cut: Re-verified after the rebase: 811 passed / 3 skipped, the vendored suite 58 + 25 (both |
|
The gate failed, and it was right to.
Two things make me read that as placement rather than as this change:
So: What I cannot tell you is whether it works. The M4 does not reproduce the regression, so I have no local instrument for this: the branch measures the same before and after the attribute here. This is the principled change and the documented first thing to try, but the gate on x86 is the only thing that can decide it. If it does not clear, the next step is #240's salted layout A/B rather than another guess. Everything else re-verified: 811 passed / 3 skipped, vendored suite 58 + 25, fmt both manifests, clippy, mypy --strict, ruff. |
…ding it (#238) Two of the three changes #238 asks for. The third -- a single-line fast path in the vendored normalize_header -- is not here; see below. collect_headers keyed its position map by a decoded String, so every header allocated its key twice: once for the map and once for the table. Latin-1 decoding is injective, so the raw key bytes are exactly the same equivalence the String gave, case-sensitive grouping included, and both containers are now sized from part.headers.len() instead of growing. disposition_token called get_first_value("Content-Disposition") purely to test presence, normalising a value it dropped on the next line. get_first_header answers the same question with the same case-insensitive first-match semantics and no tokenizer. Neither touches vendor/, so PATCH.md and the vendored suite are unchanged. Apple M4 (10 vCPU), 5 interleaved rounds, controls within 1.7%: parse_many_small_threads1 4.048 -> 3.839 ms -5.2% parse_many_metadata_small_threads1 3.622 -> 3.442 ms -5.0% parse_qp_message_metadata 0.022 -> 0.021 ms -6.0% The two threads=1 batches are new here and are the reading that matters: serial, so no scheduler, and small messages with no attachments, so no decode -- very nearly pure header work. Why the fast path is not in this PR. It was, and it was worth far more -- the same benchmarks read -28% to -34% with it. But the x86 gate failed it three times on parse_qp_message, a benchmark this branch has no mechanism to affect: the same message in metadata mode, which runs the headers and skips the decode, got 6.5% FASTER on every one of those runs. Against an unmoving 0.246 ms base the three binaries measured 0.268, 0.265 and 0.277 -- the second after pinning the now-cold tokenizer path, the third after also pinning qp::decode_robust, which made it worse. The M4 reads that benchmark 3% faster throughout and cannot see any of it. That is the code-layout sensitivity of #120 and #204, and three binaries is enough to establish that guessing at it does not converge. #240 exists to build the instrument -- a salted layout A/B on x86 -- and the fast path should go back through the gate behind that, not behind a fourth guess. These two changes are independent of it, need no vendored edit, and stand on their own. Signed-off-by: kurok <22548029+kurok@users.noreply.github.com>
4a3c866 to
34be2f4
Compare
kurok
changed the title
|
Third gate run made it worse (+12.9%), so I have stopped guessing and rescoped this PR instead. Against an unmoving 0.246 ms base,
On all three runs What this PR is now. The two changes that need no vendored edit:
Controls within 1.7%. What came out, and what should happen to it. The I would rather land 5% that is understood than 30% that fails the gate for a reason I cannot name. |
1 participant
Closes #238.
Branched from
master(e7bbb05). Independent of #245/#246/#247, but it editsvendor/mailparse/PATCH.mdand the root[patch.crates-io]comment, which #245 also touches — see the rebase note.What changed
Every header value went through mailparse's two-stage tokenizer: a
Vecper physical line, an outerVec, a secondVecfrom the whitespace pass, and a resultStringgrown with no capacity hint. About six allocations to hand back the line it was given.That machinery is for folded values and RFC 2047 encoded words, and the common case is neither. On the 767 KiB fixture the root block has 24 headers, 8 continuation lines and exactly one encoded word; every part-level
Content-Type,Content-Transfer-Encoding,Content-Disposition,Content-ID,DateandMIME-Versionis one line with no=?. And the tokenizer is paid more than once per header —collect_headersreads them all, then mailparse re-tokenisesContent-Typeper part,Content-Transfer-Encodingon everyget_body_encoded(),Content-Dispositionon everyget_content_disposition(), and the three flat parsers addSubject,DateandContent-ID. Roughly sixtyget_valuecalls per full parse, ~55 of them for values the fast path answers with oneto_owned().Three changes, no new dependency,
Cargo.lockuntouched, no output change:normalize_header(vendored) returnschars.trim_start().to_owned()when the value has no\n, no\rand no=?; otherwise it delegates to the unchanged tokenizer, kept asnormalize_header_tokens.collect_headerskeys its position map byget_key_raw()instead of a second ownedString, and sizes both containers frompart.headers.len(). Latin-1 decoding is injective, so raw-byte equality is theStringequality it had — case-sensitive grouping included.disposition_tokentests presence withget_first_headerinstead ofget_first_value, which was building and dropping a normalisedStringon the next line.Why the fast path is exactly equivalent
Not "close enough" — the comment and the test both make the argument:
tokenize_headerrunsvalue.lines().map(str::trim_start), so with no\nthat yields this string once (and nothing for"", wheretrim_startis also empty). A value produced byparse_headernever ends in\r(only a non-CR non-LF byte advancesix_value_end), so the line is the whole value.tokenize_header_linewith no=?pushes exactly onemaybe_whitespace(line)token.normalize_header_whitespacepasses a loneTextorWhitespacethrough unchanged.\ris checked anyway — redundant for valuesparse_headerproduces, but it keeps the fast path correct for aMailHeaderbuilt any other way.fast_path_matches_the_tokenizerasserts it regardless, againstnormalize_header_tokensas oracle, over a hand-written edge table (empty values, lone\r,"=?"split across the boundary, leading tabs,"= ?","?=") and a generated corpus at every length to 48 over an alphabet built from the bytes the check looks for.Numbers
Apple M4 (10 vCPU), rustc 1.98.0, CPython 3.12, 5 interleaved rounds. Pure-Python controls within 1.3%.
parse_many_small_threads1(new)parse_many_metadata_small_threads1(new)parse_metadata(767 KiB)parse_message(767 KiB)The two
threads=1batches are the reading that matters, and they are new here (the issue asks for them). Serial, so no scheduler; small messages with no attachments, so no transfer decode — very nearly pure header work, which is exactly what this changes. ~23–24% off both.On the 767 KiB rows, a caveat I want to be explicit about.
parse_metadatareads 0.037 → 0.027 ms, which looks like 36%. It is not: master's 0.037 includes the M4 code-placement artifact documented on #244 — that benchmark was 0.030 ms before base64-simd was linked, and x86 has measured it flat on #244, #245, #246 and #247. Against the artifact-free 0.030 the real gain is ~10%. The x86 gate is the number to quote for that row.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_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_metadata_small_threads1test__threaded___parse_many_smalltest__threaded___parse_many_small_threads1test__threaded___threadpool_parse_emailtest__threaded___threadpool_parse_email_smallNoise floor from the pure-Python controls: 1.3% (they cannot be affected by the build, so this is measurement error).
Checks run locally
pytest tests --ignore=tests/benchmark(811 passed, 3 skipped — includingtest_headers.py,test_multivalue_headers.py,test_stdlib_parity.pywith noDIVERGENCESchange, and the attachment/metadata/lazy/tree suites that exercisedisposition_token), the vendored suite (54 + 25 passed) via--target-diroutside the tree,cargo fmtfor both manifests,cargo clippy --all-targets -- -D warnings -W clippy::cast_possible_truncation,mypy --strict,ruff check .— all green.git diff --stat master -- Cargo.lockis empty.The M4 is not a proxy for x86; the Benchmark quality gate is the verdict.
Rebase note
#245 also edits
vendor/mailparse/PATCH.md(making it "three functions changed" for the quoted-printable decoder) and the root[patch.crates-io]comment. Whichever of the two lands second needs those merged by hand — the conflicts are additive, but the "three functions" counts collide and want reading rather than taking one side. I have done this merge twice already this series and will handle it.