diff --git a/CHANGELOG.md b/CHANGELOG.md index 5849902..24f9ac4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **The base64 whitespace strip in the vendored mailparse is now dependency-free**, and + faster: `vendor/mailparse/src/bytescan.rs` skips any word with no byte below `0x21`, walks + the exact `hasless` mask of one that might, and copies runs in one piece -- plain `std`, + no `unsafe`. One pass where the `memchr` version made two searches per run: full parse + 0.254 -> 0.228 ms and `parse_many` 2.09 -> 1.83 ms on an Apple M4, -2.5 to -2.9% on the + CI gate's EPYC 7763. The MIME boundary search keeps `memchr`: dependency-free versions of + it measured +13-14% (four words per branch) and +9-11% (eight) on the metadata paths on + x86, so it stays. Context: upstream declined the `memchr` change as an added dependency + (staktrace/mailparse#142); a dependency-free version of both loops is offered instead + (staktrace/mailparse#143), and `vendor/mailparse/PATCH.md` records what switching to it + would cost if a release carries it. - **Rust toolchain pin moved from 1.97.1 to 1.98.0** (#120). The pin was a holding action against a measured 15-30% slowdown under 1.98.0; the cause turned out to be the two mailparse loops above -- the compiler had not changed their instructions, diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 37e5634..429ecb4 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -316,17 +316,21 @@ builds at +/-0.2%. The fix replaces that scan with `memchr::memmem` -- vectorised, and laid out independently of this crate -- via the patched copy in `vendor/mailparse` -(a permanent carry: upstream declined the change as an added dependency; -`vendor/mailparse/PATCH.md` has the sync procedure). Metadata mode went +(upstream declined the change as an added dependency and is offered a +dependency-free version instead; `vendor/mailparse/PATCH.md` has the reasoning, +the sync procedure and what switching would cost). Metadata mode went 0.365 -> 0.030 ms and the full parse 1.10 -> 0.76 ms. With that gone, sampling the *full* parse put **77.7%** of what remained in `decode_base64`'s whitespace filter -- `iter().filter().cloned().collect()`, a test and a push per byte -- and the toolchain A/B confirmed it carried the rest of the placement sensitivity: decoding paths still moved +22% on one runner and +5% -on another while the metadata paths had gone flat. Same fix, same place: the -whitespace is found with `memchr` and the runs between are copied whole. The -full parse went 0.83 -> 0.28 ms on top. +on another while the metadata paths had gone flat. Same place, and here the fix +is dependency-free (`vendor/mailparse/src/bytescan.rs`): a word with no byte below +`0x21` is skipped whole, the mask of one that might says which bytes to check, +and the runs between whitespace are copied in one piece. The full parse went +0.83 -> 0.28 ms on top, and 0.25 -> 0.23 again when this one-pass version replaced +the `memchr` two-search one. **What remains true.** The gate has a false-positive mode its noise floor cannot see: the controls are pure Python and do not care how the extension was laid out, diff --git a/vendor/mailparse/PATCH.md b/vendor/mailparse/PATCH.md index 4c39ec0..ccee9d3 100644 --- a/vendor/mailparse/PATCH.md +++ b/vendor/mailparse/PATCH.md @@ -1,39 +1,32 @@ # Patched copy of `mailparse` 0.16.1 This directory is [mailparse 0.16.1](https://crates.io/crates/mailparse/0.16.1) as published, -with **two functions changed** and one dependency added. It is applied through -`[patch.crates-io]` in the root `Cargo.toml` (and `fuzz/Cargo.toml`), so `cargo` sees the -same crate name and version and every other dependency resolves exactly as before. -`memchr = "2.7.0"` was added to this crate's `[dependencies]` (MIT OR Unlicense, no -dependencies of its own). +with **two functions changed** (one via a new module) and one dependency added. It is +applied through `[patch.crates-io]` in the root `Cargo.toml` (and `fuzz/Cargo.toml`), so +`cargo` sees the same crate name and version and every other dependency resolves exactly +as before. `memchr = "2.7.0"` is added to this crate's `[dependencies]` (MIT OR Unlicense, +no dependencies of its own). ## The changes -Both replace a byte-at-a-time loop over the whole message body with a vectorised search. -Both return exactly what the code they replace returned. - -**1. `find_from_u8` in `src/lib.rs`** -- the search `parse_mail` runs for every MIME -boundary -- scanned byte by byte: - -```rust -for i in ix_start..=ix_end { - if line[i] == key[0] { /* compare the rest */ } -} -``` - -It now calls `memchr::memmem::find`. Same result: first occurrence of `key` at or after -`ix_start`, `None` when there is none. - -**2. `decode_base64` in `src/body.rs`** stripped whitespace before decoding with -`body.iter().filter(|c| !c.is_ascii_whitespace()).cloned().collect()`: a test and a -bounds-checked push per byte. It now calls `strip_ascii_whitespace`, which finds -whitespace with `memchr` and copies the runs between whole -- one search and one memcpy -per 76-byte line in the common case. The set of bytes removed is unchanged (exactly -`u8::is_ascii_whitespace`: space, tab, LF, form feed, CR) and a unit test in `body.rs` -checks it against the original filter over every byte value. - -`diff -r` against the registry copy shows exactly `src/lib.rs`, `src/body.rs`, -`Cargo.toml` and this file. +Both replace a byte-at-a-time loop over the whole message body. Both return exactly what +the code they replace returned. + +1. **`find_from_u8` in `src/lib.rs`** -- the search `parse_mail` runs for every MIME + boundary -- scanned byte by byte. It now calls `memchr::memmem::find`. Same result: + first occurrence of `key` at or after `ix_start`, `None` when there is none. +2. **`decode_base64` in `src/body.rs`** stripped whitespace with + `iter().filter(|c| !c.is_ascii_whitespace()).cloned().collect()`: a test and a + bounds-checked push per byte. It now calls `bytescan::strip_ascii_whitespace` + (`src/bytescan.rs`, new, plain `std`, no `unsafe`): a word with no byte below `0x21` + cannot contain whitespace and is skipped whole; for one that might, the exact `hasless` + mask (Anderson, *Bit Twiddling Hacks*) says which bytes to look at, each re-checked with + `is_ascii_whitespace` so `0x0B` and the other control bytes are kept, as before; runs + are copied in one piece. Its tests compare it against the filter it replaces over a + generated corpus at every alignment and over every byte value. + +`diff -r` against the registry copy shows exactly `src/bytescan.rs`, `src/lib.rs` (a `mod` +line and one function), `src/body.rs` (one function), `Cargo.toml` and this file. ## Why @@ -45,35 +38,50 @@ when a loop straddled a 64-byte boundary. A rustc minor version (#120) and a version-string bump (#204) each moved the loops and each read as a regression of up to 96% -- with zero change to the instructions executed. -Measured on this machine (interleaved A/B, Apple M4), original master to both patches: +Interleaved A/B on an Apple M4, original master to this copy: | benchmark | before | after | |---|---|---| -| `parse_email(mode="metadata")` | 0.365 ms | 0.034 ms | -| `parse_email` (full) | 1.094 ms | 0.281 ms | -| `parse_many` (8 x 767 KiB) | 9.082 ms | 2.177 ms | +| `parse_email(mode="metadata")` | 0.365 ms | 0.030 ms | +| `parse_email` (full) | 1.094 ms | 0.228 ms | +| `parse_many` (8 x 767 KiB) | 9.082 ms | 1.834 ms | + +## Why the two functions use different tools + +Upstream declined a version of this change that used `memchr` for both (staktrace/mailparse#142: +no new external dependencies), so a dependency-free word-at-a-time version of both was +written and is what upstream is now offered +([staktrace/mailparse#143](https://github.com/staktrace/mailparse/pull/143)). This copy +takes the half of it that is strictly no slower here: + +- The **strip** is the dependency-free version. It is one pass where the `memchr` version + made two searches per run, and measured faster on both CPUs tried (Apple M4: -9 to -12% + on the decoding paths; EPYC 7763 on the CI gate: -2.5 to -2.9%). +- The **byte search stays on `memchr`**. Three dependency-free variants went through the + gate: four words per branch measured +13-14% on the metadata paths on an EPYC 7763, eight + words (a cache line) +9-11% -- 6 us per 767 KiB. A word-at-a-time scan tops out below a + 32-byte AVX2 compare, and the rule for this copy is no degradation. + +So if a mailparse release ever includes #143, switching this copy's search to it (and +dropping the directory) is a decision that costs about 10% on `mode="metadata"` on x86 and +nothing on the decoding paths. The removal steps for that case: + +1. bump `mailparse` in the root `Cargo.toml` and `fuzz/Cargo.toml` to that release, +2. delete the two `[patch.crates-io]` sections and this directory, +3. drop the `vendor/mailparse/**/*` entry from `[tool.maturin] include` in + `pyproject.toml` and the `vendored mailparse tests` step from the lint job, +4. run the benchmark gate and read the metadata rows with the number above in mind. ## Keeping this in sync -This copy is permanent. The change was proposed upstream -([staktrace/mailparse#142](https://github.com/staktrace/mailparse/pull/142)) and declined on -2026-08-28: the project does not accept pull requests that add external dependencies, -particularly ones relying heavily on unsafe code -- which describes `memchr`'s SIMD paths -exactly. So no future mailparse release will carry these functions, and every upstream -release has to be merged into this directory by hand: - -1. `diff -r` the new release against the previous one (both in - `~/.cargo/registry/src/*/mailparse-/`) and apply that diff to this copy -- - *not* the other way round, or the two functions revert. -2. Keep `find_from_u8`, `strip_ascii_whitespace` and the `strip_ascii_whitespace_tests` - module as they are here, and `memchr` in `Cargo.toml`. -3. Bump the version in this copy's `Cargo.toml` and the `mailparse = "..."` requirement in - the root `Cargo.toml` and `fuzz/Cargo.toml` together, since `[patch]` only applies when - the patched version satisfies the requirement. -4. Run this copy's own suite (the lint job does: `cargo test --manifest-path - vendor/mailparse/Cargo.toml`), then the benchmark gate. The numbers should not move. - -If upstreaming is ever wanted, the shape that could be accepted is a dependency-free one: -a word-at-a-time (SWAR) scan in plain `std`, which is what `core`'s own `memchr` does -internally. It would be slower than `memchr`'s SIMD paths and would need measuring against -this copy before replacing it. +Until then, each upstream mailparse release is a hand-merge into this copy: + +1. `diff -r` the new release against the previous one (both under + `~/.cargo/registry/src/*/mailparse-/`) and apply that diff here -- not the + other way round, or the two functions revert; +2. keep `src/bytescan.rs`, the `mod bytescan;` line, the two call sites and `memchr` in + `Cargo.toml`; +3. bump the version in this copy's `Cargo.toml` and the `mailparse = "..."` requirement in + both root manifests together, since `[patch]` only applies when the patched version + satisfies the requirement; +4. run this copy's own suite (the lint job does), then the benchmark gate. diff --git a/vendor/mailparse/src/body.rs b/vendor/mailparse/src/body.rs index 2c51f29..60a7d8f 100644 --- a/vendor/mailparse/src/body.rs +++ b/vendor/mailparse/src/body.rs @@ -137,36 +137,10 @@ impl<'a> BinaryBody<'a> { } fn decode_base64(body: &[u8]) -> Result, MailParseError> { - let cleaned = strip_ascii_whitespace(body); + let cleaned = crate::bytescan::strip_ascii_whitespace(body); Ok(data_encoding::BASE64_MIME_PERMISSIVE.decode(&cleaned)?) } -/// Copy `body` without its ASCII whitespace -- the same bytes `u8::is_ascii_whitespace` -/// names: space, tab, newline, form feed, carriage return. -/// -/// Whitespace is located with a vectorised search and the runs between are copied -/// whole, rather than testing and pushing one byte at a time. A base64 body is -/// almost entirely 76-byte lines ending in CRLF, so the common case is one search -/// and one copy per line. Tabs and form feeds are rare enough that they get a second -/// search over each run instead of a place in the first. -fn strip_ascii_whitespace(body: &[u8]) -> Vec { - let mut cleaned = Vec::with_capacity(body.len()); - let mut rest = body; - loop { - let end = memchr::memchr3(b'\r', b'\n', b' ', rest).unwrap_or(rest.len()); - let mut run = &rest[..end]; - while let Some(j) = memchr::memchr2(b'\t', 0x0c, run) { - cleaned.extend_from_slice(&run[..j]); - run = &run[j + 1..]; - } - cleaned.extend_from_slice(run); - if end == rest.len() { - return cleaned; - } - rest = &rest[end + 1..]; - } -} - fn decode_quoted_printable(body: &[u8]) -> Result, MailParseError> { Ok(quoted_printable::decode( body, @@ -183,47 +157,3 @@ fn get_body_as_string(body: &[u8], ctype: &ParsedContentType) -> Result Vec { - body.iter() - .filter(|c| !c.is_ascii_whitespace()) - .cloned() - .collect() - } - - #[test] - fn matches_the_filter_it_replaces() { - let cases: &[&[u8]] = &[ - b"", - b" ", - b" \t\r\n\x0c", - b"abc", - b" abc ", - b"ab cd\tef\ngh\rij\x0ckl", - b"\x0b", // vertical tab is NOT ascii whitespace; must be kept - b"a\x0bb", - b"\t\tab\x0c\x0ccd", - b"QUJD\r\nREVG\r\n", - b"QUJD REVG\tR0hJ\x0cSktM", - ]; - for case in cases { - assert_eq!(strip_ascii_whitespace(case), reference(case), "{:?}", case); - } - // every byte value, alone and next to whitespace - for b in 0u8..=255 { - let single = [b]; - assert_eq!( - strip_ascii_whitespace(&single), - reference(&single), - "{:?}", - b - ); - let mixed = [b' ', b, b'\t', b, b'\r', b'\n', b]; - assert_eq!(strip_ascii_whitespace(&mixed), reference(&mixed), "{:?}", b); - } - } -} diff --git a/vendor/mailparse/src/bytescan.rs b/vendor/mailparse/src/bytescan.rs new file mode 100644 index 0000000..87f2fc6 --- /dev/null +++ b/vendor/mailparse/src/bytescan.rs @@ -0,0 +1,153 @@ +//! Byte scans that look at a machine word at a time instead of a byte at a time. +//! +//! The whitespace strip before base64 decoding used to test every byte of the body on +//! its own. (In this copy the MIME boundary search uses `memchr` instead -- see +//! `find_from_u8` and PATCH.md; upstream is offered a word-at-a-time version of both.) On a message with large attachments they are most of the parse, +//! and a byte-at-a-time loop's speed also depends on where the linker happens to +//! place it -- the same instructions ran at half speed on x86-64 when the loop +//! straddled a 64-byte boundary. +//! +//! These versions read `usize`-sized chunks with `chunks_exact` and +//! `from_ne_bytes` (plain loads; no `unsafe`) and use two classic word tricks to +//! decide whether a chunk needs a closer look: +//! +//! - `below_mask(x, n)`: non-zero iff some byte of `x` is below `n`, exact for `n <= 128`. +//! +//! Where a mask is non-zero, its set bits say which bytes to look at, so a hit costs a +//! `trailing_zeros` rather than a rescan of the word. +//! +//! Both are exact, so a chunk is only examined byte by byte when it really contains +//! a candidate. See Anderson, "Bit Twiddling Hacks", `haszero` and `hasless`. + +// `chunks_exact` with a constant size is what clippy would rewrite to `as_chunks`, which +// is only stable from Rust 1.88 -- above this crate's minimum supported version. +#![allow(clippy::chunks_exact_to_as_chunks)] + +const WORD: usize = core::mem::size_of::(); +const LO: usize = usize::from_ne_bytes([0x01; WORD]); +const HI: usize = usize::from_ne_bytes([0x80; WORD]); + +/// Each byte of the result has its high bit set iff that byte of `x` is below `n` +/// (`n <= 128`), with the same caveat as `zero_byte_mask` above the lowest hit: never a +/// false negative, possibly a false positive, so callers re-check. +#[inline] +fn below_mask(x: usize, n: u8) -> usize { + x.wrapping_sub(LO.wrapping_mul(n as usize)) & !x & HI +} + +/// The word at `chunk`, byte 0 in the low-order bits, so that a mask bit at position +/// `p` refers to byte `p / 8` regardless of the machine's endianness. +#[inline] +fn word(chunk: &[u8]) -> usize { + // Callers pass exactly `WORD` bytes. Spelled with `copy_from_slice` rather than + // `try_into` so it reads the same in every edition; it compiles to one load. + let mut bytes = [0u8; WORD]; + bytes.copy_from_slice(chunk); + usize::from_le_bytes(bytes) +} + +/// Byte index of the lowest set bit of a non-zero mask. +#[inline] +fn first_hit(mask: usize) -> usize { + (mask.trailing_zeros() / 8) as usize +} + +/// `body` without its ASCII whitespace -- exactly the bytes `u8::is_ascii_whitespace` +/// names: space, tab, line feed, form feed, carriage return. +/// +/// Every whitespace byte is below `0x21`, so a word with no byte below `0x21` has no +/// whitespace and is skipped whole. For a word that has candidates, the mask says +/// where they are, and each is re-checked with `is_ascii_whitespace`, so `0x0B` and +/// the other control characters below `0x21` are kept, as before. Runs of kept bytes +/// are copied in one piece rather than pushed one at a time. A base64 body is mostly +/// 76-byte lines ending in CRLF, so the common case is two mask bits and one copy per +/// line. +pub(crate) fn strip_ascii_whitespace(body: &[u8]) -> Vec { + let mut cleaned = Vec::with_capacity(body.len()); + let mut run_start = 0; + let mut offset = 0; + let mut words = body.chunks_exact(WORD); + for chunk in &mut words { + let mut mask = below_mask(word(chunk), 0x21); + while mask != 0 { + let i = offset + first_hit(mask); + if body[i].is_ascii_whitespace() { + cleaned.extend_from_slice(&body[run_start..i]); + run_start = i + 1; + } + mask &= mask - 1; + } + offset += WORD; + } + for (i, &b) in words.remainder().iter().enumerate() { + if b.is_ascii_whitespace() { + cleaned.extend_from_slice(&body[run_start..offset + i]); + run_start = offset + i + 1; + } + } + cleaned.extend_from_slice(&body[run_start..]); + cleaned +} + +#[cfg(test)] +mod tests { + use super::strip_ascii_whitespace; + + fn naive_strip(body: &[u8]) -> Vec { + body.iter() + .filter(|c| !c.is_ascii_whitespace()) + .cloned() + .collect() + } + + /// Deterministic pseudo-random bytes drawn from a small alphabet, so that needles + /// actually occur, at every alignment relative to the word size. + fn corpus() -> Vec> { + let mut out = Vec::new(); + let mut state: u32 = 0x9E37_79B9; + for len in 0..80 { + for _ in 0..4 { + let mut v = Vec::with_capacity(len); + for _ in 0..len { + state ^= state << 13; + state ^= state >> 17; + state ^= state << 5; + const ALPHABET: &[u8; 12] = b"-\r\n \tab=\x0c\x0b\x00\xff"; + v.push(ALPHABET[(state % 12) as usize]); + } + out.push(v); + } + } + out + } + + #[test] + fn strip_matches_filter() { + for h in corpus() { + assert_eq!(strip_ascii_whitespace(&h), naive_strip(&h), "{:?}", h); + } + for b in 0u8..=255 { + let single = [b]; + assert_eq!( + strip_ascii_whitespace(&single), + naive_strip(&single), + "{:?}", + b + ); + let mixed = [ + b' ', b, b'\t', b, b'\r', b'\n', b, b, b, b, b, b, b, b, b, b, b'\x0c', b, + ]; + assert_eq!( + strip_ascii_whitespace(&mixed), + naive_strip(&mixed), + "{:?}", + b + ); + } + // vertical tab is below 0x21 but is not ASCII whitespace: kept + assert_eq!( + strip_ascii_whitespace(b"a\x0bb\x0b\x0b\x0b\x0b\x0b\x0bc"), + b"a\x0bb\x0b\x0b\x0b\x0b\x0b\x0bc" + ); + } +} diff --git a/vendor/mailparse/src/lib.rs b/vendor/mailparse/src/lib.rs index 4203e21..3d02258 100644 --- a/vendor/mailparse/src/lib.rs +++ b/vendor/mailparse/src/lib.rs @@ -13,6 +13,7 @@ use charset::{decode_latin1, Charset}; mod addrparse; pub mod body; +mod bytescan; mod dateparse; mod header; pub mod headers; @@ -121,6 +122,10 @@ pub(crate) fn find_from(line: &str, ix_start: usize, key: &str) -> Option fn find_from_u8(line: &[u8], ix_start: usize, key: &[u8]) -> Option { assert!(!key.is_empty()); assert!(ix_start <= line.len()); + // `memchr` rather than `bytescan::find`: on x86-64 its AVX2 path is ~10% ahead of a + // word-at-a-time scan on the structure-only parse (measured on the CI gate), and this + // copy's rule is no degradation. The dependency-free `bytescan` version of this search + // is what upstream is offered; see PATCH.md. memchr::memmem::find(&line[ix_start..], key).map(|v| ix_start + v) }