From 8236b0e30ac965dccb1657fdaf78703b38304b04 Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 18:50:23 -0500 Subject: [PATCH] edit: skip heap blocks too small for an attribute, as libhdf5 does An attribute needing a heap block larger than the next one was refused ("skipping blocks too small for an object", "a first object too large for the starting block"); once an object's move to dense storage was refused it refused every new attribute, so 24% of set_attr calls in the review's random workload failed. Following H5HF__hdr_update_iter, H5HF__man_iblock_root_create/_double and H5HF__hdr_skip_blocks, the smaller blocks are now skipped: the iterator moves past them and they become an indirect free section with a first row section (serialized, class 1, as H5HF__sect_indirect_serialize writes it) and ghost normal rows, added as returned space so it merges with a range skipped just before it (H5HF__sect_indirect_merge_row). Later objects that best-fit a row section get a block created there (H5HF__man_iblock_alloc_row / H5HF__sect_indirect_reduce_row: from the start or end of the range, or from its middle, which splits it, with libhdf5's span bookkeeping). Heaps with such sections, as libhdf5 writes them, are now read too (they were refused at open). dense_skipped_blocks_match_libhdf5 drives every path (merge, split, end, last entry, row wrap) on earliest/v110/latest files against libhdf5 doing the same edits one session each; heaps, free sections and index B-trees are equal after every phase. The refusal test now checks the skip against libhdf5 and keeps a real refusal (last object in a block); clawhdf5-written heaps get 1-4 KiB attributes too. The three tests fail on the previous fheap.rs. Random workload refusals: 24% -> 2.2%, all the documented last-object-in-a-block case. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 18 +- .../tests/edit_coverage_interop.rs | 190 +++++- crates/clawhdf5/src/edit/fheap.rs | 609 +++++++++++++++--- crates/clawhdf5/src/edit/mod.rs | 3 +- docs/known-issues.md | 20 +- 5 files changed, 730 insertions(+), 110 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1949400..6b264f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -63,10 +63,20 @@ insertion. The heap is changed as `H5HF` changes it — best-fit free sections from its free-space manager (kept as libhdf5 keeps `FSHD`/ `FSSE`), new direct blocks through the root indirect block (created, - doubled), huge objects through the huge-object B-tree (deleted with the - last huge object), removed objects' space merged back — with libhdf5's - statistics: after the same attribute workload the heap, its free space - and both index B-trees equal libhdf5's. Attributes are encoded as libhdf5 + doubled), blocks too small for an attribute skipped as libhdf5 skips + them (`H5HF__hdr_skip_blocks`: an indirect free section with its row + sections, serialized as libhdf5 serializes them, merged with the range + skipped just before it, and later attributes given skipped blocks from + either end or the middle of a range, which splits it), huge objects + through the huge-object B-tree (deleted with the last huge object), + removed objects' space merged back — with libhdf5's statistics: after + the same attribute workload the heap, its free space and both index + B-trees equal libhdf5's (`dense_skipped_blocks_match_libhdf5` covers + every way of skipping, with libhdf5 doing one edit per session as the + editor does). In a random attribute workload (1-4 KiB attributes among + small ones) 24% of `set_attr` calls were refused before skipping was + implemented; 2.2% are now, all replacements of the last attribute in a + heap block. Attributes are encoded as libhdf5 encodes them for a file h5py opens `r+` (message version 1, 3 for non-ASCII names; simple dataspaces with their maximum dimensions). Still refused: see `docs/known-issues.md`. diff --git a/crates/clawhdf5-tools/tests/edit_coverage_interop.rs b/crates/clawhdf5-tools/tests/edit_coverage_interop.rs index fff256c..c2da7cf 100644 --- a/crates/clawhdf5-tools/tests/edit_coverage_interop.rs +++ b/crates/clawhdf5-tools/tests/edit_coverage_interop.rs @@ -1230,10 +1230,11 @@ fn dense_attributes_on_clawhdf5_files() { let mut ed = FileEditor::open(&path).unwrap(); for i in 0..30u64 { for (k, o) in ["/", "d"].iter().enumerate() { - // Small attributes: a larger one needs a heap block bigger than - // the next (see dense_attribute_refusals_change_nothing). + // Strings up to 300 bytes and of 5000 (huge objects), and + // every tenth one of 1-4 KiB (a heap block larger than the + // next: blocks skipped). let v = match AV::make(i, k as u64 + 7) { - AV::Str(s) if s.len() > 400 => AV::Str(s[..400].to_string()), + AV::Str(s) if i % 10 == 3 => AV::Str(s.repeat(3000 / s.len().max(1) + 1)), v => v, }; ed.set_attr(o, &format!("n{i}"), &v.value()) @@ -1323,10 +1324,12 @@ fn check_root_and_attrs(path: &Path, want: &[AttrOp]) { assert!(o.status.success(), "attribute check failed:\n{}", text(&o)); } -/// What the editor refuses in dense storage — an object larger than the -/// next heap block (libhdf5 would skip blocks and record their space as -/// free, which this editor does not do) — is `Error::Unsupported`, and the -/// file is left byte for byte as it was. +/// An attribute that needs a heap block larger than the next one: libhdf5 +/// skips the smaller blocks (`H5HF__hdr_skip_blocks`) and records them as +/// an indirect free section, and so does the editor — the heap and its +/// free space come out as libhdf5's. Replacing that attribute, the only +/// object in its block, with one of another size is still refused (libhdf5 +/// frees the block), and the refusal leaves the file byte-identical. #[test] fn dense_attribute_refusals_change_nothing() { if !tools_ok() { @@ -1334,16 +1337,29 @@ fn dense_attribute_refusals_change_nothing() { } let dir = tmpdir(); let path = dir.path().join("dense_refuse.h5"); - py(&format!( - "import h5py, numpy as np\n\ - with h5py.File({p:?}, 'w', libver='v110') as f:\n\ - \x20 g = f.create_group('g')\n\ - \x20 for i in range(12): g.attrs[f'k{{i}}'] = i\n", - p = path.to_str().unwrap() - )); + let twin = dir.path().join("dense_refuse_h5py.h5"); + for p in [&path, &twin] { + py(&format!( + "import h5py, numpy as np\n\ + with h5py.File({p:?}, 'w', libver='v110') as f:\n\ + \x20 g = f.create_group('g')\n\ + \x20 for i in range(12): g.attrs[f'k{{i}}'] = i\n", + p = p.to_str().unwrap() + )); + } + let big = AV::Str("x".repeat(2000)); + let mut ed = FileEditor::open(&path).unwrap(); + ed.set_attr("g", "big", &big.value()).unwrap(); + drop(ed); + py_r_plus( + &twin, + &[format!("\x20 f['g'].attrs.create('big', {})\n", big.py())], + ); + assert_eq!(dense_info(&path, "g"), dense_info(&twin, "g")); + check_tools(&path, true); let before = std::fs::read(&path).unwrap(); let mut ed = FileEditor::open(&path).unwrap(); - unsupported(ed.set_attr("g", "big", &clawhdf5::AttrValue::String("x".repeat(2000)))); + unsupported(ed.set_attr("g", "big", &clawhdf5::AttrValue::String("y".repeat(1500)))); drop(ed); assert!( std::fs::read(&path).unwrap() == before, @@ -1353,13 +1369,155 @@ fn dense_attribute_refusals_change_nothing() { py(&format!( "import h5py\n\ with h5py.File({p:?}, 'r+') as f:\n\ - \x20 f['g'].attrs['big'] = 'x' * 2000\n\ + \x20 f['g'].attrs['big'] = 'y' * 1500\n\ \x20 assert len(f['g'].attrs) == 13\n", p = path.to_str().unwrap() )); check_tools(&path, true); } +/// Attributes of every size up to the heap's 4 KiB managed limit, in an +/// order that makes libhdf5 skip heap blocks in every way it does: a first +/// attribute too large for the heap's starting block (at the move to dense +/// storage), a block larger than the rest of the current row, a root +/// indirect block doubled past rows, two skipped ranges merged; later small +/// attributes go into the skipped blocks (from either end of a skipped +/// range, and from its middle, which splits it). libhdf5 does the same +/// operations, one file session each (as the editor re-reads the heap for +/// each edit); after every phase the heaps, their free sections (single, +/// row and indirect) and index B-trees must be libhdf5's. +#[test] +fn dense_skipped_blocks_match_libhdf5() { + if !tools_ok() { + return; + } + for (libver, h5dump) in [("'earliest'", true), ("'v110'", true), ("'latest'", false)] { + let dir = tmpdir(); + let a = dir.path().join("skip_h5py.h5"); + let b = dir.path().join("skip_edit.h5"); + for p in [&a, &b] { + py(&format!( + "import h5py, numpy as np\n\ + with h5py.File({p:?}, 'w', libver={libver}) as f:\n\ + \x20 objs = [f.create_group('g'), f.create_group('t', track_order=True), \ + f.create_dataset('d', data=np.arange(4, dtype=' = Vec::new(); + for (k, o) in objs.iter().enumerate() { + want.push(( + o.to_string(), + "c0".into(), + AV::Str("s".repeat(1200 + 700 * k)), + )); + for i in 1..8i64 { + want.push((o.to_string(), format!("c{i}"), AV::Ints(vec![i]))); + } + } + for i in 0..8i64 { + want.push(("h".into(), format!("c{i}"), AV::Ints(vec![i]))); + } + // "h" starts with small attributes, so its heap has a root direct + // block and then a one-row root indirect block when the first + // large attribute comes: the rest of that row and the rows the + // doubling adds are skipped as two ranges, which merge; the next + // attributes take blocks from the start, the end and the middle + // (splitting it) of the merged range, and use up whole rows. + let h_phases: [&[usize]; 4] = [ + &[0; 20], + &[4060, 1500, 0, 3000, 0, 1500, 0, 600], + &[1000, 0, 700, 3000, 0, 0, 1500, 0, 900, 0, 0, 0, 1800], + &[0, 800, 0, 0, 0, 2000, 0, 0, 0, 0, 0, 0, 3500, 0, 0, 0, 0, 0], + ]; + // Sizes (string lengths, or 0 for a small int array) in phases. + let phases: [&[usize]; 4] = [ + &[0, 3000, 0, 700, 3900, 0, 150], + &[2500, 90, 0, 3500, 1800, 60, 3990, 0], + &[0, 0, 40, 300, 0, 900, 20, 0, 0, 500, 0, 30, 1000, 0, 0, 200], + &[0, 250, 0, 40, 600, 0, 0, 1100, 80, 0, 0, 350, 0, 0, 0, 10], + ]; + let mut n = 0; + let value = |len: usize, n: usize, k: usize| { + if len == 0 { + AV::Ints((0..1 + (n + k) as i64 % 7).collect()) + } else { + AV::Str( + (0..len + 11 * k) + .map(|j| (b'a' + ((j + n) % 26) as u8) as char) + .collect(), + ) + } + }; + for (ph, sizes) in phases.iter().enumerate() { + let mut ops: Vec = Vec::new(); + for &len in *sizes { + for (k, o) in objs.iter().enumerate() { + ops.push((o.to_string(), format!("n{n}"), value(len, n, k))); + } + n += 1; + } + for &len in h_phases[ph] { + ops.push(("h".into(), format!("n{n}"), value(len, n, 0))); + n += 1; + } + // libhdf5: one session per operation. + let lines: Vec = ops + .iter() + .map(|(o, nm, v)| { + format!( + "with h5py.File({p:?}, 'r+') as f: f[{o:?}].attrs.create({nm:?}, {})\n", + v.py(), + p = a.to_str().unwrap() + ) + }) + .collect(); + let sp = a.with_extension(format!("ops{ph}.py")); + std::fs::write(&sp, format!("import h5py, numpy as np\n{}", lines.concat())).unwrap(); + let out = Command::new(python()).arg(&sp).output().unwrap(); + assert!( + out.status.success(), + "h5py workload failed:\n{}", + text(&out) + ); + let mut ed = FileEditor::open(&b).unwrap(); + for (o, nm, v) in &ops { + ed.set_attr(o, nm, &v.value()) + .unwrap_or_else(|e| panic!("{libver} phase {ph}: set {o}/{nm}: {e}")); + set_want(&mut want, o, nm, v.clone()); + } + drop(ed); + for o in objs.iter().chain(&["h"]) { + assert_eq!( + dense_info(&b, o), + dense_info(&a, o), + "{libver}: dense storage of {o} differs from libhdf5's after phase {ph}" + ); + } + check_tools(&b, h5dump); + check_attr_values(&b, &want); + } + // libhdf5 goes on with the editor's file. + py(&format!( + "import h5py, numpy as np\n\ + with h5py.File({p:?}, 'r+') as f:\n\ + \x20 for o in ['g', 't', 'd', 'h']:\n\ + \x20 for i in range(6): f[o].attrs[f'late{{i}}'] = 'z' * (i * 700 + 5)\n\ + \x20 del f[o].attrs['c1']\n", + p = b.to_str().unwrap() + )); + want.retain(|(_, nm, _)| nm != "c1"); + check_tools(&b, h5dump); + check_attr_values(&b, &want); + } +} + /// 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 diff --git a/crates/clawhdf5/src/edit/fheap.rs b/crates/clawhdf5/src/edit/fheap.rs index 03e6b1e..dfd3af5 100644 --- a/crates/clawhdf5/src/edit/fheap.rs +++ b/crates/clawhdf5/src/edit/fheap.rs @@ -3,23 +3,29 @@ //! best-fitting free section the heap's free-space manager records (a new //! direct block when none fits: the root direct block of an empty heap, the //! next block of the root indirect block — created from the root direct -//! block, and doubled, as needed), huge objects (larger than the heap's +//! block, and doubled, as needed — with the blocks too small for the object +//! skipped and kept as free space for later objects), huge objects (larger than the heap's //! managed maximum) into their own file space tracked by the huge-object //! version-2 B-tree; removed objects return their space to the free-space //! manager, merged with adjacent free space. //! //! The heap's free-space manager (`FSHD` header, `FSSE` section info) is -//! kept exactly as libhdf5 keeps it: sections sorted by size then offset, +//! kept exactly as libhdf5 keeps it: single sections and the indirect +//! sections of skipped blocks (with their first and normal row sections), +//! sections sorted by size then offset, //! the header's counts and sizes, section info moved when its size changes, //! the manager deleted when it tracks nothing. Header statistics (managed //! space, allocated space, free space, allocation iterator, object counts) //! follow libhdf5's arithmetic. //! +//! The heap is read afresh for each edit, so the editor does what libhdf5 +//! does with one edit per file session (libhdf5 keeps an in-memory span per +//! indirect section that it recomputes when it reloads the section info). +//! //! What libhdf5 would do differently is refused ([`Error::Unsupported`], //! before anything is written): I/O filters on the heap, child indirect -//! blocks, skipped blocks (a first object too large for the next block), -//! free sections other than "single" ones, freeing a whole direct block, -//! directly addressed huge objects, tiny objects. +//! blocks (and free space in them), freeing a whole direct block, directly +//! addressed huge objects, tiny objects. use std::collections::{BTreeMap, BTreeSet}; @@ -67,8 +73,129 @@ const FS_NCLASSES: u16 = 4; const HUGE_BT2_TYPE: u8 = 1; const HUGE_BT2_NODE: u32 = 512; -/// The heap's free-space manager: only "single" sections (free space inside -/// a direct block), keyed by heap offset. +/// The heap's doubling table as its free sections need it: the block size +/// of each direct row, the direct block overhead, the width and the size +/// of a heap offset. +#[derive(Clone, Debug)] +struct Geo { + width: usize, + /// Block size of rows 0 .. max direct rows. + rblock: Vec, + /// `H5HF_MAN_ABS_DIRECT_OVERHEAD`. + ov: u64, + heap_off_size: usize, +} + +impl Geo { + /// Free space in a block of `row` (a row section's size). + fn rfree(&self, row: usize) -> u64 { + self.rblock[row] - self.ov + } + + /// Heap offset of `row`'s first block in the root indirect block. + fn row_off(&self, row: usize) -> u64 { + (0..row).map(|r| self.rblock[r] * self.width as u64).sum() + } + + /// `H5HF__dtable_span_size`. + fn span(&self, row: usize, col: usize, n: usize) -> u64 { + let w = self.width; + let end = row * w + col + n - 1; + let (end_row, end_col) = (end / w, end % w); + if row == end_row { + return self.rblock[row] * (end_col - col + 1) as u64; + } + let mut r = row; + let mut acc = 0; + if col > 0 { + acc = self.rblock[r] * (w - col) as u64; + r += 1; + } + while r < end_row { + acc += self.rblock[r] * w as u64; + r += 1; + } + acc + self.rblock[r] * (end_col + 1) as u64 + } +} + +/// A row section (`H5HF_FSPACE_SECT_FIRST_ROW` / `NORMAL_ROW`): `n` +/// unallocated direct blocks of one row of the root indirect block, from +/// column `col`; its size is one block's free space. +#[derive(Clone, Debug)] +struct RowSect { + addr: u64, + row: usize, + col: usize, + n: usize, +} + +/// An indirect section (`H5HF_FSPACE_SECT_INDIRECT`): `n` unallocated +/// entries of the root indirect block from (`row`, `col`) — blocks skipped +/// over to allocate a larger one (`H5HF__hdr_skip_blocks`). It is never in +/// the free-space manager itself; its row sections are, the first as the +/// serialized "first row" (which stands for the indirect section on disk), +/// the others as unserialized "ghost" normal rows. `span` is kept as +/// libhdf5 keeps it (reducing a row subtracts the row section's size, not +/// the block's), since merging compares it. +#[derive(Clone, Debug)] +struct Ind { + addr: u64, + row: usize, + col: usize, + n: usize, + span: u64, + rows: Vec, +} + +impl Ind { + /// `H5HF__sect_indirect_new` + `H5HF__sect_indirect_init_rows` for the + /// root indirect block (direct rows only). + fn new(geo: &Geo, row: usize, col: usize, n: usize) -> Self { + let w = geo.width; + let end = row * w + col + n - 1; + let (end_row, end_col) = (end / w, end % w); + let addr = geo.row_off(row) + geo.rblock[row] * col as u64; + let mut rows = Vec::new(); + let mut entries = if row == end_row { + end_col - col + 1 + } else { + w - col + }; + let mut row_col = col; + let mut off = addr; + for u in row..=end_row { + rows.push(RowSect { + addr: off, + row: u, + col: row_col, + n: entries, + }); + off += entries as u64 * geo.rblock[u]; + entries = if u + 1 < end_row { w } else { end_col + 1 }; + row_col = 0; + } + Self { + addr, + row, + col, + n, + span: geo.span(row, col, n), + rows, + } + } +} + +/// A free section `find` chose. +enum Found { + Single(u64, u64), + /// Row `.1` of indirect section `.0`. + Row(usize, usize), +} + +/// The heap's free-space manager: "single" sections (free space inside a +/// direct block) keyed by heap offset, and the indirect sections (with +/// their row sections) of skipped direct blocks. struct FreeSpace { addr: u64, max_sect_addr: u16, @@ -80,6 +207,8 @@ struct FreeSpace { sect_size: u64, alloc_sect_size: u64, sects: BTreeMap, + inds: Vec, + geo: Geo, } impl FreeSpace { @@ -87,7 +216,7 @@ impl FreeSpace { 6 + 4 * ls as usize + 8 + ls as usize + os as usize + 2 * ls as usize + 4 } - fn open(img: &Image<'_>, addr: u64) -> Result { + fn open(img: &Image<'_>, addr: u64, geo: &Geo) -> Result { let (os, ls) = (img.os, img.ls); let len = Self::hdr_len(os, ls); let d = img.read(addr, len)?; @@ -120,9 +249,6 @@ impl FreeSpace { let sect_addr = next(os as usize); let sect_size = next(l); let alloc_sect_size = next(l); - if ghost != 0 || tot_count != serial { - return Err(unsupported("free space in row or indirect sections")); - } let mut fs = Self { addr, max_sect_addr, @@ -134,8 +260,13 @@ impl FreeSpace { sect_size, alloc_sect_size, sects: BTreeMap::new(), + inds: Vec::new(), + geo: geo.clone(), }; if serial == 0 { + if tot_count != 0 || ghost != 0 || tot_space != 0 { + return Err(bad("free-space counts disagree")); + } return Ok(fs); } if sect_addr == undef(os) || sect_size < 9 + os as u64 || sect_size > alloc_sect_size { @@ -154,9 +285,10 @@ impl FreeSpace { let cnt_w = enc_size(serial); let len_w = enc_size(max_sect_size); let off_w = (usize::from(max_sect_addr)).div_ceil(8); + let ind_w = geo.heap_off_size + 6; let mut q = 5 + os as usize; let mut seen = 0u64; - let mut total = 0u64; + let mut addrs = BTreeSet::new(); while seen < serial { if q + cnt_w + len_w > n - 4 { return Err(bad("section info ends early")); @@ -165,8 +297,8 @@ impl FreeSpace { q += cnt_w; let size = le(&s[q..q + len_w]); q += len_w; - if count == 0 || size == 0 { - return Err(bad("empty section size node")); + if count == 0 || size == 0 || count > serial - seen { + return Err(bad("bad section size node")); } for _ in 0..count { if q + off_w + 1 > n - 4 { @@ -175,51 +307,141 @@ impl FreeSpace { let off = le(&s[q..q + off_w]); let class = s[q + off_w]; q += off_w + 1; - if class != 0 { - return Err(unsupported("free space in row or indirect sections")); + match class { + 0 => { + if fs.sects.insert(off, size).is_some() { + return Err(bad("duplicate free section")); + } + } + 1 => { + // H5HF__sect_indirect_deserialize. + if q + ind_w > n - 4 { + return Err(bad("section info ends early")); + } + let iblock_off = le(&s[q..q + geo.heap_off_size]); + let b = &s[q + geo.heap_off_size..]; + let row = usize::from(u16::from_le_bytes([b[0], b[1]])); + let col = usize::from(u16::from_le_bytes([b[2], b[3]])); + let cnt = usize::from(u16::from_le_bytes([b[4], b[5]])); + q += ind_w; + if iblock_off != 0 { + return Err(unsupported("free space in child indirect blocks")); + } + if cnt == 0 || col >= geo.width { + return Err(bad("bad indirect free section")); + } + let end_row = (row * geo.width + col + cnt - 1) / geo.width; + if end_row >= geo.rblock.len() { + return Err(unsupported("free space in child indirect blocks")); + } + let ind = Ind::new(geo, row, col, cnt); + if ind.addr != off || geo.rfree(row) != size { + return Err(bad("bad indirect free section")); + } + fs.inds.push(ind); + } + _ => return Err(bad("free section of an unknown class")), } - if fs.sects.insert(off, size).is_some() { + if !addrs.insert(off) { return Err(bad("duplicate free section")); } seen += 1; - total += size; } } - if total != tot_space { - return Err(bad("free-space total disagrees with its sections")); + let (space, total, ser, gh) = fs.totals(); + if (space, total, ser, gh) != (tot_space, tot_count, serial, ghost) { + return Err(bad("free-space totals disagree with its sections")); } Ok(fs) } + /// Every row section: (address, size, indirect section, row). + fn rows(&self) -> impl Iterator + '_ { + self.inds.iter().enumerate().flat_map(move |(i, ind)| { + ind.rows + .iter() + .enumerate() + .map(move |(r, rs)| (rs.addr, self.geo.rfree(rs.row), i, r)) + }) + } + + /// (total space, sections, serialized sections, ghost sections). + fn totals(&self) -> (u64, u64, u64, u64) { + let nrows: u64 = self.inds.iter().map(|i| i.rows.len() as u64).sum(); + let space = self.sects.values().sum::() + self.rows().map(|r| r.1).sum::(); + let singles = self.sects.len() as u64; + let firsts = self.inds.len() as u64; + (space, singles + nrows, singles + firsts, nrows - firsts) + } + + fn is_empty(&self) -> bool { + self.sects.is_empty() && self.inds.is_empty() + } + + /// The offset of the section nearest `addr` below it (or above it) in + /// the merge list — singles and first rows, by offset — when that + /// section is a first row, the only kind a first row merges with. + fn merge_neighbour(&self, addr: u64, below: bool) -> Option { + let firsts = self.inds.iter().map(|i| (i.addr, true)); + let singles = self.sects.keys().map(|&o| (o, false)); + let all = firsts.chain(singles); + let pick = if below { + all.filter(|e| e.0 < addr).max_by_key(|e| e.0) + } else { + all.filter(|e| e.0 > addr).min_by_key(|e| e.0) + }; + pick.and_then(|(a, first)| first.then_some(a)) + } + /// Best fit (`H5FS__sect_find_node`): the smallest section of at least - /// `size` bytes, the lowest offset among equals. - fn find(&self, size: u64) -> Option<(u64, u64)> { - self.sects + /// `size` bytes, the lowest offset among equals, singles and row + /// sections alike. + fn find(&self, size: u64) -> Option { + let single = self + .sects .iter() .filter(|&(_, &s)| s >= size) - .min_by_key(|&(&o, &s)| (s, o)) - .map(|(&o, &s)| (o, s)) + .map(|(&o, &s)| ((s, o), Found::Single(o, s))); + let rows = self + .rows() + .filter(|r| r.1 >= size) + .map(|(a, s, i, r)| ((s, a), Found::Row(i, r))); + single.chain(rows).min_by_key(|(k, _)| *k).map(|(_, f)| f) + } + + /// The serialized sections, by size then offset: (size, offset, + /// indirect section if a first row). + fn serial(&self) -> Vec<(u64, u64, Option)> { + let mut v: Vec<(u64, u64, Option)> = + self.sects.iter().map(|(&o, &s)| (s, o, None)).collect(); + v.extend( + self.inds + .iter() + .enumerate() + .map(|(i, ind)| (self.geo.rfree(ind.row), ind.addr, Some(i))), + ); + v.sort_unstable(); + v } /// The serialized section info's size (`H5FS__sect_serialize_size`). fn needed(&self, os: u8) -> u64 { - let n = self.sects.len() as u64; let prefix = 4 + 1 + u64::from(os) + 4; + let serial = self.serial(); + let n = serial.len() as u64; if n == 0 { return prefix; } - let sizes: BTreeSet = self.sects.values().copied().collect(); + let sizes: BTreeSet = serial.iter().map(|s| s.0).collect(); prefix + sizes.len() as u64 * (enc_size(n) + enc_size(self.max_sect_size)) as u64 + n * (u64::from(self.max_sect_addr).div_ceil(8) + 1) + + self.inds.len() as u64 * (self.geo.heap_off_size as u64 + 6) } fn serialize(&self, os: u8) -> Vec { - let n = self.sects.len() as u64; - let mut by_size: BTreeMap> = BTreeMap::new(); - for (&o, &s) in &self.sects { - by_size.entry(s).or_default().push(o); - } + let serial = self.serial(); + let n = serial.len() as u64; let cnt_w = enc_size(n); let len_w = enc_size(self.max_sect_size); let off_w = usize::from(self.max_sect_addr).div_ceil(8); @@ -229,13 +451,28 @@ impl FreeSpace { let mut a = vec![0u8; os as usize]; put_uint(&mut a, self.addr, os); d.extend_from_slice(&a); - for (size, offs) in by_size { - d.extend_from_slice(&(offs.len() as u64).to_le_bytes()[..cnt_w]); + let mut i = 0; + while i < serial.len() { + let size = serial[i].0; + let same = serial[i..].iter().take_while(|s| s.0 == size).count(); + d.extend_from_slice(&(same as u64).to_le_bytes()[..cnt_w]); d.extend_from_slice(&size.to_le_bytes()[..len_w]); - for o in offs { + for &(_, o, ind) in &serial[i..i + same] { d.extend_from_slice(&o.to_le_bytes()[..off_w]); - d.push(0); + match ind { + None => d.push(0), + Some(k) => { + // H5HF__sect_indirect_serialize (root block: offset 0). + let ind = &self.inds[k]; + d.push(1); + d.extend_from_slice(&vec![0u8; self.geo.heap_off_size]); + for v in [ind.row, ind.col, ind.n] { + d.extend_from_slice(&(v as u16).to_le_bytes()); + } + } + } } + i += same; } d } @@ -244,13 +481,12 @@ impl FreeSpace { let (os, ls) = (img.os, img.ls); let len = Self::hdr_len(os, ls); let l = ls as usize; - let n = self.sects.len() as u64; - let tot: u64 = self.sects.values().sum(); + let (tot, n, serial, ghost) = self.totals(); let mut d = Vec::with_capacity(len); d.extend_from_slice(b"FSHD"); d.push(0); d.push(FS_CLIENT_FHEAP); - for v in [tot, n, n, 0] { + for v in [tot, n, serial, ghost] { d.extend_from_slice(&v.to_le_bytes()[..l]); } for v in [self.nclasses, self.shrink, self.expand, self.max_sect_addr] { @@ -440,7 +676,7 @@ impl Heap { } } if fs_addr != undef(os) { - let fs = FreeSpace::open(img, fs_addr)?; + let fs = FreeSpace::open(img, fs_addr, &h.geo(os))?; if fs.max_sect_addr != max_index { return Err(bad("free-space manager does not match the heap")); } @@ -678,10 +914,23 @@ impl Heap { return Err(unsupported("tiny objects")); } let (off, size) = match self.fs.as_ref().and_then(|fs| fs.find(n)) { - Some((o, s)) => { + Some(Found::Single(o, s)) => { self.fs.as_mut().expect("found above").sects.remove(&o); (o, s) } + // H5HF__man_iblock_alloc_row: a skipped block, created now. + Some(Found::Row(i, r)) => { + let entry = self.alloc_row(i, r); + self.fs_dirty = true; + let w = usize::from(self.width); + let (row, col) = (entry / w, entry % w); + let bsize = self.row_size(row); + let block_off = self.row_off(row) + bsize * col as u64; + let a = self.dblock_create(img, block_off, bsize)?; + self.ents[entry] = a; + self.iblock_dirty = true; + (block_off + self.overhead(os), bsize - self.overhead(os)) + } None => self.dblock_new(img, n)?, }; // H5HF__sect_single_reduce: the object goes at the section's start. @@ -705,7 +954,16 @@ impl Heap { /// `H5FS_ADD_RETURNED_SPACE`), creating the free-space manager if the /// heap has none. fn fs_add(&mut self, img: &mut Image<'_>, off: u64, size: u64) -> Result<(), Error> { + self.fs_mut(img)?.sects.insert(off, size); + self.fs_dirty = true; + Ok(()) + } + + /// The free-space manager, created if the heap has none + /// (`H5HF__space_start`). + fn fs_mut(&mut self, img: &mut Image<'_>) -> Result<&mut FreeSpace, Error> { if self.fs.is_none() { + let geo = self.geo(img.os); let addr = img.alloc(FreeSpace::hdr_len(img.os, img.ls) as u64)?; self.fs = Some(FreeSpace { addr, @@ -718,18 +976,160 @@ impl Heap { sect_size: 0, alloc_sect_size: 0, sects: BTreeMap::new(), + inds: Vec::new(), + geo, }); self.fs_addr = addr; } - self.fs - .as_mut() - .expect("created above") - .sects - .insert(off, size); + Ok(self.fs.as_mut().expect("created above")) + } + + /// The doubling table's direct rows, for the free sections. + fn geo(&self, os: u8) -> Geo { + Geo { + width: usize::from(self.width), + rblock: (0..self.max_direct_rows) + .map(|r| self.row_size(r)) + .collect(), + ov: self.overhead(os), + heap_off_size: self.heap_off_size, + } + } + + /// `H5HF__hdr_skip_blocks`: `n` entries of the root indirect block + /// from `start` are skipped — the iterator moves past them and they + /// become an indirect free section (`H5HF__sect_indirect_add`), whose + /// first row is added as returned space, so it merges with an indirect + /// section just below it (`H5FS__sect_merge`). + fn skip_blocks(&mut self, img: &mut Image<'_>, start: usize, n: usize) -> Result<(), Error> { + let w = usize::from(self.width); + let (row, col) = (start / w, start % w); + if (start + n - 1) / w >= self.max_direct_rows { + return Err(unsupported("child indirect blocks")); + } + let geo = self.geo(img.os); + self.man_iter_off += geo.span(row, col, n); + let iter = self.man_iter_off; + let fs = self.fs_mut(img)?; + let mut cur = Ind::new(&geo, row, col, n); + loop { + let mut modified = false; + // The nearest sections below and above in the merge list + // (singles and first rows, by offset). + let below = fs.merge_neighbour(cur.addr, true); + let above = fs.merge_neighbour(cur.addr, false); + if let Some(a) = below + && let Some(k) = fs.inds.iter().position(|x| x.addr == a) + && fs.inds[k].addr + fs.inds[k].span == cur.addr + { + let mut lower = fs.inds.remove(k); + if cur.addr >= iter { + return Err(unsupported("free space past the heap's end")); + } + merge_ind(&mut lower, cur, w); + cur = lower; + modified = true; + } + if let Some(a) = above + && let Some(k) = fs.inds.iter().position(|x| x.addr == a) + && cur.addr + cur.span == a + { + let upper = fs.inds.remove(k); + if upper.addr >= iter { + return Err(unsupported("free space past the heap's end")); + } + merge_ind(&mut cur, upper, w); + modified = true; + } + if !modified { + break; + } + } + // H5HF__sect_row_can_shrink: a section past the iterator would + // shrink the heap; skipped blocks never are. + if cur.addr >= iter { + return Err(unsupported("free space past the heap's end")); + } + fs.inds.push(cur); self.fs_dirty = true; Ok(()) } + /// `H5HF__sect_row_reduce` with `H5HF__sect_indirect_reduce_row`: take + /// one block out of row `r` of indirect section `i`; returns the root + /// indirect block entry to create it at. + fn alloc_row(&mut self, i: usize, r: usize) -> usize { + let w = usize::from(self.width); + let fs = self.fs.as_mut().expect("section found"); + let geo = fs.geo.clone(); + let ind = &mut fs.inds[i]; + let rs = ind.rows[r].clone(); + let row_start = rs.row * w + rs.col; + let row_end = row_start + rs.n - 1; + let start = ind.row * w + ind.col; + let end = start + ind.n - 1; + let (start_row, end_row) = (ind.row, end / w); + let from_start = !(row_end == end && start_row != end_row); + let entry = if from_start { row_start } else { row_end }; + ind.span -= geo.rfree(rs.row); + let mut peer = None; + if ind.n > 1 { + if entry == start { + ind.addr += geo.rblock[ind.row]; + ind.col += 1; + if ind.col == w { + ind.row += 1; + ind.col = 0; + // The row's last block: the row goes (below). + } + ind.n -= 1; + } else if entry == end { + ind.n -= 1; + } else { + // Split: the rows before this one become a peer section. + let peer_n = entry - start; + let peer_rows = rs.row - start_row; + let rest = ind.rows.split_off(peer_rows); + let p = Ind { + addr: ind.addr, + row: ind.row, + col: ind.col, + n: peer_n, + span: rs.addr - ind.addr, + rows: std::mem::replace(&mut ind.rows, rest), + }; + ind.addr = rs.addr + geo.rblock[rs.row]; + ind.span -= p.span; + ind.row = rs.row; + ind.col = rs.col + 1; + ind.n -= peer_n + 1; + peer = Some(p); + } + } else { + ind.n -= 1; + } + // The row section itself. + let ri = if peer.is_some() { 0 } else { r }; + let row = &mut ind.rows[ri]; + if row.n == 1 { + ind.rows.remove(ri); + } else { + if from_start { + row.addr += geo.rblock[row.row]; + row.col += 1; + } + row.n -= 1; + } + let gone = ind.rows.is_empty(); + if gone { + fs.inds.remove(i); + } + if let Some(p) = peer { + fs.inds.push(p); + } + entry + } + /// `H5HF__man_dblock_new`: a direct block for an object of `request` /// bytes; returns its free section (not in the free-space manager). fn dblock_new(&mut self, img: &mut Image<'_>, request: u64) -> Result<(u64, u64), Error> { @@ -753,30 +1153,16 @@ impl Heap { self.total_man_free += self.start_block_size - self.overhead(os); return Ok((self.overhead(os), self.start_block_size - self.overhead(os))); } - if self.root == undef(os) { - return Err(unsupported( - "a first object too large for the starting block", - )); - } - // H5HF__hdr_update_iter. - if self.root_rows == 0 { - self.root_create(img, min)?; - } - let (mut row, mut col) = self.iter_pos(); - let min_row = self.size_to_row(min); - if min_row > row && row < usize::from(self.root_rows) { - return Err(unsupported("skipping blocks too small for an object")); - } - while row >= usize::from(self.root_rows) { - self.root_double(img, min)?; - (row, col) = self.iter_pos(); - } + // H5HF__hdr_update_iter, then a block at the iterator. + self.update_iter(img, min)?; + let (row, col) = self.iter_pos(); if row >= self.max_direct_rows { return Err(unsupported("child indirect blocks")); } let size = self.row_size(row); if min > size { - return Err(unsupported("skipping blocks too small for an object")); + // libhdf5: "skipping direct block sizes not supported". + return Err(unsupported("a direct block smaller than the object")); } self.man_iter_off += size; let entry = row * usize::from(self.width) + col; @@ -787,6 +1173,37 @@ impl Heap { Ok((block_off + self.overhead(os), size - self.overhead(os))) } + /// `H5HF__hdr_update_iter`: make the iterator point at a block of at + /// least `min` bytes, creating the root indirect block, skipping + /// smaller blocks and doubling the root indirect block as needed. + fn update_iter(&mut self, img: &mut Image<'_>, min: u64) -> Result<(), Error> { + if self.root_rows == 0 { + return self.root_create(img, min); + } + let w = usize::from(self.width); + let min_row = self.size_to_row(min); + let (mut row, col) = self.iter_pos(); + let nrows = usize::from(self.root_rows); + if min_row > row && row < nrows { + let entry = row * w + col; + let skip = if min_row >= nrows { + nrows * w - entry + } else { + min_row * w - entry + }; + self.skip_blocks(img, entry, skip)?; + row = self.iter_pos().0; + } + while row >= usize::from(self.root_rows) { + self.root_double(img, min)?; + row = self.iter_pos().0; + } + if row >= self.max_direct_rows { + return Err(unsupported("child indirect blocks")); + } + Ok(()) + } + /// The allocation iterator's (row, column) in the root indirect block. fn iter_pos(&self) -> (usize, usize) { if self.man_iter_off >= self.man_size { @@ -827,8 +1244,9 @@ impl Heap { Ok(a) } - /// `H5HF__man_iblock_root_create`: the root direct block becomes entry - /// 0 of a new root indirect block. + /// `H5HF__man_iblock_root_create`: a root indirect block, with the + /// root direct block (if any) as entry 0; when the block needed is + /// larger than the starting size, the smaller blocks are skipped. fn root_create(&mut self, img: &mut Image<'_>, min: u64) -> Result<(), Error> { let os = img.os; let mut nrows = if self.start_root_rows == 0 { @@ -846,9 +1264,6 @@ impl Heap { if nrows > self.max_direct_rows { return Err(unsupported("child indirect blocks")); } - if min > self.start_block_size { - return Err(unsupported("skipping blocks too small for an object")); - } let have_direct = self.root != undef(os); let rows = u16::try_from(nrows).map_err(|_| bad("too many rows"))?; let a = img.alloc(self.iblock_len(rows, os) as u64)?; @@ -860,43 +1275,57 @@ impl Heap { self.man_iter_off = 0; } self.iblock_dirty = true; - self.root_rows = rows; - self.root = a; let w = u64::from(self.width); let ov = self.overhead(os); let mut acc: u64 = (0..nrows).map(|u| (self.row_size(u) - ov) * w).sum(); if have_direct { acc -= self.row_size(0) - ov; } + // The header points at the new block before the skipped blocks + // are added (the free sections need the rows in place). + self.root_rows = rows; + self.root = a; + if min > self.start_block_size { + let first = usize::from(have_direct); + self.skip_blocks(img, first, (nrows - 1) * usize::from(self.width) - first)?; + } self.man_size = self.row_off(nrows); self.total_man_free += acc; Ok(()) } /// `H5HF__man_iblock_root_double`: the root indirect block gets twice - /// its rows, at a new address. + /// its rows (at least enough for a block of `min` bytes, the smaller + /// blocks before it skipped), at a new address. fn root_double(&mut self, img: &mut Image<'_>, min: u64) -> Result<(), Error> { let os = img.os; + let w = usize::from(self.width); let old = usize::from(self.root_rows); - let (row, _) = self.iter_pos(); - let next_size = self.row_size(row.min(self.max_root_rows - 1)); + let (next_row, next_col) = self.iter_pos(); + let next_entry = next_row * w + next_col; + let next_size = self.row_size(next_row.min(self.max_root_rows - 1)); + let (mut min_nrows, mut new_next_entry, mut skip) = (0, 0, false); if old < self.max_direct_rows && min > next_size { - return Err(unsupported("skipping blocks too small for an object")); + skip = true; + min_nrows = 1 + self.size_to_row(min); + new_next_entry = (min_nrows - 1) * w; } - let new = (2 * old).min(self.max_root_rows); + let new = min_nrows.max((2 * old).min(self.max_root_rows)); if new > self.max_direct_rows || new == old { return Err(unsupported("child indirect blocks")); } 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(self.iblock_len(rows, os) as u64)?; - let w = usize::from(self.width); self.ents.resize(new * w, undef(os)); + self.root = a; + self.iblock_dirty = true; + if skip { + self.skip_blocks(img, next_entry, new_next_entry - next_entry)?; + } let ov = self.overhead(os); let acc: u64 = (old * w..new * w).map(|u| self.row_size(u / w) - ov).sum(); self.root_rows = rows; - self.root = a; - self.iblock_dirty = true; self.man_size = 2 * self.row_off(new - 1); self.total_man_free += acc; Ok(()) @@ -1091,7 +1520,7 @@ impl Heap { self.fs_dirty = false; if let Some(mut fs) = self.fs.take() { let hdr_len = FreeSpace::hdr_len(os, img.ls) as u64; - if fs.sects.is_empty() { + if fs.is_empty() { // H5HF__space_close: a manager that tracks nothing is // deleted. img.free(fs.addr, hdr_len); @@ -1124,6 +1553,24 @@ impl Heap { } } +/// `H5HF__sect_indirect_merge_row` for two sections of the root indirect +/// block, `b` right after `a`: `b`'s rows join `a` (its first row joins +/// `a`'s last row when they share a row). +fn merge_ind(a: &mut Ind, mut b: Ind, w: usize) { + let end_row1 = (a.row * w + a.col + a.n - 1) / w; + if !b.rows.is_empty() { + if end_row1 == b.row { + let first = b.rows.remove(0); + if let Some(last) = a.rows.last_mut() { + last.n += first.n; + } + } + a.rows.append(&mut b.rows); + } + a.n += b.n; + a.span += b.span; +} + /// The ID in a huge-object B-tree record (type 1: address, length, ID). fn huge_rec_id(r: &[u8], os: u8, ls: u8) -> u64 { get_uint(&r[os as usize + ls as usize..], ls) diff --git a/crates/clawhdf5/src/edit/mod.rs b/crates/clawhdf5/src/edit/mod.rs index e1d301a..a5a3c1b 100644 --- a/crates/clawhdf5/src/edit/mod.rs +++ b/crates/clawhdf5/src/edit/mod.rs @@ -958,8 +958,7 @@ impl FileEditor { /// /// [`Error::Unsupported`] for shared attribute messages, heaps this /// editor cannot extend the way libhdf5 would (child indirect blocks, - /// skipped blocks, free space other than within direct blocks, freeing - /// a whole block), and version-1 object headers asked for an attribute + /// freeing a whole block), and version-1 object headers asked for an attribute /// larger than a header message holds. pub fn set_attr(&mut self, path: &str, name: &str, value: &AttrValue) -> Result<(), Error> { if name.is_empty() { diff --git a/docs/known-issues.md b/docs/known-issues.md index a178d47..2539f63 100644 --- a/docs/known-issues.md +++ b/docs/known-issues.md @@ -74,13 +74,19 @@ reuse were added). `clawhdf5::FileEditor` refuses, with an optional filter only when its own build lacks it, which none does for these; - attributes in dense storage when the heap cannot take them the way - libhdf5 would: an attribute that needs a heap block larger than the next - one (libhdf5 skips blocks and records them as free space — in practice an - attribute of roughly 1 to 4 KiB going into a young heap), a heap with I/O - filters or child indirect blocks (more than about 512 KiB of attributes), - free space the heap tracks outside direct blocks, replacing the last - attribute left in a heap block by one of another size (libhdf5 frees the - block), directly addressed huge objects; and shared attribute messages; + libhdf5 would: replacing the last attribute left in a heap block by one + of another size (libhdf5 frees the block), a heap with I/O filters or + child indirect blocks (more than about 512 KiB of attributes), free + space in child indirect blocks, directly addressed huge objects; and + shared attribute messages. Measured 2026-09-26 on tank with the review's + random-edit harness (120 runs of 150 random edits, `earliest`/`v110`/ + `latest`, about 5600 `set_attr` calls of 8 bytes to 6 KiB): 2.2% of + `set_attr` calls are refused, every one the last-attribute-in-a-block + replacement; before blocks could be skipped (an attribute needing a heap + block larger than the next one — any attribute of about 1 KiB or more + once a heap has started, or at the move to dense storage), 24% were, + since an object whose move to dense storage was refused kept refusing + every new attribute; - version-1 object headers asked for an attribute larger than a header message (they have no dense storage); - partial edge chunks stored unfiltered (`H5Pset_chunk_opts`), external