diff --git a/Cargo.toml b/Cargo.toml index ae85d096..2e9368b8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -354,6 +354,19 @@ name = "bench_micro_allocs" path = "examples/bench_micro_allocs.rs" required-features = ["bench"] +# Round-18 battery ──────────────────────────────────────────────────────────── + +# Round-18 calendar-event edit CPU/alloc micro-pack — `update_ical_property` / +# `remove_ical_property` mutate the `ical_data` body in place (`replace_range` / +# `insert`) instead of re-`format!`ing the whole body per changed property, and +# build the single `\nNAME:` search needle on the stack (dropping the redundant +# `\r\nNAME:` needle). A multi-field REST edit went from one full-body (up to +# ~11 KB) allocation per property to none. No Postgres. +[[example]] +name = "bench_round18_micro" +path = "examples/bench_round18_micro.rs" +required-features = ["bench"] + # Round-17 battery ──────────────────────────────────────────────────────────── # Round-17 dedup + CardDAV CPU/alloc micro-pack — `hash_chunk_sequence` by-value diff --git a/benches/ROUND18.md b/benches/ROUND18.md new file mode 100644 index 00000000..4a1e33ae --- /dev/null +++ b/benches/ROUND18.md @@ -0,0 +1,183 @@ +# Round 18 — calendar-event in-place iCal edit, ResourceList incremental id-index + +Benchmark-gated, same rule as ROUND2–17: every change ships with a +BEFORE/AFTER benchmark and an equivalence/safety gate; an AFTER that doesn't +beat its BEFORE is rolled back (never applied). The roll-back rule is encoded +directly into each harness — a `GATE FAIL … rollback` non-zero exit (Rust) or a +failing `expect(afterMs).toBeLessThan(beforeMs / K)` assertion (vitest) — so a +regression fails CI rather than shipping. + +This round picks up two items carried on the **ROUND17 deferred list**: + +- the **REST calendar-event edit** that re-`format!`'d the whole `ical_data` + body once per changed property (backend, no Postgres — counting-allocator + example), and +- **`ResourceList.itemIndexById`**, which re-scanned the whole accumulated list + per infinite-scroll page (frontend, vitest-benchmarked). + +Reproduce: + +``` +cargo run --release --features bench --example bench_round18_micro +cd frontend && npx vitest run src/lib/components/round18.bench.test.ts +``` + +## Summary + +| # | change | key metric | before → after | +|--:|---|---|---| +| **C1** | `CalendarEvent::update_ical_property` / `remove_ical_property` rewrote the ENTIRE `ical_data` body with `format!("{}{}{}")` on every call and allocated **two** search needles per call (`\nNAME:` + the redundant `\r\nNAME:`). `calendar_storage_adapter::update_event` fans a multi-field edit out into one `update_ical_property` per changed field, so a full edit paid one full-body (up to ~11 KB) allocation **per property**. Now the body is mutated in place (`replace_range` for an existing property, four `insert`/`insert_str` for a new one) and the single `\nNAME:` needle is built on the stack. | 9-op edit, 1187-byte body | **70 → 2 allocs/op (68 fewer, ~35×) · 2.46× wall** (4255 → 1727 ns/op) | +| **F1** | `ResourceList` derived `itemIndexById = new Map(items.map((i,idx)=>[i.id,idx]))` — a fresh Map over the WHOLE accumulated list every infinite-scroll page (O(N)/page, Σ O(N²)), and being a new instance each page it also re-fired the reap-stale `$effect` (another O(N) id `Set`/page for a reap an append can never cause). New `ItemIndexBuilder` extends a persistent Map with the fresh page only and reuses the reference across appends. | 40 pages × 50 | **74.1 → 6.4 ms · 11.5× faster** index build across the drain | + +## [C1] calendar-event edit — in-place iCal property rewrite + +`calendar_storage_adapter::update_event` hydrates the stored event, then applies +each present field of the `UpdateEventDto` independently: + +```rust +if let Some(summary) = update.summary { event.update_summary(summary)?; } +if let Some(description) = update.description { event.update_description(Some(description)); } +if let Some(location) = update.location { event.update_location(Some(location)); } +// …start/end (update_time_range), all_day (rewrites DTSTART+DTEND again), rrule… +``` + +Every one of those funnels into `CalendarEvent::update_ical_property` (an absent +field cleared → `remove_ical_property`), and the shipped-before body of that +method rebuilt the **entire** `ical_data` String on each call: + +```rust +let search_str = format!("\n{}:", property_name); // needle 1 +let search_str_alt = format!("\r\n{}:", property_name); // needle 2 (redundant) +let pos = self.ical_data.find(&search_str).or_else(|| self.ical_data.find(&search_str_alt)); +// … +let before = &self.ical_data[..value_start]; +let after = &self.ical_data[value_end..]; +self.ical_data = format!("{}{}{}", before, value, after); // a whole fresh body String +``` + +So a REST edit that changes summary + description + location + start + end + +all-day + rrule allocated **one full-body String per property** — and calendar +bodies run to ~11 KB once attendees / VALARMs are present — plus two throwaway +needles per call. + +Two observations drive the fix: + +1. **The `\r\nNAME:` needle is redundant.** `\nNAME:` is a *suffix* of + `\r\nNAME:`, so `find("\nNAME:")` already matches a CRLF-terminated property + line (returning the `\n` offset) — the `.or_else(find("\r\nNAME:"))` branch + can never be reached. One needle suffices, and since iCal property names are + short ASCII it is built into a 64-byte **stack** buffer (`line_needle`) — zero + heap needle. +2. **The rewrite can be in place.** `replace_range(value_start..value_end, value)` + is byte-for-byte what `before + value + after` produced, but it mutates the + body's own buffer (growing once only when the new value is longer) instead of + allocating a fresh body. The absent-property branch inserts the four pieces at + one point in reverse (`\n`, value, `:`, name) after a single `reserve`, so a + new property costs no fresh-body and no value-sized fragment either. + +Because both arms edit the **same byte spans**, the emitted body is identical — +including the pre-existing quirk that editing a line on a CRLF body drops that +line's `\r` (the old span already included it; `replace_range` over the same +span preserves the behaviour exactly). The bench's equivalence gate asserts the +full 9-op edit is byte-identical, and the existing `calendar_event` unit tests +(`update_summary` / `update_time_range` / `update_all_day` round-trips) pin the +observable semantics. + +Measured (`bench_round18_micro`, counting allocator, no Postgres). Both arms pay +one identical `base.to_string()` reset per op (a shared constant), so the +**fewer-allocs** figure is the pure per-edit saving. + +``` +## [C1] calendar-event multi-field edit (9 ops, 1187-byte body) +| arm | ns/op | allocs/op | +| BEFORE update_event in-place property rewrite | 4255.4 | 70.00 | +| AFTER update_event in-place property rewrite | 1726.5 | 2.00 | +# 2.46x wall, 68.00 fewer allocs/op +``` + +The AFTER arm's two allocations per whole 9-op edit are the shared +`base.to_string()` reset and a single buffer grow (the longer DESCRIPTION value + +the inserted RRULE), versus 70 for the old per-property `format!` fan-out — a +35× cut, byte-identical output. + +## [F1] ResourceList `itemIndexById` — incremental id→index Map + +`ResourceList` pages its list in via infinite scroll (`items = [...items, +...page]`) and derived, on every change: + +```js +const itemIndexById = $derived(new Map(items.map((i, idx) => [i.id, idx]))); +``` + +`selectedItems` reads that Map to project the current selection in list order. +The derive is a full O(N) rebuild of a **fresh** Map over the whole accumulated +list every page — Σ O(N²) across a P-page drain with a selection active — and, +being a new instance each page, it also re-fired the reap-stale `$effect` (which +reference-diffs it), and that effect built *another* throwaway O(N) `Set` of ids +per page for a reap that an append can never trigger (an append only adds ids). + +`ItemIndexBuilder` (new, `$lib/utils/itemIndex.ts`, mirroring +`ResourceSectionsBuilder`) uses the shared O(1) `isAppendExtension` witness: on +an append it indexes only the fresh tail into a persistent Map and returns the +**same reference**; any other change (reload, deletion, non-append) rebuilds into +a **new** Map. That reference contract is exactly what the two consumers want: + +- `selectedItems` re-derives on every `items` change regardless (it indexes + `items[idx]`), so it always reads the freshly-extended Map — a stable + reference on append costs it nothing; +- the reap-stale `$effect` now tests membership against that Map instead of a + fresh `Set`, and a stable reference on append means it **doesn't re-run** there + (nothing to reap) while a rebuild — the delete/reload case — yields a new + reference and **does** re-run it, precisely when stale ids must be dropped. + +Gates (`round18.bench.test.ts`): the builder is asserted deep-equal to the +verbatim `buildItemIndex` reference at **every** page of a 12-page drain (and on +the final index of a 20-page one), a later duplicate id resolves to its highest +index (matching `Map`'s last-wins), and the reference-contract gate pins +same-ref-on-append / new-ref-on-rebuild. The perf gate requires the incremental +drain to beat the rebuild-per-page by ≥5×. + +Measured: a 40-page × 50-item drain builds the index in **6.4 ms vs 74.1 ms — +11.5× faster** — and no longer churns a fresh Map + id-Set per page. + +## Not shipped — deferred to a later round + +Surfaced during the Round-18 audit but not landed (each needs its own decision, +fixture, or a different reactivity treatment): + +- **Frontend — flat dotfile filter (`ResourceList` inline `items.filter` + + `utils/dotfileFilter::filterDotfiles`, on photos):** the ROUND17 note flagged + it as an O(N²) per-page rescan when *hide dotfiles* is on. Unlike the favorites + `Set` (ROUND14 §F2) or this round's id-`Map`, the filtered result is a **flat + array** that feeds `.filter`/rendering — reactivity needs a *fresh* array + reference each page, and building one by `prev.concat(freshFiltered)` is itself + O(N) (a mutate-in-place same-reference array would stop `photoRows` / + `visibleItems` consumers from recomputing). There is no clean O(N)→O(page) win + without partitioning the list; deferred pending a design (e.g. filtering per + page in the loader and accumulating in page state, which changes the + toggle-refilter semantics). +- **Frontend — `VirtualRows.offsets` prefix-sum (photos timeline):** rebuilt in + full per page. An incremental prefix-sum needs (a) a version-counter to force + `band`/`totalHeight` to recompute without a fresh `offsets` array reference + (Svelte deriveds short-circuit on `===`), and (b) `PhotoTimeline` to guarantee + row-object identity at the append boundary (a page that grows the last group + re-lays-out its trailing strip row, breaking `isAppendExtension`). Both are + real but want their own round; the raw numeric prefix-sum is also cheap (rows ≪ + photos), so this is lower-priority than the item-list rescans. +- **Backend query-shape (needs Postgres, carried from ROUND17):** + `music_storage_adapter::list_public_playlists` 1 + N `COUNT(*)` fold; contact + REST listings over-fetch the multi-KB `vcard` TEXT the `ContactDto` mappers + never read (wants a *lite* row mapper). + +## Environment / methodology + +- `cargo run --release --features bench --example bench_round18_micro` — + counting global allocator, no Postgres. Tunable: `BENCH_ITERS` (200000). +- `cd frontend && npx vitest run src/lib/components/round18.bench.test.ts` — + equivalence, reference-contract, and wall-time perf gates. +- Each section is BEFORE (verbatim replica of the shipped-before shape) vs AFTER + (verbatim replica of the shipped-after shape) with a byte/-value equivalence + gate; the shipped source now matches each AFTER arm. +- Roll-back rule encoded per section: the Rust harness `std::process::exit(1)`s + with `GATE FAIL … rollback` if an AFTER arm fails to reduce allocations; the + vitest perf gate fails the test if the incremental arm isn't ≥5× faster. diff --git a/examples/bench_round18_micro.rs b/examples/bench_round18_micro.rs new file mode 100644 index 00000000..3e21d448 --- /dev/null +++ b/examples/bench_round18_micro.rs @@ -0,0 +1,331 @@ +//! Round-18 calendar-event edit CPU/alloc micro-pack (no Postgres). +//! +//! Same rule as ROUND2–17: each section is BEFORE (verbatim replica of the +//! shipped-before shape) vs AFTER (verbatim replica of the shipped-after +//! shape), with a byte-for-byte equivalence gate and a `GATE FAIL … rollback` +//! check that exits non-zero if the AFTER arm fails to reduce allocations — the +//! round's roll-back rule encoded into the benchmark. +//! +//! [C1] `CalendarEvent::update_ical_property` / `remove_ical_property` +//! rewrote the ENTIRE `ical_data` body with `format!("{}{}{}")` on every +//! call, and allocated TWO search needles per call (`\nNAME:` and the +//! redundant `\r\nNAME:` — the CRLF form can never match where the LF +//! form doesn't, since `\nNAME:` is its suffix). Because +//! `calendar_storage_adapter::update_event` applies each changed field +//! independently, a multi-field REST edit paid one full-body (up to +//! ~11 KB) String allocation PER changed property, plus two needles. +//! The shipped-after form mutates the body in place (`replace_range` for +//! an existing property, four `insert`/`insert_str` for a new one) and +//! builds the single `\nNAME:` needle on the stack — zero heap needle, +//! no fresh-body allocation. The edited spans are byte-for-byte the same +//! the `format!` reconstruction produced (`replace_range(a..b, v)` ≡ +//! `before + v + after`), so the emitted body is identical — including +//! the pre-existing quirk that editing a CRLF line drops its `\r`. +//! +//! Run: +//! cargo run --release --features bench --example bench_round18_micro +//! Tunables (env): BENCH_ITERS (200000) + +use std::alloc::{GlobalAlloc, Layout, System}; +use std::env; +use std::hint::black_box; +use std::sync::atomic::{AtomicU64, Ordering}; +use std::time::Instant; + +static ALLOC_CALLS: AtomicU64 = AtomicU64::new(0); + +struct CountingAlloc; + +unsafe impl GlobalAlloc for CountingAlloc { + unsafe fn alloc(&self, layout: Layout) -> *mut u8 { + ALLOC_CALLS.fetch_add(1, Ordering::Relaxed); + unsafe { System.alloc(layout) } + } + unsafe fn dealloc(&self, ptr: *mut u8, layout: Layout) { + unsafe { System.dealloc(ptr, layout) } + } + unsafe fn realloc(&self, ptr: *mut u8, layout: Layout, new_size: usize) -> *mut u8 { + ALLOC_CALLS.fetch_add(1, Ordering::Relaxed); + unsafe { System.realloc(ptr, layout, new_size) } + } + unsafe fn alloc_zeroed(&self, layout: Layout) -> *mut u8 { + ALLOC_CALLS.fetch_add(1, Ordering::Relaxed); + unsafe { System.alloc_zeroed(layout) } + } +} + +#[global_allocator] +static GLOBAL: CountingAlloc = CountingAlloc; + +fn env_or(key: &str, default: T) -> T { + env::var(key) + .ok() + .and_then(|v| v.parse().ok()) + .unwrap_or(default) +} + +struct Measured { + wall_ns_per_op: f64, + allocs_per_op: f64, +} + +fn measure(iters: usize, mut f: F) -> Measured { + let a0 = ALLOC_CALLS.load(Ordering::Relaxed); + let t = Instant::now(); + for _ in 0..iters { + f(); + } + let wall = t.elapsed().as_nanos() as f64 / iters as f64; + let allocs = (ALLOC_CALLS.load(Ordering::Relaxed) - a0) as f64 / iters as f64; + Measured { + wall_ns_per_op: wall, + allocs_per_op: allocs, + } +} + +fn print_row(label: &str, m: &Measured) { + println!( + "| {:<44} | {:>12.1} | {:>10.2} |", + label, m.wall_ns_per_op, m.allocs_per_op + ); +} + +fn header_footer(name: &str, before: &Measured, after: &Measured) { + println!("| arm | ns/op | allocs/op |"); + print_row(&format!("BEFORE {name}"), before); + print_row(&format!("AFTER {name}"), after); + println!( + "# {:.2}x wall, {:.2} fewer allocs/op", + before.wall_ns_per_op / after.wall_ns_per_op, + before.allocs_per_op - after.allocs_per_op + ); +} + +fn gate_allocs(tag: &str, before: &Measured, after: &Measured) { + if after.allocs_per_op >= before.allocs_per_op { + eprintln!("GATE FAIL [{tag}]: AFTER did not reduce allocations — rollback"); + std::process::exit(1); + } +} + +// ──────────────────────────────────────────────────────────────────────────── +// [C1] calendar-event edit — per-property full-body format! vs in-place edit +// ──────────────────────────────────────────────────────────────────────────── + +/// An edit step, mirroring the `event.update_*(…)` calls that +/// `calendar_storage_adapter::update_event` fans out into `update_ical_property` +/// (present → replace) and `remove_ical_property` (a cleared `Option`). +enum Op<'a> { + Update(&'a str, &'a str), + Remove(&'a str), +} + +// ── BEFORE: verbatim replica of the shipped-before methods ────────────────── + +fn before_update(ical: &mut String, property_name: &str, value: &str) { + let search_str = format!("\n{}:", property_name); + let search_str_alt = format!("\r\n{}:", property_name); + + let pos = ical + .find(&search_str) + .or_else(|| ical.find(&search_str_alt)); + + if let Some(pos) = pos { + let value_start = pos + search_str.len(); + let value_end = ical[value_start..] + .find('\n') + .map(|p| value_start + p) + .unwrap_or_else(|| ical.len()); + let before = &ical[..value_start]; + let after = &ical[value_end..]; + *ical = format!("{}{}{}", before, value, after); + } else { + let end_pos = ical.find("END:VEVENT").unwrap_or(ical.len()); + let before = &ical[..end_pos]; + let after = &ical[end_pos..]; + *ical = format!("{}{}:{}\n{}", before, property_name, value, after); + } +} + +fn before_remove(ical: &mut String, property_name: &str) { + let search_str = format!("\n{}:", property_name); + let search_str_alt = format!("\r\n{}:", property_name); + + let pos = ical + .find(&search_str) + .or_else(|| ical.find(&search_str_alt)); + + if let Some(pos) = pos { + let value_end = ical[pos + 1..] + .find('\n') + .map(|p| pos + 1 + p) + .unwrap_or_else(|| ical.len()); + let before = &ical[..pos]; + let after = &ical[value_end..]; + *ical = format!("{}{}", before, after); + } +} + +// ── AFTER: verbatim replica of the shipped-after methods ──────────────────── + +fn line_needle<'a>(buf: &'a mut [u8; 64], name: &str) -> Option<&'a str> { + let n = name.len(); + if n + 2 > buf.len() { + return None; + } + buf[0] = b'\n'; + buf[1..1 + n].copy_from_slice(name.as_bytes()); + buf[1 + n] = b':'; + std::str::from_utf8(&buf[..n + 2]).ok() +} + +fn after_update(ical: &mut String, property_name: &str, value: &str) { + let mut buf = [0u8; 64]; + let needle_owned; + let needle: &str = match line_needle(&mut buf, property_name) { + Some(n) => n, + None => { + needle_owned = format!("\n{property_name}:"); + &needle_owned + } + }; + + if let Some(pos) = ical.find(needle) { + let value_start = pos + needle.len(); + let value_end = ical[value_start..] + .find('\n') + .map_or(ical.len(), |p| value_start + p); + ical.replace_range(value_start..value_end, value); + } else { + let end_pos = ical.find("END:VEVENT").unwrap_or(ical.len()); + ical.reserve(property_name.len() + value.len() + 2); + ical.insert(end_pos, '\n'); + ical.insert_str(end_pos, value); + ical.insert(end_pos, ':'); + ical.insert_str(end_pos, property_name); + } +} + +fn after_remove(ical: &mut String, property_name: &str) { + let mut buf = [0u8; 64]; + let needle_owned; + let needle: &str = match line_needle(&mut buf, property_name) { + Some(n) => n, + None => { + needle_owned = format!("\n{property_name}:"); + &needle_owned + } + }; + + if let Some(pos) = ical.find(needle) { + let value_end = ical[pos + 1..] + .find('\n') + .map_or(ical.len(), |p| pos + 1 + p); + ical.replace_range(pos..value_end, ""); + } +} + +fn apply_before(base: &str, ops: &[Op]) -> String { + let mut ical = base.to_string(); + for op in ops { + match op { + Op::Update(n, v) => before_update(&mut ical, n, v), + Op::Remove(n) => before_remove(&mut ical, n), + } + } + ical +} + +fn apply_after(base: &str, ops: &[Op]) -> String { + let mut ical = base.to_string(); + for op in ops { + match op { + Op::Update(n, v) => after_update(&mut ical, n, v), + Op::Remove(n) => after_remove(&mut ical, n), + } + } + ical +} + +/// A realistic stored VEVENT body (CRLF-terminated, ~1.5 KB with attendees and +/// a VALARM) — the shape `calendar_storage_adapter` hydrates before applying an +/// `UpdateEventDto`. +fn base_body() -> String { + let mut b = String::from( + "BEGIN:VCALENDAR\r\nVERSION:2.0\r\nPRODID:-//OxiCloud//NONSGML Calendar//EN\r\n\ + BEGIN:VEVENT\r\nUID:evt-round18@oxicloud.test\r\nDTSTAMP:20260101T100000Z\r\n\ + DTSTART:20260101T120000Z\r\nDTEND:20260101T130000Z\r\n\ + SUMMARY:Original quarterly planning sync\r\n\ + DESCRIPTION:The original description body for the event, moderately long.\r\n\ + LOCATION:Room A, Ground Floor\r\nCATEGORIES:work,planning,quarterly\r\n\ + ORGANIZER;CN=Alice Example:mailto:alice@oxicloud.test\r\n", + ); + // A handful of attendees + a VALARM to bring the body to a realistic size, + // so BEFORE's per-property full-body `format!` copies real bytes. + for i in 0..8 { + b.push_str(&format!( + "ATTENDEE;CN=Guest {i};PARTSTAT=NEEDS-ACTION;RSVP=TRUE:mailto:guest{i}@oxicloud.test\r\n" + )); + } + b.push_str( + "BEGIN:VALARM\r\nACTION:DISPLAY\r\nDESCRIPTION:Reminder\r\nTRIGGER:-PT15M\r\nEND:VALARM\r\n\ + END:VEVENT\r\nEND:VCALENDAR\r\n", + ); + b +} + +fn section_calendar_edit() { + let iters: usize = env_or("BENCH_ITERS", 200_000); + let base = base_body(); + + // A full multi-field REST edit: five existing properties replaced (SUMMARY, + // DESCRIPTION, LOCATION, DTSTART, DTEND — the last two rewritten twice, as + // the time-range + all-day updates both do), one new property inserted + // (RRULE, absent from the body), one cleared (CATEGORIES). Exactly the + // `update_ical_property` / `remove_ical_property` fan-out of `update_event`. + let ops = [ + Op::Update("SUMMARY", "Updated quarterly planning sync"), + Op::Update( + "DESCRIPTION", + "A revised, noticeably longer description so the replacement value differs in length from the original and exercises the grow path.", + ), + Op::Update("LOCATION", "Conference Room 42, Building B"), + Op::Update("DTSTART", "20260202T090000Z"), + Op::Update("DTEND", "20260202T100000Z"), + Op::Update("DTSTART", "20260202T000000Z"), + Op::Update("DTEND", "20260202T010000Z"), + Op::Update("RRULE", "FREQ=WEEKLY;COUNT=10"), + Op::Remove("CATEGORIES"), + ]; + + // Equivalence gate: the emitted body is byte-for-byte identical. + let b = apply_before(&base, &ops); + let a = apply_after(&base, &ops); + assert_eq!(b, a, "C1 emitted body differs between BEFORE and AFTER"); + + let before = measure(iters, || { + black_box(apply_before(black_box(&base), black_box(&ops))); + }); + let after = measure(iters, || { + black_box(apply_after(black_box(&base), black_box(&ops))); + }); + + println!( + "\n## [C1] calendar-event multi-field edit ({} ops, {}-byte body)", + ops.len(), + base.len() + ); + println!("# both arms pay one identical `base.to_string()` reset per op (constant, shared)"); + header_footer("update_event in-place property rewrite", &before, &after); + gate_allocs("C1", &before, &after); +} + +fn main() { + println!("#################################################################"); + println!("# Round-18 calendar-event edit CPU/alloc micro-pack"); + println!("#################################################################"); + + section_calendar_edit(); + + println!("\nGATE PASS (all sections)"); +} diff --git a/frontend/src/lib/components/ResourceList.svelte b/frontend/src/lib/components/ResourceList.svelte index 628ad18e..9818c4bc 100644 --- a/frontend/src/lib/components/ResourceList.svelte +++ b/frontend/src/lib/components/ResourceList.svelte @@ -79,6 +79,7 @@ import { formatDate, iconNameFromClass, fileIconKindClass } from '$lib/utils/display'; import { gridColumns } from '$lib/utils/grid'; import { ResourceSectionsBuilder } from '$lib/utils/resourceSections'; + import { ItemIndexBuilder } from '$lib/utils/itemIndex'; import { fileThumbnailUrl, thumbSizeForView } from '$lib/api/endpoints/files'; import { canThumbnailClientSide, @@ -468,13 +469,18 @@ // stale-selection cleanup only fires when items truly leave the // dataset (reload, delete, etc.), not when the filter hides them. // - // Index rebuilt only when `items` changes; the projection is then - // O(k · log k) in the selection size k instead of a full O(N) re-scan - // of the list on every toggle (O(N²)-ish across a shift-range gesture - // once the batch toolbar is mounted — benches/ROUND11.md §S1). The - // index sort preserves item order, so the toolbar sees the same array - // the old filter produced. - const itemIndexById = $derived(new Map(items.map((i, idx) => [i.id, idx]))); + // Index extended over the freshly-appended page only (never re-scanned in + // full) via `ItemIndexBuilder`: an infinite-scroll drain with a selection + // active collapses from Σ O(N²) Map rebuilds to O(N) total, and the Map + // reference is reused across appends so the reap-stale effect below no + // longer re-fires (nor re-allocates an O(N) id Set) on a page that removed + // nothing — its reference only changes on a rebuild (reload / deletion), + // exactly when a reap is warranted. The projection is then O(k · log k) in + // the selection size k, not a full O(N) re-scan on every toggle + // (benches/ROUND11.md §S1, benches/ROUND18.md §F1). The index sort preserves + // item order, so the toolbar sees the same array the old filter produced. + const itemIndex = new ItemIndexBuilder(); + const itemIndexById = $derived(itemIndex.sync(items)); const selectedItems = $derived.by(() => { const picked: { idx: number; item: FileItem | FolderItem }[] = []; for (const id of selected) { @@ -487,15 +493,20 @@ // Drop selection ids that are no longer present after a reload. $effect(() => { - // With nothing selected (the common case) every infinite-scroll page - // re-fired this effect and built a throwaway O(N) id Set for a loop - // that never runs — skip straight out. `selected.size` is reactive, - // so the effect re-fires when a selection appears. + // With nothing selected (the common case) the loop never runs — skip + // straight out. `selected.size` is reactive, so the effect re-fires + // when a selection appears. if (selected.size === 0) return; - const ids = new Set(items.map((i) => i.id)); + // Test membership against the incremental `itemIndexById` rather than a + // throwaway O(N) id Set rebuilt per page. Its reference is stable across + // infinite-scroll appends (which never remove an id — nothing to reap) + // so this effect no longer re-fires on every page; the reference changes + // only on a rebuild (reload / deletion), which is exactly when a stale + // selection must be dropped (benches/ROUND18.md §F1). + const index = itemIndexById; let changed = false; for (const id of selected) { - if (!ids.has(id)) { + if (!index.has(id)) { selected.delete(id); changed = true; } diff --git a/frontend/src/lib/components/round18.bench.test.ts b/frontend/src/lib/components/round18.bench.test.ts new file mode 100644 index 00000000..e2542d3a --- /dev/null +++ b/frontend/src/lib/components/round18.bench.test.ts @@ -0,0 +1,134 @@ +// Round-18 frontend micro-pack (benches/ROUND18.md §F1). +// +// Each section is BEFORE (verbatim replica of the shipped-before shape) vs +// AFTER (the shipped incremental builder), with an equivalence gate, a +// reference-contract gate, and a wall-time perf gate — the same discipline as +// the Rust micro-packs: an AFTER that doesn't beat its BEFORE fails the gate. + +import { describe, expect, it } from 'vitest'; +import { buildItemIndex, ItemIndexBuilder } from '$lib/utils/itemIndex'; + +// ──────────────────────────────────────────────────────────────────────────── +// [F1] ResourceList `itemIndexById` — rebuild-a-fresh-Map-per-page vs incremental +// ──────────────────────────────────────────────────────────────────────────── +// +// Audit finding (ROUND17 deferred list): ResourceList derived +// `itemIndexById = new Map(items.map((i, idx) => [i.id, idx]))`. Every +// infinite-scroll page (`items = [...items, ...page]`) rebuilt a brand-new Map +// over the WHOLE accumulated list — O(N) per page, Σ O(N²) across a P-page +// drain — and, being a fresh instance each page, re-fired the reap-stale +// `$effect` that reference-diffs it (allocating another O(N) id Set for a reap +// an append can never trigger). `ItemIndexBuilder` extends the persistent Map +// with the fresh page only and returns the same reference on an append. + +interface Item { + id: string; +} + +/** A page of `{ id }` items (50/page, the default page size). */ +function pageOf(start: number, n: number): Item[] { + return Array.from({ length: n }, (_, i) => ({ id: `it-${start + i}` })); +} + +/** BEFORE: rebuild a fresh Map over the whole accumulated list each page. */ +function rebuildPerPage(pages: Item[][]): Map { + let acc: Item[] = []; + let index = new Map(); + for (const page of pages) { + acc = [...acc, ...page]; // the component's `items = [...items, ...page]` + index = new Map(acc.map((i, idx) => [i.id, idx])); // new instance + O(N) rebuild + } + return index; +} + +/** AFTER: one persistent builder, extend with only the fresh page's ids. */ +function incrementalPerPage(pages: Item[][]): Map { + const builder = new ItemIndexBuilder(); + let acc: Item[] = []; + let index = new Map(); + for (const page of pages) { + acc = [...acc, ...page]; + index = builder.sync(acc); + } + return index; +} + +describe('round18 §F1 — ResourceList itemIndexById incremental Map', () => { + it('final index is identical to the full rebuild (equivalence gate)', () => { + const pages = Array.from({ length: 20 }, (_, p) => pageOf(p * 50, 50)); + const acc = pages.flat(); + const before = rebuildPerPage(pages); + const after = incrementalPerPage(pages); + const reference = buildItemIndex(acc); + expect(after.size).toBe(before.size); + for (const [id, idx] of reference) expect(after.get(id)).toBe(idx); + for (const [id, idx] of after) expect(before.get(id)).toBe(idx); + }); + + it('the index stays deep-equal to the reference at EVERY page (equivalence gate)', () => { + const builder = new ItemIndexBuilder(); + let acc: Item[] = []; + for (let p = 0; p < 12; p++) { + acc = [...acc, ...pageOf(p * 50, 50)]; + const got = builder.sync(acc); + const want = buildItemIndex(acc); + expect(got.size).toBe(want.size); + for (const [id, idx] of want) expect(got.get(id)).toBe(idx); + } + }); + + it('a later duplicate id resolves to its highest index, matching Map (equivalence gate)', () => { + // The old `new Map(items.map(...))` keeps the last (highest-index) + // occurrence of a duplicate id; the incremental extend must too. + const builder = new ItemIndexBuilder(); + const dup: Item = { id: 'dup' }; + const p1 = [dup, { id: 'a' }]; + const p2 = [{ id: 'b' }, dup]; // 'dup' re-appears at index 3 + builder.sync(p1); + const got = builder.sync([...p1, ...p2]); + const want = buildItemIndex([...p1, ...p2]); + expect(got.get('dup')).toBe(want.get('dup')); + expect(got.get('dup')).toBe(3); + }); + + it('reuses the Map reference on append, mints a new one on rebuild (reference-contract gate)', () => { + const builder = new ItemIndexBuilder(); + const p1 = pageOf(0, 50); + const first = builder.sync(p1); + // Append: same reference (so the reap-stale effect does NOT re-fire — + // an append removes nothing). + const appended = builder.sync([...p1, ...pageOf(50, 50)]); + expect(appended).toBe(first); + // Deletion (shorter, non-append prefix): fresh reference (so the + // reap-stale effect DOES re-fire and drops the removed id). + const afterDelete = builder.sync(p1.slice(0, 40)); + expect(afterDelete).not.toBe(first); + expect(afterDelete.has('it-49')).toBe(false); + // Reload with a different first element (non-append): fresh reference. + const reloaded = builder.sync(pageOf(1000, 50)); + expect(reloaded).not.toBe(afterDelete); + }); + + it('a P-page drain builds the index ≥5x faster incrementally (perf gate)', () => { + const PAGES = 40; + const PER = 50; // 2 000 items total + const pages = Array.from({ length: PAGES }, (_, p) => pageOf(p * PER, PER)); + + const run = (f: (p: Item[][]) => Map): number => { + const t0 = performance.now(); + for (let r = 0; r < 20; r++) f(pages); + return performance.now() - t0; + }; + + // Warm-up (JIT) then measure. + run(rebuildPerPage); + run(incrementalPerPage); + const beforeMs = run(rebuildPerPage); + const afterMs = run(incrementalPerPage); + + console.info( + `§F1 ${PAGES} pages × ${PER}: rebuild-per-page ${beforeMs.toFixed(1)} ms vs incremental ${afterMs.toFixed(1)} ms (${(beforeMs / afterMs).toFixed(1)}x)` + ); + expect(afterMs).toBeLessThan(beforeMs / 5); + }); +}); diff --git a/frontend/src/lib/utils/itemIndex.ts b/frontend/src/lib/utils/itemIndex.ts new file mode 100644 index 00000000..d77f6dcc --- /dev/null +++ b/frontend/src/lib/utils/itemIndex.ts @@ -0,0 +1,83 @@ +/** + * Incremental `id → position` index for `ResourceList`, extracted so the O(N²) + * accumulation of its `itemIndexById` `$derived` (and the reap-stale effect's + * per-page `new Set(items.map(…))`) can be replaced with an append-aware + * builder — and unit/benchmark-tested off the Svelte reactive graph. + * + * `ResourceList` pages its list in via infinite scroll (`items = [...items, + * ...page]`) and rebuilt `new Map(items.map((i, idx) => [i.id, idx]))` on every + * page — O(N) per page, Σ ≈ O(N²) across a P-page drain, and a fresh Map each + * page (so the reap-stale effect that reference-diffs it re-ran on every append + * too, allocating another O(N) id Set for a reap that an append can never + * trigger). This is the same class ROUND6 fixed for the files listing, ROUND14 + * §F2 for favorites, and ROUND15/16 for the grouped/shared lanes. + * + * Because a fresh page only ever *appends* (server order is stable; existing + * rows keep their index), {@link ItemIndexBuilder} extends the persistent Map + * with just the new tail on an append and returns the SAME Map reference; any + * other change (reload, deletion, non-append) rebuilds into a NEW Map. That + * reference contract is load-bearing for the two `ResourceList` consumers: + * + * - `selectedItems` re-derives on every `items` change regardless (it indexes + * `items[idx]`), so it always reads the freshly-extended Map — a stable ref + * on append costs it nothing. + * - the reap-stale `$effect` reference-diffs the Map, so a stable ref on + * append means it does NOT re-run there (an append never removes an id, so + * there is nothing to reap), while a rebuild (delete / reload) yields a new + * ref and DOES re-run it — exactly when stale selections must be dropped. + * + * The pure {@link buildItemIndex} is the verbatim reference (what the old + * `itemIndexById` derive produced); the benchmark gate holds the builder equal + * to it at every page. + */ + +import { isAppendExtension } from './appendExtension'; + +/** Minimal shape the index needs: a stable string `id`. */ +export interface HasId { + id: string; +} + +/** + * Verbatim reference: the `Map` the old `itemIndexById` `$derived` + * produced — `new Map(items.map((i, idx) => [i.id, idx]))`. On a duplicate id + * the highest index wins (last insertion), matching `Map`'s own semantics. + */ +export function buildItemIndex(items: readonly T[]): Map { + const index = new Map(); + for (let i = 0; i < items.length; i++) index.set(items[i].id, i); + return index; +} + +/** + * Append-aware `id → index` builder. Call {@link sync} with the current item + * list on every change; it detects the common case — the list grew by appending + * a page — and indexes only the fresh tail, reusing the persistent Map (same + * reference). Any other change rebuilds into a new Map, so the result is always + * deep-equal to {@link buildItemIndex} and the reference changes exactly when a + * reap-stale pass is warranted. + */ +export class ItemIndexBuilder { + /** Last synced list — the append cursor and the append-detection baseline. */ + #items: readonly T[] = []; + /** id → index; a stable reference across appends, a fresh one on rebuild. */ + #index = new Map(); + + sync(items: readonly T[]): Map { + if (isAppendExtension(this.#items, items)) { + // Append: the prefix is unchanged (existing ids keep their index), so + // only the fresh tail needs indexing. A duplicate id in the tail + // overwrites to its higher index — identical to the full rebuild's + // last-wins. Same Map reference is returned (see the class doc). + for (let i = this.#items.length; i < items.length; i++) { + this.#index.set(items[i].id, i); + } + } else { + // Reload / deletion / non-append / first run: rebuild into a NEW Map so + // the reap-stale effect (which reference-diffs it) re-runs. + this.#index = buildItemIndex(items); + } + this.#items = items; + return this.#index; + } +} diff --git a/src/domain/entities/calendar_event.rs b/src/domain/entities/calendar_event.rs index 20186645..944db301 100644 --- a/src/domain/entities/calendar_event.rs +++ b/src/domain/entities/calendar_event.rs @@ -1092,40 +1092,70 @@ impl CalendarEvent { * @param property_name The name of the property to update * @param value The new value for the property */ + /// Write `\n{name}:` into `buf` and return it as `&str`, or `None` if the + /// name is too long to fit (unreachable for RFC 5545 property names, the + /// longest of which — `LAST-MODIFIED`, `RECURRENCE-ID` — are 13 bytes). + /// + /// The bare-LF form is deliberate: `\n{name}:` is a suffix of the CRLF form + /// `\r\n{name}:`, so a single search for it matches a property line whether + /// the body is LF- or CRLF-terminated and returns the LF offset either way + /// — behaviour-identical to the old `find("\n..").or(find("\r\n.."))` (the + /// CRLF needle could never match where the LF one didn't). Built on the + /// stack: no per-call heap needle. + fn line_needle<'a>(buf: &'a mut [u8; 64], name: &str) -> Option<&'a str> { + let n = name.len(); + if n + 2 > buf.len() { + return None; + } + buf[0] = b'\n'; + buf[1..1 + n].copy_from_slice(name.as_bytes()); + buf[1 + n] = b':'; + // `name` is valid UTF-8 and only ASCII bytes were added around it. + std::str::from_utf8(&buf[..n + 2]).ok() + } + fn update_ical_property(&mut self, property_name: &str, value: &str) { - let search_str = format!("\n{}:", property_name); - let search_str_alt = format!("\r\n{}:", property_name); + let mut buf = [0u8; 64]; + let needle_owned; + let needle: &str = match Self::line_needle(&mut buf, property_name) { + Some(n) => n, + None => { + needle_owned = format!("\n{property_name}:"); + &needle_owned + } + }; - // Check if property exists - let pos = self - .ical_data - .find(&search_str) - .or_else(|| self.ical_data.find(&search_str_alt)); - - if let Some(pos) = pos { - // Find the start of the value - let value_start = pos + search_str.len(); - - // Find the end of the value (next line or end of string) + if let Some(pos) = self.ical_data.find(needle) { + // Value spans from just after `\n{NAME}:` to the next LF (or EOF). + let value_start = pos + needle.len(); let value_end = self.ical_data[value_start..] .find('\n') - .map(|p| value_start + p) - .unwrap_or_else(|| self.ical_data.len()); + .map_or(self.ical_data.len(), |p| value_start + p); - // Replace the value - let before = &self.ical_data[..value_start]; - let after = &self.ical_data[value_end..]; - self.ical_data = format!("{}{}{}", before, value, after); + // In place: reuses the body's own buffer (growing it once only when + // the new value is longer) instead of allocating a whole fresh body + // String per property, as the old `format!("{}{}{}")` did — on a + // multi-field edit that was one full-body (up to ~11 KB) allocation + // per changed property. + self.ical_data.replace_range(value_start..value_end, value); } else { - // Property doesn't exist, add it before END:VEVENT + // Property absent: insert `{NAME}:{value}\n` before END:VEVENT. let end_pos = self .ical_data .find("END:VEVENT") .unwrap_or(self.ical_data.len()); - let before = &self.ical_data[..end_pos]; - let after = &self.ical_data[end_pos..]; - self.ical_data = format!("{}{}:{}\n{}", before, property_name, value, after); + // Insert the four pieces at one point in reverse order so the result + // is `{NAME}:{value}\n` before END:VEVENT — byte-identical to the old + // `format!("{}{}:{}\n{}")` — without allocating a fresh body String + // (nor a value-sized fragment). One `reserve` caps it at a single + // grow; the shifted tail is just the trailing END:VEVENT/VCALENDAR. + self.ical_data + .reserve(property_name.len() + value.len() + 2); + self.ical_data.insert(end_pos, '\n'); + self.ical_data.insert_str(end_pos, value); + self.ical_data.insert(end_pos, ':'); + self.ical_data.insert_str(end_pos, property_name); } } @@ -1135,26 +1165,25 @@ impl CalendarEvent { * @param property_name The name of the property to remove */ fn remove_ical_property(&mut self, property_name: &str) { - let search_str = format!("\n{}:", property_name); - let search_str_alt = format!("\r\n{}:", property_name); + let mut buf = [0u8; 64]; + let needle_owned; + let needle: &str = match Self::line_needle(&mut buf, property_name) { + Some(n) => n, + None => { + needle_owned = format!("\n{property_name}:"); + &needle_owned + } + }; - // Check if property exists - let pos = self - .ical_data - .find(&search_str) - .or_else(|| self.ical_data.find(&search_str_alt)); - - if let Some(pos) = pos { - // Find the end of the value (next line or end of string) + if let Some(pos) = self.ical_data.find(needle) { + // Delete from the property's leading LF (`pos`) through the end of + // its value line (exclusive of the next line's LF) — byte-identical + // to the old `format!("{}{}", &data[..pos], &data[value_end..])`, + // but in place, with no fresh body String. let value_end = self.ical_data[pos + 1..] .find('\n') - .map(|p| pos + 1 + p) - .unwrap_or_else(|| self.ical_data.len()); - - // Remove the property - let before = &self.ical_data[..pos]; - let after = &self.ical_data[value_end..]; - self.ical_data = format!("{}{}", before, after); + .map_or(self.ical_data.len(), |p| pos + 1 + p); + self.ical_data.replace_range(pos..value_end, ""); } } }