Conversation
Step 1 shared the rule -- classify_part, envelope, part_identity. This shares the loop the rule is applied in. Full and lazy mode ran two copies of the same walk: one get_body_encoded() per part threaded to everything that needs it, the quoted-printable escape check, the per-channel warning indices, and the text/plain vs text/html dispatch. They differed only in what an attachment is made of. That difference is now a PartSink trait with two implementations, and the walk is flat_parts::<A>. Counted: warn_charset goes from four call sites to one, warn_transfer_decode from six to two, decode_body from three to one. Why this is worth the risk of touching the hot path: the two modes are REQUIRED to report the identical warning list, including each warning's part_path index, because that is what lets strict=True mean one thing in both. Until now that was two hand-maintained copies that had to agree, with a fuzz invariant to notice when they stopped. A shared loop cannot disagree with itself. Two deviations from the issue's sketch, both deliberate. It proposes replacing Mail and LazyMail with one FlatMail<A>. LazyMail grew a `repaired` field in #239 that full mode has no use for, so one struct would give Mail a field that is always None -- and the loop, not the struct, is where the duplication that can go wrong lives. Keeping both shapes also means the binding layer and the fuzz targets need no edits: the diff is the core, the changelog and three new tests. It also proposes an associated type for what a sink needs beyond the part. Lazy mode needs the buffer being parsed, to record offsets into it (#239), and that associated type would carry a lifetime. One unused parameter in the full-mode impl is cheaper than a GAT. Verification of the part most at risk: a message crafted to trip every warning kind, with two entries in each of text_plain and text_html so the indices have to count per channel rather than per part, renders a byte-identical seven-warning list before and after, in both modes. That probe is now three tests rather than a scratch script. Measured flat: local interleaved A/B on an M4, 3 rounds, worst +0.9% (parse_lazy_all_attachments) against a 2.8% control noise floor, verdict "no significant difference". 867 Python tests, 13 core tests, 236K fuzz executions with debug assertions live, clippy, fmt, mypy --strict, ruff and both repo invariant scripts.
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.
Completes #234 (step 2; step 1 is #260).
Step 1 shared the rule —
classify_part,envelope,part_identity. This shares the loop the rule is applied in. Full and lazy mode ran two copies of the same walk — oneget_body_encoded()per part threaded to everything that needs it, the quoted-printable escape check, the per-channel warning indices, thetext/plainvstext/htmldispatch — differing only in what an attachment is made of.warn_charsetwarn_transfer_decodedecode_bodyWhy this one is worth touching the hot path for
The two modes are required to report the identical warning list, each warning's
part_pathindex included, because that is what letsstrict=Truemean one thing in both. Until now that was two hand-maintained copies that had to agree, with a fuzz invariant to notice when they stopped. A shared loop cannot disagree with itself.Two deviations from the issue's sketch, both deliberate
No
FlatMail<A>. The issue proposes replacingMailandLazyMailwith one generic struct.LazyMailgrew arepairedfield in #239 that full mode has no use for, so one struct would giveMaila field that is alwaysNone— and the loop, not the struct, is where the duplication that can go wrong lives. Keeping both shapes also means the binding layer and the fuzz targets need no edits at all: the diff is the core, the changelog and three new tests.No associated type on
PartSink. Lazy mode needs the buffer being parsed to record offsets into it (#239); an associated type for that would carry a lifetime. One unused parameter in the full-mode impl is cheaper than a GAT.flat_parts::<A>is not#[inline(never)]and has exactly one call site per instantiation — the property recorded onMail::from_payload, where a second call site cost the flat path 28%.Verifying the part most at risk
A message crafted to trip every warning kind, with two entries in each of
text_plainandtext_htmlso the indices have to count per channel rather than per part:Byte-identical before and after, in both modes — envelope warnings first in header order, then parts in visit order, each naming the slot its content landed in. That probe is now three tests (
test_parse_warnings.py) rather than a scratch script.Measurements
Local interleaved A/B on an M4, 3 rounds, against this PR's base:
parse_messageparse_message_strictparse_lazy_untouchedparse_lazy_all_attachmentsparse_manyparse_treeNoise floor 2.8%; worst treatment delta +0.9%; verdict no significant difference.
parse_messagereads 2.7% faster but that is inside the floor, so I am not claiming it — flat is the claim.Verification: 867 Python tests (3 new); 13 core Rust tests; 236K
cargo fuzz run -a parse_agreementexecutions, zero crashes — invariants 3 and 7 are exactly the drift this removes; clippy on all three crates, fmt,mypy --strict, ruff,check_vendored_mailparse.sh,check_bench_lockfile.py.__init__.pyi,tests/test_contract.py,docs/,vendor/andsrc/are untouched.