Cache the headers dict per object, collapse the six getters, and freeze the result classes (#231) - #246
Conversation
848b681 to
17d0f3e
Compare
|
Gate status, and why I am not patching it here. This PR was green at 15/15 before I rebased it onto Nothing here can reach quoted-printable decoding. The same thing happened to #247 on the same rebase — a pure thread-scheduler change, which came back +19.0% on So this is code placement, not this diff, and #249 is the instrument for it: it builds the same source K times differing only in a layout salt and reports the per-benchmark spread across salts. If I would rather leave this failing and explained than make it pass with a guess. Happy to rebase again once #249 has been dispatched and there is a measured number to judge it against. |
Six result classes expose `headers` -- PyMail, PyMailMetadata, PyLazyMail, PyMimePart, PyMimePartMetadata, PyLazyMimePart -- and each rebuilt a whole Python dict on every read: one PyDict, a str per key, a list per key, a str per value. For tests/data/valid_message.eml that is about ninety objects per read, on an attribute Python callers reasonably treat as stored and read several times. The README's own idiom reads it twice per message. All six now share one Headers type holding the pairs and a OnceLock<Py<PyDict>>, so the first read builds the dict and every later read returns that same object. Same pattern, and the same race rationale, as PyLazyAttachment.content: the dict is built outside get_or_init because set_item can raise and a closure producing a value cannot propagate that, so a race builds two and publishes one. The cell is filled on first access and never in a constructor, so a parse that never reads headers -- a metadata sweep for one field, say -- pays nothing for it. This also leaves one copy of the getter body where there were six byte-identical ones, which is what made #157 a six-place change. Signed-off-by: kurok <22548029+kurok@users.noreply.github.com>
None of the eleven #[pyclass]es has a &mut self method or a #[pyo3(set)] field, and the module docs already claim the results are immutable. Without frozen, PyO3 still runs its borrow flag on every getter call -- an AcqRel compare-exchange loop on entry and a fetch_sub on release -- to guard mutation that cannot happen. Frozen classes use the no-op checker instead. Every field is Sync (String, Vec<u8>, Option<_>, Vec<Py<_>>, and the OnceLock<Py<_>> caches), so this compiles as written. Kept separate from the headers cache in the previous commit so that a code-layout swing in the A/B can be attributed to one or the other -- this repository has been bitten three times by placement effects that read as regressions (#120, #204, and again while rebasing #228). Signed-off-by: kurok <22548029+kurok@users.noreply.github.com>
…ange (#231) The identity contract is new, so it is tested rather than assumed: headers is the same dict on every read for all six classes that expose it, eight racing first readers all observe the one dict the cell published, and it is still a plain dict[str, list[str]] rather than a proxy. The first three of those fail against master and pass here. Also pinned is the consequence of sharing, which is a documented behaviour change: edits to the returned dict persist across later reads of the same object, while the parse behind it does not move and a fresh parse is unaffected. docs/migrating.md called headers 'a plain dict snapshot'; it now says to treat it as read-only and to copy it if you need one you can edit, and the stub and Rust doc comments say the same. Two benchmarks, because nothing in the suite read headers inside a timed call and so none of this was visible. headers_repeat_read does the README's own idiom -- three lookups on an already-parsed message -- and headers_first_read is its control: the dict build has not gone away, it has moved to first access, so that one pays exactly one build either way. Apple M4 (10 vCPU), 5 interleaved rounds, controls within 2.0%: headers_repeat_read 2.416 -> 0.073 us 33x attachment_reread 0.083 -> 0.042 us 2x, from frozen alone Signed-off-by: kurok <22548029+kurok@users.noreply.github.com>
17d0f3e to
14439b8
Compare
1 participant
Closes #231.
Branched from
master(e7bbb05). Independent of #245.Three commits, deliberately: the headers cache, then
frozen, then tests/benchmarks/docs. The issue asks forfrozento be separable so a code-layout swing can be attributed to one change or the other — which, given this repo's history, turned out to be the right instinct.What changed
Six classes expose
headers—PyMail,PyMailMetadata,PyLazyMail,PyMimePart,PyMimePartMetadata,PyLazyMimePart— and each rebuilt a whole Pythondicton every read: onePyDict, astrper key, alistper key, astrper value. About ninety objects forvalid_message.eml, on an attribute Python callers reasonably treat as stored. The README's own idiom reads it twice per message.All six now share one
Headerstype holding the pairs plus aOnceLock<Py<PyDict>>— the same publish-once pattern asPyLazyAttachment.content. The dict is built on first access and never in a constructor, so a parse that never readsheaderspays nothing. The six byte-identical getter bodies collapse to one, which is what made #157 a six-place change.Separately, the eleven
#[pyclass]es are nowfrozen. None has a&mut selfmethod or a#[pyo3(set)]field, so PyO3 was running anAcqRelcompare-exchange borrow flag on every getter call, plus afetch_subon release, to guard mutation that cannot happen.Numbers
Apple M4 (10 vCPU), rustc 1.98.0, CPython 3.12, 5 interleaved rounds. Pure-Python controls within 2.0%.
headers_repeat_read(new)headers_first_read(new control)attachment_rereadfrozenaloneheaders_repeat_readis the README idiom — three lookups on an already-parsed message. 33x, against the issue's ≥5x bar and ~10x estimate.attachment_rereadhalving is thefrozencommit on its own: that benchmark is threea.contentreads on shared objects after #227, so with the dict rebuild gone what is left is almost entirely PyO3's borrow-flag atomics. It is a small absolute number but a clean read on whatfrozenbuys.Two readings in that table I am not claiming as wins.
headers_first_read(+22.8%) and every metadata/lazy-untouched benchmark (+16% to +24%) are the M4 code-placement artifact I documented on #244 — master sits at 0.037 ms onparse_metadatawhere it was 0.030 ms before base64-simd was linked, and touching this file happens to shift the layout back. The x86 gate measured those same benchmarks flat on both #244 and #245. The issue predictsheaders_first_readshould be flat, and on x86 I expect it to be. The gate is the number to quote for those rows; the 33x is the claim this PR makes.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___headers_first_readtest__fast_mail_parser___headers_repeat_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_smalltest__threaded___threadpool_parse_emailtest__threaded___threadpool_parse_email_smallThe behaviour change, and why it is documented rather than hidden
mail.headers is mail.headersis nowTrue, so edits to the returned dict persist across later reads of that object. The parse behind it does not move:subject,from_and the rest are unaffected, and re-parsing the payload gives the original headers back.docs/migrating.mdpreviously said "headersis a plain dict snapshot; changing it changes nothing". That sentence is now false in its second half, so it is rewritten to say to treatheadersas read-only and to copy it (dict(mail.headers)) if you need one you can edit. The stub docstrings and the Rust doc comment say the same.No test pinned the old snapshot semantics (
grep -rn 'headers\[.*\] *=' tests/was empty), andtests/test_contract.pyonly requiresisinstance(headers, dict)withstrkeys andlist[str]values — all still true. If you consider the old sentence a contract rather than an incidental property, this wants thebreaking-changelabel and a minor bump; say so and I will add it.Tests
Added to
tests/test_headers.py, and I ran them against master first — the first three fail there and pass here, which is what makes them worth having:test__headers_is_the_same_dict_on_every_read— all six classes, via full/metadata/lazy and the tree root plus first child.test__headers_is_built_on_first_read_and_shared_across_threads— eight barrier-synchronised first readers, oneid().test__mutating_the_headers_dict_does_not_change_the_parse— pins the new semantics in both directions.test__headers_is_still_a_plain_dict_of_str_to_listandtest__headers_agree_across_modes— invariants that pass either way.tests/test_contract.pyattribute sets are untouched: the new field carries no#[pyo3(get)].Checks run locally
pytest tests --ignore=tests/benchmark(818 passed, 3 skipped),cargo fmt --all --check,cargo clippy --all-targets -- -D warnings -W clippy::cast_possible_truncation,mypy --strict fast_mail_parser/,ruff check .— all green. Clippy earned its keep here: it rejected apairs()accessor I had added speculatively and nothing used.No
vendor/change, so noPATCH.mdand no vendoredcargo test.src/mail_parser.rsuntouched — its module doc forbids interior mutability in the core and places caches "on the Python object ... in the binding layer", which is where this one is.