crates/clawhdf5-tools/tests/edit_coverage_interop.rs

@@ -1323,3 +1323,76 @@ fn dense_attribute_refusals_change_nothing() {
     ));
     check_tools(&path, true);
 }
+
+/// Space one edit frees is reused by later edits of the same editor: the
+/// chunks a shrink removes are where the chunks of the following growth
+/// go, so the file does not grow; with a new editor per edit (nothing to
+/// reuse) it does. h5py, h5dump and `h5rs check` read the result, and
+/// h5py goes on.
+#[test]
+fn freed_space_is_reused_within_a_session() {
+    if !tools_ok() {
+        return;
+    }
+    let dir = tmpdir();
+    let mut sizes = Vec::new();
+    for session in [true, false] {
+        let path = dir.path().join(format!("reuse_{session}.h5"));
+        py(&format!(
+            "import h5py, numpy as np\n\
+             with h5py.File({p:?}, 'w', libver='v110') as f:\n\
+             \x20   f.create_dataset('x', data=np.zeros(2000, dtype='<i4'), maxshape=(None,), \
+             chunks=(100,), compression='gzip')\n",
+            p = path.to_str().unwrap()
+        ));
+        let mut rng = Rng(5);
+        let vals: Vec<i32> = (0..2000).map(|_| rng.next() as i32).collect();
+        let mut ed = FileEditor::open(&path).unwrap();
+        ed.write_values("x", &Selection::All, &vals).unwrap();
+        drop(ed);
+        let len0 = std::fs::metadata(&path).unwrap().len();
+        let mut ed = FileEditor::open(&path).unwrap();
+        ed.resize("x", &[1000]).unwrap();
+        if session {
+            assert!(ed.reusable_bytes() > 0);
+        } else {
+            ed = {
+                drop(ed);
+                FileEditor::open(&path).unwrap()
+            };
+        }
+        ed.resize("x", &[2000]).unwrap();
+        ed.write_values("x", &block(&[1000], &[1000]), &vals[1000..])
+            .unwrap();
+        if session {
+            assert_eq!(ed.reusable_bytes(), 0, "every freed chunk is reused");
+        }
+        drop(ed);
+        let len1 = std::fs::metadata(&path).unwrap().len();
+        sizes.push((len0, len1));
+        let m = Model {
+            shape: vec![2000],
+            data: vals.clone(),
+        };
+        verify(&path, "x", &m);
+        check_tools(&path, true);
+        py(&format!(
+            "import h5py, numpy as np\n\
+             with h5py.File({p:?}, 'r+') as f:\n\
+             \x20   f['x'].resize((2100,))\n\
+             \x20   f['x'][2000:] = 9\n",
+            p = path.to_str().unwrap()
+        ));
+        let mut m = m;
+        m.resize(&[2100], 0);
+        m.write_block(&[2000], &[100], &[9; 100]);
+        verify(&path, "x", &m);
+        check_tools(&path, true);
+    }
+    let (reuse, fresh) = (sizes[0], sizes[1]);
+    assert_eq!(
+        reuse.1, reuse.0,
+        "a session reusing freed chunks does not grow the file"
+    );
+    assert!(fresh.1 > fresh.0, "without reuse the file grows");
+}

crates/clawhdf5-tools/tests/edit_interop.rs

@@ -1122,9 +1122,10 @@ fn out_of_order_chunk_creation_matches_libhdf5() {
     }
 }
 
