Merge perf/mmap-open: open a store without copying the whole file

read_from_disk mapped the file and then copied the whole mapping for
File::from_bytes, which maps it itself. Store open is ~28% faster
(455 ms -> 327 ms at 100k x 384); peak memory is unchanged, because the
peak falls after the parse during the index build.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

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

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

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<WalMark>), 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<WalMa
 pub fn read_from_disk_with_meta(
     path: &Path,
 ) -> 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();

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