diff --git a/crates/clawhdf5-tools/tests/edit_coverage_interop.rs b/crates/clawhdf5-tools/tests/edit_coverage_interop.rs index 036839f..332dffa 100644 --- a/crates/clawhdf5-tools/tests/edit_coverage_interop.rs +++ b/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=' = (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"); +} diff --git a/crates/clawhdf5-tools/tests/edit_interop.rs b/crates/clawhdf5-tools/tests/edit_interop.rs index bc4c9a6..cb345f8 100644 --- a/crates/clawhdf5-tools/tests/edit_interop.rs +++ b/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 -/// never reuses space), against libhdf5 doing the same appends and against -/// `h5repack` of each. Run with `--ignored --nocapture`. +/// Not a check: prints how much space an append workload leaks (one +/// editor for the whole workload, which reuses the space it frees but not +/// 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() { diff --git a/crates/clawhdf5/src/edit/btree2.rs b/crates/clawhdf5/src/edit/btree2.rs index 80be9f0..5761e1e 100644 --- a/crates/clawhdf5/src/edit/btree2.rs +++ b/crates/clawhdf5/src/edit/btree2.rs @@ -316,7 +316,7 @@ impl Bt2 { } fn new_node(&mut self, img: &mut Image<'_>, depth: u16) -> Result { - let addr = img.alloc_reusing(u64::from(self.node_size))?; + let addr = img.alloc(u64::from(self.node_size))?; self.nodes.insert( addr, Node { diff --git a/crates/clawhdf5/src/edit/fheap.rs b/crates/clawhdf5/src/edit/fheap.rs index 7c7b72a..2fab657 100644 --- a/crates/clawhdf5/src/edit/fheap.rs +++ b/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 { 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 { 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; diff --git a/crates/clawhdf5/src/edit/image.rs b/crates/clawhdf5/src/edit/image.rs index 722534d..08d389c 100644 --- a/crates/clawhdf5/src/edit/image.rs +++ b/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); + +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 { + 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 - /// zeros until written. Nothing is ever freed: space an edit stops - /// using (a relocated chunk, say) is leaked, as there is no free-space - /// manager. + /// Allocate `size` bytes: from space an earlier edit of this session + /// freed when a block holds them (best fit), else at the end of the + /// file. The space reads as zeros until written. pub(crate) fn alloc(&mut self, size: u64) -> Result { + 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 { 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 - /// edit of this session freed when there is a block that fits, - /// otherwise at the end of the file. - pub(crate) fn alloc_reusing(&mut self, size: u64) -> Result { - self.alloc(size) - } - - /// Note that the edit no longer uses `[addr, addr + len)`. + /// Note that the edit no longer uses `[addr, addr + len)`; later edits + /// of the session may reuse it. 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 { - Plan { - patches: self.patches, - eoa: self.eoa, - old_eoa: self.old_eoa, + 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>, 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 - /// yet, then a sync; then the changes to existing bytes — raw data - /// overwritten in place and the metadata that links the new space in - /// (superblock end of file, chunk index entries, object header + /// index blocks, relocated structures — past the old end of file, or in + /// space an earlier edit of the session freed), which nothing on disk + /// refers to yet, then a sync; then the changes to existing bytes — raw + /// 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 - /// crash during the second can leave it inconsistent, as with libhdf5 - /// without SWMR: there is no journal. + /// file as it was (plus unreferenced bytes); a crash during the second + /// can leave it inconsistent, as with libhdf5 without SWMR: there is no + /// 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 - // merge); its new part goes with the new space. - let split = old_eoa.saturating_sub(addr).min(bytes.len() as u64) as usize; - let (old, new) = bytes.split_at(split); - if !new.is_empty() { - write_at(file, user_block + addr + split as u64, new)?; - } - if !old.is_empty() { - in_place.push((addr, old)); + // A patch may run across new and existing space (writes + // merge): split it where that changes. + let end = addr + bytes.len() as u64; + let mut at = addr; + while at < end { + 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)); + } + at = upto; } } 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::>(), + [(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::>(), + [(32, 16)] + ); + } + /// Random reads and writes against a flat copy of the bytes. #[test] fn matches_a_flat_model() { diff --git a/crates/clawhdf5/src/edit/mod.rs b/crates/clawhdf5/src/edit/mod.rs index c32431a..336fbb8 100644 --- a/crates/clawhdf5/src/edit/mod.rs +++ b/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 -/// chunks are added to the chunk index: version-1 B-tree (layout v1-v3, -/// what h5py's default `libver` writes), Extensible Array, Fixed Array and -/// single-chunk indexes. A version-2 B-tree index (two or more unlimited -/// dimensions) can only have existing chunks overwritten in place, and an -/// implicit index only in place. -/// - [`resize`](Self::resize): grow a chunked dataset up to its maximum -/// dimensions (h5py's `Dataset.resize`); new chunks come with the writes. +/// in place when it still fits and otherwise in new space. New chunks are +/// added to the chunk index: version-1 B-tree (layout v1-v3, what h5py's +/// default `libver` writes), Extensible Array, Fixed Array, version-2 +/// B-tree (two or more unlimited dimensions) and single-chunk indexes, as +/// libhdf5 adds them (the same splits and blocks). An implicit index can +/// only be written in place. +/// - [`resize`](Self::resize): grow or shrink a chunked dataset within its +/// 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 -/// the file and its old bytes are leaked, as are index blocks that are -/// replaced. `h5repack` reclaims such space. +/// Space an edit stops using — a filtered chunk that moved, chunks a +/// shrink removed, index nodes a B-tree merged away, a heap's replaced +/// 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));