-/// Not a check: prints how much space an append workload leaks (the editor
+/// Not a check: prints how much space an append workload leaks (one
-/// never reuses space), against libhdf5 doing the same appends and against
+/// editor for the whole workload, which reuses the space it frees but not
-/// `h5repack` of each. Run with `--ignored --nocapture`.
+/// space it cannot fit a grown chunk into), against libhdf5 doing the same
+/// appends and against `h5repack` of each. Run with `--ignored --nocapture`.
 #[test]
 #[ignore]
 fn measure_append_waste() {

crates/clawhdf5/src/edit/btree2.rs

@@ -316,7 +316,7 @@ impl Bt2 {
     }
 
     fn new_node(&mut self, img: &mut Image<'_>, depth: u16) -> Result<u64, Error> {
-        let addr = img.alloc_reusing(u64::from(self.node_size))?;
+        let addr = img.alloc(u64::from(self.node_size))?;
         self.nodes.insert(
             addr,
             Node {

crates/clawhdf5/src/edit/fheap.rs

@@ -455,7 +455,7 @@ impl Heap {
     /// blocks, 4 KiB managed objects, 8-byte IDs).
     pub(crate) fn create_attribute_heap(img: &mut Image<'_>) -> Result<Self, Error> {
         let (os, ls) = (img.os, img.ls);
-        let addr = img.alloc_reusing(Self::header_len(os, ls) as u64)?;
+        let addr = img.alloc(Self::header_len(os, ls) as u64)?;
         let mut h = Self {
             addr,
             id_len: 8,
@@ -706,7 +706,7 @@ impl Heap {
     /// heap has none.
     fn fs_add(&mut self, img: &mut Image<'_>, off: u64, size: u64) -> Result<(), Error> {
         if self.fs.is_none() {
-            let addr = img.alloc_reusing(FreeSpace::hdr_len(img.os, img.ls) as u64)?;
+            let addr = img.alloc(FreeSpace::hdr_len(img.os, img.ls) as u64)?;
             self.fs = Some(FreeSpace {
                 addr,
                 max_sect_addr: self.max_index,
@@ -813,7 +813,7 @@ impl Heap {
         size: u64,
     ) -> Result<u64, Error> {
         let os = img.os;
-        let a = img.alloc_reusing(size)?;
+        let a = img.alloc(size)?;
         let n = usize::try_from(size).map_err(|_| bad("block too large"))?;
         let mut d = vec![0u8; n];
         d[0..4].copy_from_slice(b"FHDB");
@@ -851,7 +851,7 @@ impl Heap {
         }
         let have_direct = self.root != undef(os);
         let rows = u16::try_from(nrows).map_err(|_| bad("too many rows"))?;
-        let a = img.alloc_reusing(self.iblock_len(rows, os) as u64)?;
+        let a = img.alloc(self.iblock_len(rows, os) as u64)?;
         self.ents = vec![undef(os); nrows * usize::from(self.width)];
         if have_direct {
             self.ents[0] = self.root;
@@ -889,7 +889,7 @@ impl Heap {
         }
         img.free(self.root, self.iblock_len(self.root_rows, os) as u64);
         let rows = u16::try_from(new).map_err(|_| bad("too many rows"))?;
-        let a = img.alloc_reusing(self.iblock_len(rows, os) as u64)?;
+        let a = img.alloc(self.iblock_len(rows, os) as u64)?;
         let w = usize::from(self.width);
         self.ents.resize(new * w, undef(os));
         let ov = self.overhead(os);
@@ -973,7 +973,7 @@ impl Heap {
             return Err(unsupported("huge-object B-tree layout"));
         }
         let n = obj.len() as u64;
-        let a = img.alloc_reusing(n)?;
+        let a = img.alloc(n)?;
         img.write(a, obj)?;
         let w = usize::from(self.id_len - 1).min(8);
         let max_id = if w >= 8 {
@@ -1096,7 +1096,7 @@ impl Heap {
                         if fs.sect_addr != undef(os) {
                             img.free(fs.sect_addr, fs.alloc_sect_size);
                         }
-                        fs.sect_addr = img.alloc_reusing(need)?;
+                        fs.sect_addr = img.alloc(need)?;
                         fs.alloc_sect_size = need;
                     }
                     fs.sect_size = fs.alloc_sect_size;

crates/clawhdf5/src/edit/image.rs

@@ -35,8 +35,61 @@ pub(crate) struct Image<'a> {
     /// Width of addresses and lengths in the file.
     pub(crate) os: u8,
     pub(crate) ls: u8,
-    /// Space the edit stopped using.
+    /// Space the edit stopped using. Not reused by this edit: until the
+    /// edit is committed, the file's metadata still points at it.
     freed: Vec<(u64, u64)>,
+    /// Space earlier edits of the session freed, available to this one.
+    reusable: FreeList,
+    /// Blocks this edit took from `reusable`: nothing on disk refers to
+    /// them, so they are written with the new space, before the changes
+    /// that link them in (see [`Plan::commit`]).
+    fresh: Vec<(u64, u64)>,
+}
+
+/// Free space, address -> length, adjacent blocks merged.
+#[derive(Debug, Clone, Default)]
+pub(crate) struct FreeList(BTreeMap<u64, u64>);
+
+impl FreeList {
+    /// Add `[addr, addr + len)`, merged with neighbours it touches.
+    pub(crate) fn add(&mut self, addr: u64, len: u64) {
+        if len == 0 {
+            return;
+        }
+        let (mut lo, mut hi) = (addr, addr.saturating_add(len));
+        if let Some((&a, &l)) = self.0.range(..=lo).next_back()
+            && a + l >= lo
+        {
+            lo = a;
+            hi = hi.max(a + l);
+            self.0.remove(&a);
+        }
+        while let Some((&a, &l)) = self.0.range(lo..=hi).next() {
+            hi = hi.max(a + l);
+            self.0.remove(&a);
+        }
+        self.0.insert(lo, hi - lo);
+    }
+
+    /// Take `size` bytes from the smallest block that holds them (the
+    /// lowest address among equals), from its start.
+    fn take(&mut self, size: u64) -> Option<u64> {
+        let (&a, &l) = self
+            .0
+            .iter()
+            .filter(|&(_, &l)| l >= size)
+            .min_by_key(|&(&a, &l)| (l, a))?;
+        self.0.remove(&a);
+        if l > size {
+            self.0.insert(a + size, l - size);
+        }
+        Some(a)
+    }
+
+    /// Total bytes.
+    pub(crate) fn total(&self) -> u64 {
+        self.0.values().sum()
+    }
 }
 
 impl<'a> Image<'a> {
@@ -50,9 +103,23 @@ impl<'a> Image<'a> {
             os,
             ls,
             freed: Vec::new(),
+            reusable: FreeList::default(),
+            fresh: Vec::new(),
         }
     }
 
+    /// Let the edit allocate from `free` (space earlier edits freed).
+    pub(crate) fn with_reusable(mut self, free: FreeList) -> Self {
+        // Only space inside the file as it is now.
+        self.reusable = FreeList(
+            free.0
+                .into_iter()
+                .filter(|&(a, l)| a.saturating_add(l) <= self.old_eoa)
+                .collect(),
+        );
+        self
+    }
+
     pub(crate) fn eoa(&self) -> u64 {
         self.eoa
     }
@@ -66,11 +133,24 @@ impl<'a> Image<'a> {
         !self.patches.is_empty() || self.eoa != self.old_eoa
     }
 
-    /// Allocate `size` bytes at the end of the file. The space reads as
+    /// Allocate `size` bytes: from space an earlier edit of this session
-    /// zeros until written. Nothing is ever freed: space an edit stops
+    /// freed when a block holds them (best fit), else at the end of the
-    /// using (a relocated chunk, say) is leaked, as there is no free-space
+    /// file. The space reads as zeros until written.
-    /// manager.
     pub(crate) fn alloc(&mut self, size: u64) -> Result<u64, Error> {
+        if size > 0
+            && let Some(a) = self.reusable.take(size)
+        {
+            self.fresh.push((a, size));
+            let n = usize::try_from(size)
+                .map_err(|_| Error::Unsupported("allocation too large".into()))?;
+            self.write(a, &vec![0u8; n])?;
+            return Ok(a);
+        }
+        self.alloc_end(size)
+    }
+
+    /// Allocate `size` bytes at the end of the file.
+    fn alloc_end(&mut self, size: u64) -> Result<u64, Error> {
         let addr = self.eoa;
         let end = addr
             .checked_add(size)
@@ -80,14 +160,8 @@ impl<'a> Image<'a> {
         Ok(addr)
     }
 
-    /// Allocate `size` bytes for metadata or data, from space an earlier
+    /// Note that the edit no longer uses `[addr, addr + len)`; later edits
-    /// edit of this session freed when there is a block that fits,
+    /// of the session may reuse it.
-    /// otherwise at the end of the file.
-    pub(crate) fn alloc_reusing(&mut self, size: u64) -> Result<u64, Error> {
-        self.alloc(size)
-    }
-
-    /// Note that the edit no longer uses `[addr, addr + len)`.
     pub(crate) fn free(&mut self, addr: u64, len: u64) {
         if len > 0 {
             self.freed.push((addr, len));
@@ -108,7 +182,7 @@ impl<'a> Image<'a> {
         }
         let old_end = self.eoa;
         self.eoa = addr;
-        if let Err(e) = self.alloc(new_len) {
+        if let Err(e) = self.alloc_end(new_len) {
             self.eoa = old_end;
             return Err(e);
         }
@@ -206,12 +280,24 @@ impl<'a> Image<'a> {
     /// The edit's writes, detached from the base bytes (see the module's
     /// invariant: the reader that owns them can then be dropped before
     /// anything is written).
-    pub(crate) fn into_plan(self) -> Plan {
+    pub(crate) fn into_plan(self) -> (Plan, FreeList) {
+        // What the session may reuse once this edit is committed: what it
+        // did not take, and what it freed.
+        let mut free = self.reusable;
+        for (a, l) in self.freed {
+            free.add(a, l);
+        }
+        let mut fresh = self.fresh;
+        fresh.sort_unstable();
+        (
             Plan {
                 patches: self.patches,
                 eoa: self.eoa,
                 old_eoa: self.old_eoa,
-        }
+                fresh,
+            },
+            free,
+        )
     }
 }
 
@@ -220,33 +306,57 @@ pub(crate) struct Plan {
     patches: BTreeMap<u64, Vec<u8>>,
     eoa: u64,
     old_eoa: u64,
+    /// Reused blocks (sorted): written with the new space.
+    fresh: Vec<(u64, u64)>,
 }
 
 impl Plan {
+    /// Whether `addr` is in space nothing on disk refers to yet (past the
+    /// old end of file, or in a reused block), and up to where (before
+    /// `end`) that stays so.
+    fn new_space(&self, addr: u64, end: u64) -> (bool, u64) {
+        if addr >= self.old_eoa {
+            return (true, end);
+        }
+        let limit = end.min(self.old_eoa);
+        // The reused block holding `addr`, or the next one after it.
+        let i = self.fresh.partition_point(|&(a, l)| a + l <= addr);
+        match self.fresh.get(i) {
+            Some(&(a, l)) if a <= addr => (true, limit.min(a + l)),
+            Some(&(a, _)) => (false, limit.min(a)),
+            None => (false, limit),
+        }
+    }
+
     /// Write the edit to `file`, whose superblock is at `user_block`.
     ///
     /// Order: first everything in newly allocated space (new chunks, new
-    /// index blocks, relocated structures), which nothing on disk refers to
+    /// index blocks, relocated structures — past the old end of file, or in
-    /// yet, then a sync; then the changes to existing bytes — raw data
+    /// space an earlier edit of the session freed), which nothing on disk
-    /// overwritten in place and the metadata that links the new space in
+    /// refers to yet, then a sync; then the changes to existing bytes — raw
-    /// (superblock end of file, chunk index entries, object header
+    /// data overwritten in place and the metadata that links the new space
+    /// in (superblock end of file, chunk index entries, object header
     /// messages) — then a sync. A crash during the first phase leaves the
-    /// file as it was (plus unreferenced bytes past its end of file); a
+    /// file as it was (plus unreferenced bytes); a crash during the second
-    /// crash during the second can leave it inconsistent, as with libhdf5
+    /// can leave it inconsistent, as with libhdf5 without SWMR: there is no
-    /// without SWMR: there is no journal.
+    /// journal.
     pub(crate) fn commit(self, file: &mut std::fs::File, user_block: u64) -> Result<(), Error> {
         let old_eoa = self.old_eoa;
         let mut in_place: Vec<(u64, &[u8])> = Vec::new();
         for (&addr, bytes) in &self.patches {
-            // A patch may run from existing bytes into new space (writes
+            // A patch may run across new and existing space (writes
-            // merge); its new part goes with the new space.
+            // merge): split it where that changes.
-            let split = old_eoa.saturating_sub(addr).min(bytes.len() as u64) as usize;
+            let end = addr + bytes.len() as u64;
-            let (old, new) = bytes.split_at(split);
+            let mut at = addr;
-            if !new.is_empty() {
+            while at < end {
-                write_at(file, user_block + addr + split as u64, new)?;
+                let (new, upto) = self.new_space(at, end);
+                let part = &bytes[(at - addr) as usize..(upto - addr) as usize];
+                if new {
+                    write_at(file, user_block + at, part)?;
+                } else {
+                    in_place.push((at, part));
                 }
-            if !old.is_empty() {
+                at = upto;
-                in_place.push((addr, old));
             }
         }
         if self.eoa > old_eoa {
@@ -322,6 +432,52 @@ mod tests {
         assert!(img.write(42, &[1]).is_err());
     }
 
+    #[test]
+    fn free_list_merges_and_takes_best_fit() {
+        let mut f = FreeList::default();
+        f.add(100, 10);
+        f.add(120, 5);
+        f.add(110, 10); // joins both neighbours
+        assert_eq!(
+            f.0.iter().map(|(&a, &l)| (a, l)).collect::<Vec<_>>(),
+            [(100, 25)]
+        );
+        f.add(300, 8);
+        f.add(200, 40);
+        // Best fit: the 8-byte block for 6 bytes, from its start.
+        assert_eq!(f.take(6), Some(300));
+        assert_eq!(f.take(30), Some(200));
+        assert_eq!(f.take(26), None);
+        assert_eq!(f.total(), 25 + 2 + 10);
+    }
+
+    /// An edit allocates from space earlier edits freed (zeroed), never
+    /// from what it frees itself; the plan writes reused blocks with the
+    /// new space.
+    #[test]
+    fn reuse_across_edits_only() {
+        let base = vec![7u8; 64];
+        let mut free = FreeList::default();
+        free.add(8, 16);
+        let mut img = Image::new(&base, 8, 8).with_reusable(free);
+        img.free(32, 16); // freed by this edit: not reusable yet
+        let a = img.alloc(16).unwrap();
+        assert_eq!(a, 8);
+        assert_eq!(img.read(8, 16).unwrap(), vec![0u8; 16]);
+        let b = img.alloc(8).unwrap();
+        assert_eq!(b, 64, "the edit's own freed space is not reused");
+        img.write(4, &[1; 8]).unwrap(); // existing bytes 4..8, reused 8..12
+        let (plan, next) = img.into_plan();
+        assert_eq!(plan.new_space(4, 12), (false, 8));
+        assert_eq!(plan.new_space(8, 12), (true, 12));
+        assert_eq!(plan.new_space(30, 40), (false, 40));
+        assert_eq!(plan.new_space(64, 72), (true, 72));
+        assert_eq!(
+            next.0.iter().map(|(&a, &l)| (a, l)).collect::<Vec<_>>(),
+            [(32, 16)]
+        );
+    }
+
     /// Random reads and writes against a flat copy of the bytes.
     #[test]
     fn matches_a_flat_model() {

crates/clawhdf5/src/edit/mod.rs

@@ -44,7 +44,7 @@ use btree1::{BTree1, Key};
 use btree2::Bt2;
 use earray::{Ea, EaParams, Elem};
 use farray::Fa;
-use image::{Image, put_uint, undef};
+use image::{FreeList, Image, put_uint, undef};
 use ohdr::Header;
 
 const MSG_DATASPACE: u16 = 0x01;
@@ -74,16 +74,18 @@ const MSG_FLAG_DONTSHARE: u8 = 0x04;
 ///   contiguous or chunked dataset with values of the dataset's own
 ///   datatype, under any selection. Chunks are decoded, updated and
 ///   re-encoded; an unfiltered chunk is rewritten in place, a filtered one
-///   in place when it still fits and otherwise at the end of the file. New
+///   in place when it still fits and otherwise in new space. New chunks are
-///   chunks are added to the chunk index: version-1 B-tree (layout v1-v3,
+///   added to the chunk index: version-1 B-tree (layout v1-v3, what h5py's
-///   what h5py's default `libver` writes), Extensible Array, Fixed Array and
+///   default `libver` writes), Extensible Array, Fixed Array, version-2
-///   single-chunk indexes. A version-2 B-tree index (two or more unlimited
+///   B-tree (two or more unlimited dimensions) and single-chunk indexes, as
-///   dimensions) can only have existing chunks overwritten in place, and an
+///   libhdf5 adds them (the same splits and blocks). An implicit index can
-///   implicit index only in place.
+///   only be written in place.
-/// - [`resize`](Self::resize): grow a chunked dataset up to its maximum
+/// - [`resize`](Self::resize): grow or shrink a chunked dataset within its
-///   dimensions (h5py's `Dataset.resize`); new chunks come with the writes.
+///   maximum dimensions (h5py's `Dataset.resize`), pruning the chunks a
+///   shrink leaves outside the extent as libhdf5 does.
 /// - [`set_attr`](Self::set_attr): add or replace an attribute of any
-///   object whose attributes are stored in its object header.
+///   object, in its object header or in dense storage (moving attributes
+///   there when the object reaches its compact limit).
 ///
 /// Anything else is an [`Error::Unsupported`] and leaves the file untouched.
 ///
@@ -95,13 +97,20 @@ const MSG_FLAG_DONTSHARE: u8 = 0x04;
 /// before that point leaves the file as it was; a crash while the existing
 /// structures are being patched can leave the file inconsistent.
 ///
-/// Space is never reused: a filtered chunk that grows moves to the end of
+/// Space an edit stops using — a filtered chunk that moved, chunks a
-/// the file and its old bytes are leaked, as are index blocks that are
+/// shrink removed, index nodes a B-tree merged away, a heap's replaced
-/// replaced. `h5repack` reclaims such space.
+/// blocks — is reused by later edits of the same editor (best fit, at the
+/// lowest address), never by the edit that freed it: until that edit is
+/// committed the file still refers to it. Space reused this way is written
+/// with the new space, before any existing byte changes. Freed space still
+/// unused when the editor is dropped is lost, as it is when libhdf5 closes
+/// a file without a persistent free-space manager; `h5repack` reclaims it.
 #[derive(Debug)]
 pub struct FileEditor {
     path: PathBuf,
     file: std::fs::File,
+    /// Space edits of this session freed, which later ones reuse.
+    free: FreeList,
 }
 
 /// Where a layout message keeps the fields an edit may change (offsets in
@@ -754,7 +763,11 @@ impl FileEditor {
             }
             Err(TryLockError::Error(e)) => return Err(Error::Io(e)),
         }
-        let ed = Self { path, file };
+        let ed = Self {
+            path,
+            file,
+            free: FreeList::default(),
+        };
         let f = File::open(&ed.path)?;
         check_editable(&f)?;
         Ok(ed)
@@ -765,6 +778,12 @@ impl FileEditor {
         &self.path
     }
 
+    /// Bytes earlier edits of this editor freed that later ones can still
+    /// reuse.
+    pub fn reusable_bytes(&self) -> u64 {
+        self.free.total()
+    }
+
     /// Plan an edit over the file's current bytes, then commit it.
     ///
     /// The reader (a memory map of the file, with the `mmap` feature) is
@@ -779,7 +798,8 @@ impl FileEditor {
         check_editable(&f)?;
         let sb = f.superblock().clone();
         let user_block = f.user_block_size();
-        let mut img = Image::new(f.as_bytes(), sb.offset_size, sb.length_size);
+        let mut img = Image::new(f.as_bytes(), sb.offset_size, sb.length_size)
+            .with_reusable(self.free.clone());
         let r = op(&f, &mut img).map_err(unsupported_filter)?;
         let plan = if img.is_dirty() {
             if img.eoa() != img.old_eoa() {
@@ -790,10 +810,14 @@ impl FileEditor {
             None
         };
         drop(f);
-        if let Some(plan) = plan {
+        if let Some((plan, free)) = plan {
             #[cfg(test)]
             tests::note_commit(&self.path);
+            // A commit that fails part-way leaves the file in an unknown
+            // state: reuse nothing after it.
+            self.free = FreeList::default();
             plan.commit(&mut self.file, user_block)?;
+            self.free = free;
         }
         Ok(r)
     }
@@ -1524,7 +1548,7 @@ fn store_chunk(
             })
         }
         _ => {
-            let a = img.alloc_reusing(len)?;
+            let a = img.alloc(len)?;
             img.write(a, &bytes)?;
             if let Some(info) = existing {
                 img.free(info.address, u64::from(info.chunk_size));