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); }