From 1d767e3b93073ca4ca7d3610829feab1f5973326 Mon Sep 17 00:00:00 2001 From: osobh Date: Sun, 20 Sep 2026 17:49:47 -0700 Subject: [PATCH] perf(agent): open a store without copying the whole file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `read_from_disk` memory-mapped the file and then copied the entire mapping into a `Vec` to hand to `File::from_bytes` — but `File::open` memory-maps it itself whenever the facade's `mmap` feature is on, which it is by default. So every open mapped the file, memcpy'd all of it, and parsed the copy. Store open at 100k x 384: 455 ms -> 327 ms, about 28% faster (two runs after the change, 326.8 and 328.1 ms). Peak memory is unchanged, which is worth saying because the opposite is the natural assumption. The footprint harness now tracks a high-water mark next to the retained figure, and it shows the peak falling after the parse, during the index build — so a buffer allocated and freed inside the parse never reaches it. Confirmed rather than assumed: holding a deliberate extra copy of the whole file across the parse leaves the peak exactly where it was, which is also what proved the instrument was working before trusting its answer. Co-Authored-By: Claude Opus 5 (1M context) --- BENCHMARKS.md | 25 ++++++++++ CHANGELOG.md | 10 ++++ crates/clawhdf5-agent/src/storage.rs | 16 +++---- .../clawhdf5-bench/src/bin/search_harness.rs | 47 +++++++++++++++---- 4 files changed, 80 insertions(+), 18 deletions(-) diff --git a/BENCHMARKS.md b/BENCHMARKS.md index 7668fb8..edcbdcb 100644 --- a/BENCHMARKS.md +++ b/BENCHMARKS.md @@ -123,6 +123,31 @@ vectors, and recall is measured against brute-force ground truth rather than against the f32 index, whose own approximation errors a re-scored search is entitled to get right. +### Opening a store (`read_from_disk`) + +`HDF5Memory::open` memory-mapped the file, copied the whole mapping into a +`Vec`, and handed that to `File::from_bytes` — while `File::open` memory-maps +the file itself. Dropping the copy takes **store open from 455 ms to 327 ms** +at 100 000 x 384 (`--e2e-only --full`; two runs after the change, 326.8 and +328.1 ms). + +It does **not** lower the process's peak memory, which is worth stating +precisely because it is the obvious thing to assume. The harness now reports a +high-water mark alongside the retained figure: + +| N | reopened MiB | peak during open MiB | +|---:|---:|---:| +| 1 000 | 4 | 5 | +| 10 000 | 44 | 61 | +| 100 000 | 399 | 562 | + +The peak is set *after* the parse, by the index build, so a buffer allocated +and freed during the parse never reaches the high-water mark. Holding a +deliberate extra copy of the file across the whole parse leaves the peak +unmoved, which is how this was confirmed rather than assumed. What the change +saves is the copy itself: a full-file memcpy on every open, and the transient +that goes with it. + ## Read harness Produced by `cargo run --release -p clawhdf5-bench --bin read_harness`: a 4096 x diff --git a/CHANGELOG.md b/CHANGELOG.md index e5bd339..dbb0618 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,16 @@ ## Unreleased +### Performance +- `clawhdf5-agent`: **opening a store is ~28% faster** (455 ms -> 327 ms at + 100k x 384). `read_from_disk` memory-mapped the file and then copied the + entire mapping into a `Vec` for `File::from_bytes`, when `File::open` + memory-maps it directly — so every open paid a full-file memcpy for nothing. + Process peak memory is unchanged: the peak falls after the parse, during the + index build, so the transient never reached the high-water mark. The + footprint harness now reports that peak next to the retained figure, which + is how this was checked rather than assumed. + ### Integrity - `clawhdf5-format`: **Fixed and Extensible Array chunk indexes now verify their checksums** (the `checksum` feature, on by default). Every structure diff --git a/crates/clawhdf5-agent/src/storage.rs b/crates/clawhdf5-agent/src/storage.rs index 533bad3..7ced239 100644 --- a/crates/clawhdf5-agent/src/storage.rs +++ b/crates/clawhdf5-agent/src/storage.rs @@ -113,13 +113,11 @@ pub type StoreState = (MemoryConfig, MemoryCache, SessionCache, KnowledgeCache); /// [`read_from_disk`], plus the checkpoint's [`WalMark`] (if any) so the /// caller can skip WAL entries this file already contains. pub fn read_from_disk_with_mark(path: &Path) -> Result<(StoreState, Option), MemoryError> { - let mmap = clawhdf5_io::MmapReader::open(path).map_err(MemoryError::Io)?; - - // Advise the OS we'll need the whole file for parsing - mmap.advise_willneed(0, mmap.len()); - - // Parse the HDF5 file from the mmap'd bytes - let file = clawhdf5::File::from_bytes(mmap.as_bytes().to_vec()) + // `File::open` memory-maps the file itself (the facade's `mmap` feature is + // on by default). Mapping it here and handing over `as_bytes().to_vec()` + // did the same work and then copied the whole store — a second full copy + // of the file, live for the whole parse, on top of the mapping. + let file = clawhdf5::File::open(path) .map_err(|e| MemoryError::Hdf5(format!("cannot open {}: {e}", path.display())))?; let (mut config, cache, sessions, knowledge) = schema::validate_and_load(&file)?; @@ -133,9 +131,7 @@ pub fn read_from_disk_with_mark(path: &Path) -> Result<(StoreState, Option Result<(StoreState, schema::CheckpointMeta), MemoryError> { - let mmap = clawhdf5_io::MmapReader::open(path).map_err(MemoryError::Io)?; - mmap.advise_willneed(0, mmap.len()); - let file = clawhdf5::File::from_bytes(mmap.as_bytes().to_vec()) + let file = clawhdf5::File::open(path) .map_err(|e| MemoryError::Hdf5(format!("cannot open {}: {e}", path.display())))?; let (mut config, cache, sessions, knowledge) = schema::validate_and_load(&file)?; config.path = path.to_path_buf(); diff --git a/crates/clawhdf5-bench/src/bin/search_harness.rs b/crates/clawhdf5-bench/src/bin/search_harness.rs index fabf42c..951f0de 100644 --- a/crates/clawhdf5-bench/src/bin/search_harness.rs +++ b/crates/clawhdf5-bench/src/bin/search_harness.rs @@ -230,13 +230,27 @@ struct CountingAllocator; static LIVE_BYTES: std::sync::atomic::AtomicI64 = std::sync::atomic::AtomicI64::new(0); +/// High-water mark of [`LIVE_BYTES`] since it was last reset. +/// +/// Live bytes at a checkpoint cannot see a buffer that was allocated and +/// freed in between, and that is exactly the shape of a transient copy — +/// which still has to fit in memory while it exists. +static PEAK_BYTES: std::sync::atomic::AtomicI64 = std::sync::atomic::AtomicI64::new(0); + +fn note_peak(live: i64) { + PEAK_BYTES.fetch_max(live, std::sync::atomic::Ordering::Relaxed); +} + // SAFETY: every method forwards to the system allocator with the same layout // it was given, and only adds bookkeeping around it. unsafe impl std::alloc::GlobalAlloc for CountingAllocator { unsafe fn alloc(&self, layout: std::alloc::Layout) -> *mut u8 { let ptr = unsafe { std::alloc::System.alloc(layout) }; if !ptr.is_null() { - LIVE_BYTES.fetch_add(layout.size() as i64, std::sync::atomic::Ordering::Relaxed); + let live = LIVE_BYTES + .fetch_add(layout.size() as i64, std::sync::atomic::Ordering::Relaxed) + + layout.size() as i64; + note_peak(live); } ptr } @@ -249,10 +263,9 @@ unsafe impl std::alloc::GlobalAlloc for CountingAllocator { unsafe fn realloc(&self, ptr: *mut u8, layout: std::alloc::Layout, new_size: usize) -> *mut u8 { let new_ptr = unsafe { std::alloc::System.realloc(ptr, layout, new_size) }; if !new_ptr.is_null() { - LIVE_BYTES.fetch_add( - new_size as i64 - layout.size() as i64, - std::sync::atomic::Ordering::Relaxed, - ); + let delta = new_size as i64 - layout.size() as i64; + let live = LIVE_BYTES.fetch_add(delta, std::sync::atomic::Ordering::Relaxed) + delta; + note_peak(live); } new_ptr } @@ -266,6 +279,19 @@ fn heap_bytes() -> u64 { LIVE_BYTES.load(std::sync::atomic::Ordering::Relaxed).max(0) as u64 } +/// Start watching for a new high-water mark from the current live total. +fn reset_peak() { + PEAK_BYTES.store( + LIVE_BYTES.load(std::sync::atomic::Ordering::Relaxed), + std::sync::atomic::Ordering::Relaxed, + ); +} + +/// The highest live total seen since [`reset_peak`]. +fn peak_bytes() -> u64 { + PEAK_BYTES.load(std::sync::atomic::Ordering::Relaxed).max(0) as u64 +} + fn mib(bytes: u64) -> f64 { bytes as f64 / (1 << 20) as f64 } @@ -580,19 +606,24 @@ fn bench_footprint(n: usize) { let path = mem.config().path.clone(); drop(mem); let before_open = heap_bytes(); + reset_peak(); let reopened = HDF5Memory::open(&path).unwrap(); let after_open = heap_bytes(); let loaded = after_open.saturating_sub(before_open); + // Peak over the open, not just what it leaves behind: a buffer allocated + // and freed during the parse never shows up in the live total. + let peak = peak_bytes().saturating_sub(before_open); drop(reopened); let raw = (n * DIM * 4) as u64; println!( - "| {n} | {:.0} | {:.0} | {:.0} | {:.0} | {:.0} | {:.2}x |", + "| {n} | {:.0} | {:.0} | {:.0} | {:.0} | {:.0} | {:.0} | {:.2}x |", mib(raw), mib(after_entries.saturating_sub(base)), mib(after_store.saturating_sub(after_entries)), mib(after_indexes.saturating_sub(after_store)), mib(loaded), + mib(peak), loaded as f64 / raw as f64, ); } @@ -644,9 +675,9 @@ fn main() { if args.iter().any(|a| a == "--footprint") { println!("\n### Resident memory, {DIM}-dim f32\n"); println!( - "| N | vectors (raw) | entries MiB | store MiB | indexes MiB | reopened MiB | reopened / raw |" + "| N | vectors (raw) | entries MiB | store MiB | indexes MiB | reopened MiB | peak during open MiB | reopened / raw |" ); - println!("|---:|---:|---:|---:|---:|---:|---:|"); + println!("|---:|---:|---:|---:|---:|---:|---:|---:|"); for &n in sizes { bench_footprint(n); }