fix(storage): cache open HDF5 read handles for multi-bag bag files - #786
fix(storage): cache open HDF5 read handles for multi-bag bag files#786pmrv wants to merge 2 commits into
Conversation
The multi-bag layout introduced in PR #746 made every contains() and list() pay a full HDF5 file open/close (~120 us), regressing contains 8.6x against the per-key layout (240 us vs 28 us, #625). Add a process-wide cache of open read-only h5py handles, keyed by absolute path: a WeakValueDictionary index plus a bounded strong-ref MRU that keeps the most recently used handles alive between operations. Cached handles are validated against an (inode, mtime_ns, size) stat signature on every acquisition, so bags rewritten by other processes or other storage instances are detected and reopened. Handles are opened with locking=False so a long-lived cached reader cannot make writes from other processes fail. Within a process, HDF5 rejects any open whose mode or locking flags differ from a live handle's, so every operation that opens the bag file itself - put, get, evict, and rebag - closes the cached handle first and holds a per-bag in-process lock across the operation; readers hold the same lock while using the handle, so a writer can never close it mid-read. The cache is process-wide rather than per-instance because a handle cached by one instance must be closable by any other instance addressing the same file. contains(hit) drops from 240 us back to the pre-regression ~29 us. evict is unchanged: it genuinely writes, and the filelock acquisition plus HDF5 write open/close are structural for a durable per-key delete inside a shared file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014DWRGyuEoH3qXNSQbSXddp
Benchmark ResultsBaseline: 071d395 (cached) DeltaSignificant changes (|Δ| > 10%): 161 — 79 other rows hiddenValue Storage
Integration
Call Storage
Full resultsCall Storagecalls
Digest
Integrationcompute_heavy
data_heavy
lightweight
Value Storagenested_structures
numpy_arrays
small_strings
|
pmrv
left a comment
There was a problem hiding this comment.
I see, because bag of holding doesn't expose the file handles nor let's us inject them, your 'caching' caches raw h5py handles only to peek inside the bags without bag of holding and invalidates on any writing operation, then actually using the bag of holding API. I don't like this.
Sketch what API changes we'd need from bag of holding to be able to just hold onto bag instances we might reuse.
| # Cap on read-only bag-file handles kept open between operations. Handles are | ||
| # shared process-wide (keyed by absolute path), so this also bounds the | ||
| # process's open-fd contribution regardless of how many storages exist. | ||
| _MAX_OPEN_BAGS = 32 |
There was a problem hiding this comment.
| with self._meta_lock: | ||
| f = self._files.get(key) | ||
| if f is not None and self._signatures.get(key) != signature: | ||
| # Rewritten by another process (or storage instance): safe to | ||
| # close because we hold the bag lock, so no reader is mid-use. | ||
| f.close() | ||
| f = None | ||
| if f is None: | ||
| f = _open_readonly(path) | ||
| with self._meta_lock: | ||
| self._files[key] = f | ||
| self._signatures[key] = signature | ||
| self._recent[key] = f | ||
| self._recent.move_to_end(key) | ||
| while len(self._recent) > _MAX_OPEN_BAGS: | ||
| self._recent.popitem(last=False) |
There was a problem hiding this comment.
does this not expose a race condition between both acquirations of meta_lock?
There was a problem hiding this comment.
No — the invariant that closes it is that every caller of acquire() holds the per-bag lock from lock(path) for the whole call (readers additionally for the duration of their use of the handle). So between the two _meta_lock sections no other thread can acquire or invalidate this path; the only interleaving the dropped _meta_lock admits is activity on other paths, and that never touches this path's entries — MRU eviction only drops other paths' strong references, it doesn't close anything. _meta_lock exists solely to keep the shared dicts internally consistent across different bags; per-path mutual exclusion comes from the bag lock.
4243748 adds a comment in acquire() stating this. The alternative — holding _meta_lock across the h5py open — would also be correct and simpler to reason about, at the cost of serializing concurrent cold opens of different bags (~120 µs each); happy to switch if you prefer the simpler model.
Generated by Claude Code
| @contextlib.contextmanager | ||
| def _bag_writer(self, key: str): | ||
| """Hold *key*'s in-process bag lock across an operation that opens the | ||
| bag file itself, closing any cached read handle first. HDF5 refuses a |
There was a problem hiding this comment.
I think we can make our lives easier by just opening all files in read-write mode, so that cached handles can just be given out to any operation that requests them
There was a problem hiding this comment.
I probed this (h5py 3.16 / HDF5 2.0) and it splits cleanly along the process boundary:
In-process it works exactly as you say. A cached default-flag r+ handle coexists with H5Bag's own r and a opens (verified — HDF5 shares the underlying file object when flags are compatible, which is also why the views stay coherent). All the invalidate-before-write and flag juggling would disappear, and evict/future batched ops could even go through the cached handle directly.
Cross-process it's a trap. A default-locking r+ handle holds HDF5's exclusive lock for as long as it's cached — I verified that another process then fails any open, reads included, with BlockingIOError, until we happen to drop the handle (which for a hot bag is never). Today no handle outlives a single operation, so this would turn "two jobs sharing a cache root" from working into deadlocked-until-timeout. And the escape hatch of r+ with locking=False just reintroduces the same flag-mismatch failure with H5Bag's default-flag opens that the current invalidation dance exists to avoid — verified as well.
So RW handles become a real simplification only once every open goes through us, i.e. once bagofholding can accept an injected handle/group — then one r+, locking=False handle per bag serves everything and cross-process coordination stays with the filelock sidecars, exactly as now. That's the sketch in the main thread. Alternatively, if you're happy declaring multi-bag roots single-process-at-a-time, I can switch to cached default-locking r+ handles now and the code does get meaningfully smaller — your call.
Generated by Claude Code
| with self._bag_reader(p) as f: | ||
| if f is not None: | ||
| keys.extend(Digest(name) for name in f.keys()) | ||
| except OSError as e: |
There was a problem hiding this comment.
f = None only on OSError, in which case this operation here would be skipped, rather than repeated as advised in by bagreaders docstring.
There was a problem hiding this comment.
Right, good catch — fixed in 4243748. Reads now go through _read_bag(file_path, reader), which on a mid-read error drops the broken handle and repeats the read once from a fresh open; only a failure of the fresh open still propagates (the file really is unreadable, matching the old per-open semantics). While testing this I also found that a closed h5py handle silently answers False to key in f instead of raising, so acquire() now additionally validates cached handles via h5py truthiness (not f) and reopens dead ones.
Generated by Claude Code
Review follow-ups on the handle cache: - _MAX_OPEN_BAGS 32 -> 8. - A cached handle that errors mid-read is now dropped and the read repeated once from a fresh open instead of skipping the operation (misreporting a rewritten bag as corrupt/missing); only a failure of the fresh open still propagates. - acquire() validates cached handles with h5py truthiness: a closed handle silently answers False to `key in f` rather than raising, so staleness checks alone would not catch it. - Comment in acquire() documenting why the two _meta_lock sections are race-free (the caller holds the per-bag lock throughout). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014DWRGyuEoH3qXNSQbSXddp
|
Fair characterization — the cache peeks at raw h5py handles for Option A — minimal: handle injection (two changes)
That's sufficient. fleche then keeps exactly one handle per bag file, opened f = cache.acquire(bag_path) # the existing MRU + stat-signature machinery
key in f # contains
f.keys() # list
H5Bag(f[key]).load(...) # get
H5Bag.save(value, f.require_group(key)) # put (+ f.flush())
del f[key] # evict (+ f.flush())Since nothing ever double-opens the file, the whole invalidate-on-write/flag-mismatch dance disappears ( Option B — nicer: a first-class multi-bag containerPromote the multi-bag file to an object that owns the open handle: bags = bagofholding.BagFile(path, mode="r+", locking=False) # long-lived
key in bags; bags.keys()
bags.load(key, version_validator=...)
bags.save(value, key)
del bags[key]
bags.flush(); bags.close()fleche's cache then holds Supporting needs under either option: pass-through of open kwargs (at least Option A is small enough to be one upstream PR and I'd start there; B can grow out of it. Happy to draft the bagofholding side if useful. Status of the review otherwise: cap is 8, reads retry once from a fresh open instead of skipping (plus closed-handle detection), and the Generated by Claude Code |
Benchmark ResultsBaseline: 2c236c0 (cached) DeltaSignificant changes (|Δ| > 10%): 220 — 20 other rows hiddenValue Storage
Integration
Digest
Call Storage
Full resultsCall Storagecalls
Digest
Integrationcompute_heavy
data_heavy
lightweight
Value Storagenested_structures
numpy_arrays
small_strings
|
|
I'd like to see a sketch of an option C as well, where bagofholding does everything transparently and keeps a file open behind a timeout for a while in case of repeated operations. |
Option C — transparent handle pool inside bagofholdingThe public API stays path-based ( # bagofholding internals
_pool = FilePool(max_open=8, idle_timeout=5.0) # both configurable
class FilePool:
def open(self, path, *, write=False) -> ContextManager[h5py.File]: ...
# checkout: refcount += 1; checkin: refcount -= 1, stamp idle deadline
def flush_all(self): ... # close every idle handle now (tests, Windows, rsync)
# every internal open becomes
with _pool.open(path, write=True) as f:
...
f.flush() # writes flush at checkin — durability must not wait for the timeoutKey internals (all four are things this PR already had to solve, so they transplant directly):
What C does not remove: fleche's Trade-offs vs A/B:
My recommendation stands as a sequence rather than a choice: land A's injected-handle constructor (small, unblocks fleche dropping the raw-h5py peeking), grow B's Generated by Claude Code |
|
Will let this sit for a bit and discuss with liam. 300ums per open hurt my pride, but are also not life threatening. |
…theme (#889) Scheduled AGENTS.md audit, 2026-08-23. No code has landed on `main` since the 2026-08-21 audit (63a0584), and the issue/PR state recorded in `agents/DEVELOPING.md` is still current: in-flight PRs #873–#878, #881, #887, and #797 all remain open; no new issues since #883–#886. Spot-checks of the Quick Reference and Architecture claims against `src/` all pass (public `__all__`, `_lazy_default`/`_sticky_set`/`_hard_set`, `PreparedCall`/two-phase save, `register_storage`, `_CACHE_TEMPLATES`, `Runtime.cputime`/`systime`, module line counts quoted in #789/#832). `AGENTS.md` and `agents/USAGE.md` need no changes. One gap found: the performance theme names "pooled file handles in `bagofholding_file.py`" as a fix candidate without noting that draft PR #786 (open since 2026-07-23) already implements it, and PR #804 (open since 2026-07-31) — the benchmark-harness fix that removes the always-evicting `SizeLimitedCache(max_size=10)` config — was recorded nowhere. Both are now listed under the perf theme as check-before-duplicating entries, added as separate paragraphs to keep future edits conflict-free. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01H9uSJpX8SLJrtYbc2ZaSv5 --- _Generated by [Claude Code](https://claude.ai/code/session_01H9uSJpX8SLJrtYbc2ZaSv5)_ Co-authored-by: claude[bot] <claude[bot]@users.noreply.github.com>
…qlFile fsync cheap fix (#898) Weekly AGENTS.md/DEVELOPING.md audit. Only one change since the 2026-08-27 pass (#897): issue #625's perf audit was refreshed the same morning. - Update the perf-audit pointer in `agents/DEVELOPING.md` from "refreshed 2026-08-20" to 2026-08-27 and record the run's verdict (no new source-caused regressions; flagged rows are the chronic `BagOfHoldingH5File` per-op open cost or noise). - Record the still-open SQL-side hot spot the refreshed audit re-flags: `Sql` fsyncs once per key on `save`/`evict` because `_configure_sqlite_pragmas` (`src/fleche/storage/sql.py:190-227`) sets `journal_mode=WAL` but no `synchronous` pragma; the cheap fix (`PRAGMA synchronous=NORMAL`) has been flagged in every audit since 2026-05-07. Verified against the source — the function sets only `foreign_keys` and `journal_mode`. Everything else checked and current: no merges to `main` since #897; open PRs (#873, #874, #887, #892, #894, #896, #797, #786, #804) and issues (#893, #895, #625) are all already recorded. `eisenforschung/landau` was audited in the same pass and needs no update (nothing landed since its 2026-08-25 pass; in-flight PRs #391/#394/#395/#414/#422 unchanged; spot-checked claims hold). --- _Generated by [Claude Code](https://claude.ai/code/session_012SCLy9y7aUR7F44Q7UviGo)_ Co-authored-by: Claude <noreply@anthropic.com>
Fixes the
BagOfHoldingH5Filemulti-bag regression flagged in #625: since PR #746, everycontains()(and each bag inlist()) paid a full HDF5 file open/close, ~8.6× slower than the per-key layout (240 µs vs 28 µs).What changed
bagofholding_file.pygains a process-wide cache of open read-onlyh5py.Filehandles (_BagHandleCache), keyed by absolute bag-file path:WeakValueDictionaryindexes the open handles; a bounded strong-ref MRU (_MAX_OPEN_BAGS = 32) keeps the most recently used ones alive between operations. A handle evicted from the MRU is never closed eagerly — the strong reference is dropped and the interpreter closes the file once the last in-flight user releases it.(inode, mtime_ns, size)stat signature, so bags rewritten by other processes or other storage instances are detected and reopened. One stat (~1 µs) replaces one HDF5 open (~120 µs).locking=False: a long-lived reader holding HDF5's shared OS lock would otherwise make every write from another process fail for as long as it lives. Cross-process write mutual exclusion was always provided by thefilelocksidecar locks, not by HDF5's locking, so nothing is lost.put/_to_file,get/_from_file,_evict,rebag(H5Bag uses default flags) — runs inside_bag_writer(key): it closes the cached handle and holds a per-bag in-process lock across the operation. Readers (_contains,list) hold the same lock while using the handle, so a writer can never close a handle mid-read.WeakKeyDictionarypattern used for lock tables would leave two non-equal instances on the same root able to brick each other's writes.locking=Falsefalls back to a default-flag open on h5py < 3.5 / HDF5 < 1.12.1.Numbers
contains(hit)contains(miss)evictevictis deliberately untouched: it genuinely mutates the bag, so it must pay thefilelockacquisition (~100 µs) plus an HDF5 write open/del/close (~130 µs+). That cost is structural for a durable per-key delete inside a shared file; caching write handles across operations would hold HDF5's exclusive state hostage against every other process. If evict throughput ever matters, the realistic lever is batching (one write open per bag for N evictions, e.g. aevict_manyon the storage or transaction-style deferral), not handle caching.list()— remaining cost and optionslist()now reuses cached handles, so a warm repeatedlist()costs ~0 opens for up to_MAX_OPEN_BAGSbags. But a coldlist()over a root with many bags still opens every file once, and roots with more than 32 bags will churn the MRU. Options, roughly in order of effort:list()on a defaultprefix_length=2root is up to 256 opens (~30 ms worst case).list()on an unchanged root free even beyond the MRU bound (key lists survive handle eviction). Doesn't help the truly cold first call.list()one file read, but adds cost and a consistency obligation to every write, plus a repair story for crashes between bag write and index write. This is the only option that fixes the cold path, and the only one with real correctness surface.prefix_length=0already gives O(readdir)list()today for workloads that are list-heavy and don't hit the many-small-files problem multi-bagging exists to solve — worth remembering before building 3.Happy to follow up with 2 (small) or 3 (needs design agreement) in a separate PR.
Behavioral notes
h5py.File(path)(default flags) in the same process while a cached handle is live will now get the flag-mismatchOSError; go through the storage API, or open withlocking=False.st_mtime_ns: on filesystems with coarse mtime granularity, an external rewrite that changes neither size nor inode within the same timestamp tick could go briefly unnoticed bycontains. Same-process writes are exact (explicit invalidation); cross-process reads were already best-effort._evictitself closes the handle under the bag lock before unlinking, so the backend's own deletes are safe; CI is Linux-only).Testing
contains/list(open counting), invalidation on sibling-keyput,getafter a warmcontains(the flag-mismatch trap), evict with a warm cache including file removal, writes from a second non-equal instance on the same root, external-process writes being neither blocked nor masked (subprocess test — also pinslocking=False), and the MRU bound.ty check src/clean.🤖 Generated with Claude Code
https://claude.ai/code/session_014DWRGyuEoH3qXNSQbSXddp
Generated by Claude Code