Conversation
…phase 1) date_parsed had three byte-identical copies, children three, the decode-and-cache body two, the strict gate four, and the parse_many result loop three. They are now src/convert.rs. The file already stated the rule, on resolve_workers: "Out of line and shared by all three modes rather than written three times: the message is part of the API, and three copies of it are three chances for them to stop agreeing." The strict rejection's text and what raise_on_error=False puts in a failed slot are part of the API in exactly that sense. Not included: headers_dict. The issue lists six byte-identical headers getters, but that evidence predates #231, which replaced them with a cached Headers::to_dict -- so all six are already one-line delegations and the dict is memoised as well as shared. decode_into keeps the #[cold] #[inline(never)] both copies carried, so the cached fast path in the getters above it is undisturbed, and children_list is a tiny non-recursive generic: the #99 incident was a generic wrapping the whole parse body, which is not this shape. Behaviour is unchanged, including which errors propagate out of a batch rather than landing in a slot: Py::new's fallibility stays outside the outcome, exactly where it was.
The crate root was the 2000-line binding file, and `mod convert;` from phase 1 was its only module boundary. A change to one mode could not be reviewed without loading the other seventeen hundred lines -- which is backwards for a library whose reason to exist is a small fast core with a thin wrapper around it. Eight files, all directly under src/ because the sdist include glob is `src/*` and is one level deep: errors.rs the four exception types, to_py_err, the panic backstop payload.rs Payload, payload_to_bytes, Pinned, RetainedBytes convert.rs Headers, addresses, and phase 1's shared helpers metadata.rs PyAttachmentMetadata, PyMailMetadata flat.rs ParseWarning, PyAddress, PyAttachment, PyMail lazy.rs PyLazyAttachment, PyLazyMail tree.rs the three node types, metadata_node, lazy_node api.rs the three pyfunctions and their mode dispatch Code was moved, not edited. Visibility widened from private to pub(crate) only where a moved item is read across files, never by #[allow(dead_code)]. Every #[inline(never)], #[cold] and #[inline(always)] stayed on the item it was on -- 21, 5 and 1, the same counts as the commit before this one, which is the check that matters most here: the #99 incident was one of those being lost. The sdist was built locally and confirmed to carry all eight modules, and CI's sdist manifest check now names them rather than trusting the glob, so a glob that stopped matching fails by naming the missing file instead of surfacing as a compile error later in the job. Phase 3 -- splitting the core crate the same way -- is explicitly out of scope in the issue and is not here.
The issue said the attributes move onto decode_into AND the two thin wrappers keep theirs, so the cached fast path in the getters above them is undisturbed. I dropped them from the wrappers in phase 1. Their own doc comments still said "#[cold] and out of line", which is how it was caught. Losing an #[inline(never)] is the documented 24% failure mode in this crate (#99), so this is not a cosmetic restoration. Attribute counts are now 23 inline(never) and 7 cold against the pre-#233 baseline's 22 and 6: +1 each, which is decode_into itself.
The gate caught a real regression, and it was mineThe benchmark gate failed twice on My first instinct was wrong and I want to record it, because it is the instinct this repo's history encourages. This PR is binding-only: So I measured it instead of asserting it, with the instrument #240 built for this.
The actual cause. The issue specifies that the My attribute check had missed it because I compared the commit before the split against the commit after — which correctly proved the move changed nothing, and silently inherited phase 1's loss. Against the real baseline the counts were 21/5 where they should have been 23/7. After restoring them, on the same CPU class as both failures:
Same runner class, same base value, treatment down from 0.267 to 0.252. That is causal, not a luckier draw — the first passing run was on a different CPU and I did not want to rest on it, so I re-ran until one landed on 7763. Worst treatment delta is now +3.5% ( Two things worth keeping from this: the gate earned its keep here — an organisational refactor was quietly costing 8% on a decode path — and "it's layout" is a hypothesis that this repo now has a tool to test rather than a conclusion to reach for. |
1 participant
Closes #233 (phases 1 and 2; phase 3 is out of scope in the issue itself).
Phase 1 — the dedupe
date_parsedhad three byte-identical copies,childrenthree, the decode-and-cache body two, the strict gate four, and theparse_manyresult loop three. They are nowsrc/convert.rs.The file already stated the rule, on
resolve_workers: "Out of line and shared by all three modes rather than written three times: the message is part of the API, and three copies of it are three chances for them to stop agreeing." The strict rejection's text and whatraise_on_error=Falseputs in a failed slot are part of the API in exactly that sense.One item on the issue's list is not here:
headers_dict. The issue cites six byte-identical five-lineheadersgetters — but that evidence predates #231, which replaced them with a cachedHeaders::to_dict. All six are already one-line delegations, and the dict is memoised as well as shared, which is strictly better than what the issue asked for. Nothing to do.decode_intokeeps the#[cold] #[inline(never)]both copies carried, so the cached fast path in the getters above it is undisturbed, andchildren_listis a tiny non-recursive generic — the #99 incident was a generic wrapping the whole parse body, which is not this shape. Behaviour is unchanged including which errors propagate out of a batch rather than landing in a slot:Py::new's fallibility stays outside the outcome, exactly where it was.Phase 2 — the split
errors.rsto_py_err,strict_rejection, the panic backstoppayload.rsPayload,payload_to_bytes,Pinned,RetainedBytesconvert.rsHeaders,addresses, phase 1's helpersmetadata.rsPyAttachmentMetadata,PyMailMetadataflat.rsParseWarning,PyAddress,PyAttachment,PyMaillazy.rsPyLazyAttachment,PyLazyMailtree.rsmetadata_node,lazy_nodeapi.rs#[pyfunction]s and their mode dispatch2000 lines → an 85-line root that is the module list and the
#[pymodule]. All eight files sit directly undersrc/, because the sdist include glob issrc/*and is one level deep.Code was moved, not edited. Visibility widened from private to
pub(crate)only where a moved item is read across files, never with#[allow(dead_code)].The checks that matter for a pure move
Attributes survived — after a correction. Final counts are
#[inline(never)]23,#[cold]7,#[inline(always)]1, against a pre-#233 baseline of 22, 6 and 1. The+1on each isdecode_intoitself, which is what the issue asked for: the attributes move onto the shared helper and the two thin wrappers keep theirs.I got that wrong first time round. Phase 1 dropped
#[cold] #[inline(never)]from bothdecodewrappers, and my original check compared the commit before the split against the commit after — which verified the move changed nothing but silently accepted phase 1's loss. The third commit restores them. See the comment below for how the gate caught it; the lesson is that the baseline for an attribute count is the commit before the whole PR, not before its last step.The sdist really carries them. Built locally with
python -m build --sdistand listed: all eight modules present. CI's sdist manifest check now names them rather than trusting the glob, so a glob that stopped matching fails by naming the missing file instead of surfacing as a confusing compile error later in that job.Nothing public moved.
__init__.pyi,tests/test_contract.py's frozen sets,docs/andvendor/are untouched; 867 Python tests pass, includingtest_contract.py,test_stub_matches_runtime.py, the #157 header-order tests andtest_stdlib_parity.pywith noDIVERGENCESedit.Measurements
Local interleaved A/B on an M4, 3 rounds, against this PR's base:
parse_messageparse_metadataparse_treeparse_lazy_untouchedparse_manyparse_qp_messageNoise floor 2.2%; worst real movement +1.2%.
ab_median.pyreportsattachment_rereadat "+97.6%" in this PR's favour — ignore it, as on #259: that benchmark is 42–83 ns and both sides sample the same two timer ticks, so the median flips between them. It is quantisation, not a measurement.Verification: 867 Python tests;
cargo clippy --workspace --all-targets -D warnings -W clippy::cast_possible_truncation;cargo fmt;mypy --strict;ruff; sdist built and inspected.