867e1fe259
Benchmark-gated (benches/ROUND20.md), same rule as rounds 2-19: every change ships with a BEFORE/AFTER counting-allocator micro-benchmark and a byte-value equivalence gate; a non-winning AFTER is rolled back (never applied). The rollback rule is encoded in the harness (GATE FAIL exit). All 8 sections pass. Reproduce: cargo run --release --features bench --example bench_round20_micro - A1 CalendarEvent iCal parse: replace the throwaway per-property HashMap<String,Vec<String>> (DTSTART/DTEND/RECURRENCE-ID) with a direct VALUE=DATE scan; prop_with_params kept #[cfg(test)] (6->2 allocs/event, 4.2x) - A2 UserDto::from: add User::into_parts and MOVE image (<=512 KiB data URI) + ui_preferences JSON instead of cloning on every /api/auth/me (27->14 allocs) - A3 parse_vcard: drop the per-line to_ascii_uppercase copy + the lines Vec; promote ascii_ci_contains to common::text and share it (8->1 allocs/contact) - A4 Calendar/AddressBook DTO: into_parts move incl. custom_properties map (18->10) - I1 file-listing repos: collect::<Result<Vec>>() size-hints to 0 and grows from capacity 0; pre-size with Vec::with_capacity (8->1 container reallocs, 4 sites) - I4 plaintext_stream: lazy emit iterator instead of eager Vec collect (43x wall) - C1 NC write_etag_element: borrowed pre-escaped quote events, no owned quoted String/escape re-alloc; byte-identical output (3->0 allocs/PROPFIND row) - C3 NC favorites REPORT: map.remove() move instead of get().clone() (~7 allocs/fav) Deferred (documented in ROUND20.md): NC oc:id/trashbin buffer reuse, I1 sibling CardDAV/CalDAV listing paths, Contact JSONB Json<Vec<_>> decode, dedup settle_batch &str bind, and a fast DoS-resistant hasher for hot trusted-key maps (needs a dependency decision). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JsJjcVX9RoN96DMa35Wqzd
239 lines
15 KiB
Markdown
239 lines
15 KiB
Markdown
# Round 20 — parse-path HashMap purge, owned-DTO moves, Result-collect pre-size, NC etag/favorites emit
|
||
|
||
Benchmark-gated, same rule as ROUND2–19: every change ships with a BEFORE/AFTER
|
||
benchmark and a byte/-value equivalence gate; an AFTER that doesn't beat its
|
||
BEFORE is rolled back (never applied). The roll-back rule is encoded directly in
|
||
the harness — a `GATE FAIL … rollback` non-zero exit if an AFTER arm fails to
|
||
reduce allocations — so a regression fails CI rather than shipping.
|
||
|
||
This round drains three seams the earlier passes left: the **inbound parse
|
||
paths** (CalDAV iCal, CardDAV vCard) that rounds 4–19 optimized on the *emit*
|
||
side but not on ingest; three **owned-entity → DTO conversions** that the
|
||
`into_parts` move-not-clone rounds skipped (`User`, `Calendar`, `AddressBook`);
|
||
and a **stdlib footgun** — `collect::<Result<Vec<_>, _>>()` never pre-sizes — on
|
||
the file-listing repositories. Plus two NextCloud DAV emit micro-cuts the M4/M6
|
||
row passes didn't reach.
|
||
|
||
Reproduce:
|
||
|
||
```
|
||
cargo run --release --features bench --example bench_round20_micro
|
||
```
|
||
|
||
All arms are **no-Postgres** (release-profile counting-allocator example).
|
||
|
||
## Summary
|
||
|
||
| # | change | key metric | before → after |
|
||
|--:|---|---|---|
|
||
| **A1** | `CalendarEvent::prop_with_params` built a throwaway `HashMap<String, Vec<String>>` (uppercased keys + cloned value Vecs) per DTSTART/DTEND/RECURRENCE-ID on **every CalDAV PUT / iCal import**, though all 5 production call sites only read `.get("VALUE")` (all-day detect) or discarded the map. Now `prop_value_and_is_date` scans `prop.params` directly for a case-insensitive `VALUE=DATE`. | per timed event | **6 → 2 allocs/op · 4.15× wall** (168.8 → 40.7 ns) |
|
||
| **A2** | `UserDto::from(User)` took the `User` **by value** yet cloned every field through its accessors — including `image` (a data URI up to **512 KiB**) and `ui_preferences` (a full `serde_json::Value` tree) — on **every `/api/auth/me`** and admin user listing. Now `User::into_parts()` moves the owned fields (the treatment File/Folder/Contact already had). | 48 KiB avatar user | **27 → 14 allocs/op · 2.14× wall** (image memcpy + JSON deep-clone gone) |
|
||
| **A3** | `ContactService::parse_vcard` collected `vcard_data.lines()` into a `Vec` it only iterated, and ran `line.to_ascii_uppercase()` — a full per-line `String` copy — per EMAIL/TEL/ADR line just to `.contains` a `TYPE=` token, on **every CardDAV PUT / vCard import**. Now iterates `lines()` directly and matches with the allocation-free `common::text::ascii_ci_contains` (the CalDAV parse path already used this shape). | 2 email / 1 tel / 1 adr | **8 → 1 allocs/op · 1.67× wall** (444.8 → 266.1 ns) |
|
||
| **A4** | `CalendarDto::from` / `AddressBookDto::from` consumed the entity yet cloned `name`/`description`/`color` and (calendars) the whole `custom_properties` `HashMap<String,String>`, on **every CalDAV/CardDAV discovery listing**. Now `Calendar`/`AddressBook` grow `into_parts()` and move them. | calendar + 2 props | **18 → 10 allocs/op · 1.78× wall** |
|
||
| **I1** | The file-listing repositories map rows with `.collect::<Result<Vec<T>, E>>()`, whose `Result`-shunt reports `size_hint().0 == 0` — so the `Vec` grows **from capacity 0** with ~⌈log₂N⌉ reallocations, memcpy-ing the accumulated `File`-sized rows each grow. Now `Vec::with_capacity(rows.len())` + push with `?` (the pattern `list_media_files` already used). | 500-row listing | **8 → 1 allocs/op** (container reallocs 8 → 0) |
|
||
| **I4** | `encrypted_blob_backend::plaintext_stream` `.collect()`ed every emit-slice into a `Vec` before `stream::iter` — an eager container of ⌈len/64 KiB⌉ entries per **encrypted-blob read**. Now hands the lazy `map` iterator to `stream::iter` directly (same slice sequence). | 4 MiB → 64 slices | **2 → 1 allocs/op · 42.85× wall** (1732.8 → 40.4 ns) |
|
||
| **C1** | NC `write_etag_element` built a `"…"`-quoted `String` then wrote it auto-escaped — `quick_xml` escapes the `"` → `"`, re-allocating an owned `Cow`. Called **per file AND per folder row** of the NC streaming PROPFIND (the hottest DAV emit path), plus every favorites/search REPORT row and trashed item. Now emits the two quotes as **borrowed pre-escaped** `"` text events around the escaped body. | per PROPFIND row | **3 → 0 allocs/op · 1.71× wall** (137.9 → 80.5 ns) |
|
||
| **C3** | The NC favorites REPORT (`oc:filter-files`) hydrated `files`/`folders` by `file_map.get(&id).clone()` — cloning the **whole** `FileDto`/`FolderDto` out of maps that are dropped at fn end. Now `map.remove(&id)` moves them (item ids are unique per user; favorites order preserved — the round-19 M4 move-not-clone pattern applied to a path it missed). | 20 favorites | **302 → 162 allocs/op · 1.35× wall** (~7 allocs/favorite) |
|
||
|
||
> Allocs/op is the deterministic primary gate (identical run to run). Wall
|
||
> figures are single-shot and noise-bounded. Every section carries a
|
||
> byte/-value equivalence gate; the shipped source now matches each AFTER arm.
|
||
|
||
## [A1] CalendarEvent iCal parse — drop the per-property parameter HashMap
|
||
|
||
`from_ical` and `update_ical_data` parse a VEVENT once, then read DTSTART, DTEND
|
||
and RECURRENCE-ID via `prop_with_params`, which built a full
|
||
`HashMap<String, Vec<String>>` per property:
|
||
|
||
```rust
|
||
let mut params: HashMap<String, Vec<String>> = HashMap::new();
|
||
if let Some(param_list) = &prop.params {
|
||
for (name, values) in param_list {
|
||
params.insert(name.to_ascii_uppercase(), values.clone()); // upper key + value clone
|
||
}
|
||
}
|
||
Some((trimmed.to_string(), params))
|
||
```
|
||
|
||
Every production caller only ever asked the map one question — *does it carry
|
||
`VALUE=DATE`?* (the all-day / date-only marker) — and the two DTEND sites
|
||
discarded the map outright (`_dtend_params`, `_params`). The new
|
||
`prop_value_and_is_date` answers exactly that, scanning `prop.params` directly:
|
||
|
||
```rust
|
||
let is_date = prop.params.as_ref()
|
||
.and_then(|list| list.iter().rev().find(|(n, _)| n.eq_ignore_ascii_case("VALUE")))
|
||
.map(|(_, vs)| vs.iter().any(|v| v.eq_ignore_ascii_case("DATE")))
|
||
.unwrap_or(false);
|
||
```
|
||
|
||
`.rev().find(...)` reproduces the old map's last-insert-wins semantics for a
|
||
(pathological) duplicate-`VALUE` property, so the flag is byte-identical; DTEND
|
||
now uses the plain `prop_value`. `prop_with_params` is retained behind
|
||
`#[cfg(test)]` for its existing test wrapper. On a timed event (DTSTART+DTEND,
|
||
each with a `TZID`): **6 → 2 allocs/op, 4.15× wall** — the 2 remaining are the
|
||
DTSTART/DTEND value strings the callers need owned.
|
||
|
||
## [A2] UserDto::from — move the 512 KiB image + JSON, don't clone
|
||
|
||
`UserDto::from` consumes an owned `User` yet cloned every field through the
|
||
borrowing accessors. Two of them are large: `image` is "a data URI of up to
|
||
512 KiB" (the entity's own comment) and `ui_preferences` is a
|
||
`serde_json::Value` tree — both deep-cloned on **every `/api/auth/me`** (session
|
||
bootstrap on every app load, and after each profile edit) and once per user in
|
||
admin listings. `User` was the one core entity without `into_parts`; adding it
|
||
(exhaustive-destructure, compiler-checked) lets the conversion move:
|
||
|
||
```rust
|
||
let role = format!("{}", user.role());
|
||
let can_edit_image = !user.is_oidc_user(); // derived flags read before the move
|
||
let p = user.into_parts();
|
||
… image: p.image, ui_preferences: p.ui_preferences,
|
||
auth_provider: p.oidc_provider.unwrap_or_else(|| "local".to_string()), …
|
||
```
|
||
|
||
**27 → 14 allocs/op, 2.14× wall** — the `image` memcpy + `String` alloc, the
|
||
`ui_preferences` deep-clone, and 5 small field clones are gone; the OIDC-user
|
||
`auth_provider` also stops re-allocating (moves the provider `String`). The DTO
|
||
is byte-identical.
|
||
|
||
## [A3] parse_vcard — allocation-free `TYPE=` routing
|
||
|
||
`parse_vcard` (every CardDAV PUT / bulk import) collected the body into
|
||
`Vec<&str>` it only iterated, and per EMAIL/TEL/ADR line ran
|
||
`line.to_ascii_uppercase()` — a whole-line copy — purely to `.contains("TYPE=…")`.
|
||
This is the exact allocation the CalDAV parse path already killed with
|
||
`starts_with_ci`/`find_ci`; `ascii_ci_contains` was promoted from
|
||
`search_service` to the shared `common::text` module (DRY) and both callers now
|
||
use it. **8 → 1 allocs/op, 1.67× wall** for a 2-email/1-phone/1-address card
|
||
(the remaining alloc is the result Vec both arms build).
|
||
|
||
## [A4] Calendar/AddressBook DTO — finish the into_parts family
|
||
|
||
`CalendarDto::from` / `AddressBookDto::from` consumed the entity but cloned
|
||
`name`/`description`/`color` and — for calendars — the whole
|
||
`custom_properties` `HashMap<String,String>`, on every CalDAV/CardDAV discovery
|
||
listing (DAVx5/Apple poll these repeatedly). Both entities grew `into_parts()`
|
||
and the conversions move. **18 → 10 allocs/op, 1.78× wall** (the HashMap clone +
|
||
3 string clones gone; the two `Uuid::to_string`s remain).
|
||
|
||
## [I1] Result-collect never pre-sizes — the file-listing repositories
|
||
|
||
`collect::<Result<Vec<T>, E>>()` collects through a `Result` shunt whose
|
||
`size_hint().0` is `0` (any element may short-circuit the collect), so `Vec`'s
|
||
`extend` reserves nothing and the container grows **from capacity 0** — ~⌈log₂N⌉
|
||
reallocations, each memcpy-ing the accumulated `File` rows (≈120 B apiece). The
|
||
bench isolates the container behaviour on 500 File-sized rows: **8 container
|
||
reallocations → 0** (one `with_capacity` alloc). Applied to the four
|
||
`file_blob_read_repository` listing/paging/subtree/by-ids mappers (the hottest
|
||
paths — folder browse, PROPFIND, search, favorites/ACL hydration); the fix is
|
||
the loop `list_media_files` already used:
|
||
|
||
```rust
|
||
let mut files = Vec::with_capacity(rows.len());
|
||
for (id, name, …) in rows {
|
||
files.push(Self::row_to_file(id, name, …).map_err(…)?);
|
||
}
|
||
Ok(files)
|
||
```
|
||
|
||
`?` short-circuits on the first row error exactly as the `Result`-collect did —
|
||
byte-identical behaviour and error message.
|
||
|
||
## [I4] plaintext_stream — lazy emit iterator
|
||
|
||
The encrypted backend's `plaintext_stream` `.collect()`ed a
|
||
`Vec<Result<Bytes>>` of ⌈len/64 KiB⌉ zero-copy slices before handing it to
|
||
`stream::iter` — an eager container built per encrypted read (a legacy
|
||
whole-file blob → thousands of entries). The `move` closure owns the refcounted
|
||
`Bytes`, so the `map` iterator is `Send + 'static` and can be streamed lazily.
|
||
**2 → 1 allocs/op, 42.85× wall** (the eager Vec build + fill is gone; each slice
|
||
is now produced on demand as the consumer polls, also cutting peak RAM).
|
||
|
||
## [C1] NC write_etag_element — borrowed pre-escaped quotes
|
||
|
||
`write_etag_element` is called per file **and** per folder row of the NC
|
||
streaming PROPFIND — the single most-travelled DAV emit path — plus every
|
||
favorites/search REPORT row and trashed item. It built a `"…"`-quoted `String`
|
||
and wrote it auto-escaped; `quick_xml` escapes a literal `"` to `"`, so the
|
||
whole-string escape re-allocated an owned `Cow` (3 allocs total, measured). The
|
||
new form emits the two quotes as **borrowed** pre-escaped `"` text events
|
||
around the escaped etag body:
|
||
|
||
```rust
|
||
xml.write_event(Event::Text(BytesText::from_escaped(""")))?; // borrowed, 0 alloc
|
||
xml.write_event(Event::Text(BytesText::new(etag)))?; // escaped body
|
||
xml.write_event(Event::Text(BytesText::from_escaped(""")))?;
|
||
```
|
||
|
||
The output is byte-identical to escaping `"{etag}"` as one string — the
|
||
equivalence gate asserts it, including an etag with `&`/`<`/`"`. **3 → 0
|
||
allocs/op, 1.71× wall.**
|
||
|
||
## [C3] favorites REPORT — move the DTO out of the map
|
||
|
||
`oc:filter-files` builds `file_map`/`folder_map` two lines before the hydrate
|
||
loop, uses them only to populate `files`/`folders` in favorites order, and drops
|
||
them at fn end — yet cloned the **whole** DTO out with `.get().clone()`. Since
|
||
`favorites.item_id` is unique per user, `.remove()` moves the DTO out with no
|
||
risk of dropping a needed duplicate and preserves order (the round-19 M4
|
||
pattern). **302 → 162 allocs/op** for a 20-favorite page — ~7 owned-String
|
||
allocs saved per favorite.
|
||
|
||
## Not shipped — deferred to a later round
|
||
|
||
Surfaced during the Round-20 audit, measured or confirmed, but held back to keep
|
||
this round's diff focused / because they need Postgres or a dependency decision:
|
||
|
||
- **NC `oc:id` per-row `String` (`format_oc_id`):** `format!("{:08}{instance}")`
|
||
allocates one `String` per PROPFIND/REPORT/trashbin row. A `format_oc_id_into(&mut
|
||
String, …)` buffer reused across the page (mirroring the M6 href buffer already
|
||
threaded through those loops) makes it **1 → 0 allocs/row** — but it's a
|
||
multi-signature change through `write_file_response`/`write_folder_response`,
|
||
deferred to keep this round per-item-local.
|
||
- **NC trashbin PROPFIND per-item href + folder content_type:** the trashbin loop
|
||
still `format!`s each `href` and `"httpd/unix-directory".to_string()`s the folder
|
||
content-type per row — the M6 href-buffer + `Cow<'static, str>` fix that reached
|
||
the files/folders loops but not trashbin.
|
||
- **I1 sibling listing paths:** the same `collect::<Result<Vec>>()` /
|
||
`Vec::new()`+push shape lives in the CardDAV (`contact_pg_repository`,
|
||
`contact_group_pg_repository`) and CalDAV (`calendar_event_pg_repository`,
|
||
`calendar_pg_repository`) row mappers. Mechanically identical to the file-side
|
||
fix shipped here; extend next (bulk address-book / calendar sync builds
|
||
thousands of rows).
|
||
- **Contact JSONB columns decode through a throwaway `serde_json::Value`
|
||
(`contact_pg_repository::row_to_contact`, needs Postgres to bench):**
|
||
`row.get::<JsonValue>` builds a full `Value` tree per email/phone/address column
|
||
before `from_value` walks and drops it. `sqlx::types::Json<Vec<Dto>>` runs
|
||
`from_slice` on the raw JSONB — same Vec, no intermediate tree, tens of allocs
|
||
saved per contact.
|
||
- **Dedup `settle_batch` clones chunk-hash `String`s for the SQL array bind
|
||
(`dedup_service`, needs Postgres):** `batch.iter().map(|(h, _)| h.clone())` deep-
|
||
clones each 64-char hash purely to `.bind()`, though `batch` outlives the query;
|
||
`&[&str]` encodes to `text[]` identically — up to 32 fewer allocs per new-content
|
||
batch (~4000 over a 1 GB upload).
|
||
- **Fast hasher for internal maps (cross-cutting, needs a dependency decision):**
|
||
every `HashMap`/`HashSet` in the tree uses std SipHash. Trusted-key,
|
||
built-per-request maps would benefit from a faster `BuildHasher` — the hottest
|
||
are the NC PROPFIND per-row `favorite_ids.contains(&file.id)` /
|
||
`nc_id_of` lookups, and the delta-upload `distinct_hashes` /
|
||
`authorize_chunk_download` sets over up to ~40 000 client-supplied 64-char
|
||
hashes. Two caveats keep it out of this round: it changes **no allocations** (so
|
||
it can't use the alloc gate — only the noisy wall metric), and it needs a
|
||
`Cargo.toml` dependency; the delta sets are **attacker-controlled**, so the
|
||
replacement must stay DoS-resistant (`ahash`/`foldhash` with a random seed, not
|
||
`FxHash`). Worth a dedicated, wall-gated evaluation.
|
||
|
||
## Environment / methodology
|
||
|
||
- `cargo run --release --features bench --example bench_round20_micro` —
|
||
counting global allocator, no Postgres. Tunables (env): `BENCH_ITERS` (200000),
|
||
`I1_ROWS` (500).
|
||
- Each section is BEFORE (verbatim replica of the shipped-before shape) vs AFTER
|
||
(verbatim replica of the shipped-after shape, which the source is then made to
|
||
match), with a byte/-value equivalence gate; the shipped source now matches
|
||
each AFTER arm.
|
||
- Roll-back rule encoded per section: the harness `std::process::exit(1)`s with
|
||
`GATE FAIL … rollback` if an AFTER arm fails to reduce allocations. All eight
|
||
sections pass.
|