Repository navigation
Look up a vlmeta name once per read - #783
Open
FabianIsensee wants to merge 2 commits into
Open
FabianIsensee wants to merge 2 commits into
FabianIsensee wants to merge 2 commits into
Conversation
vlmeta.get_vlmeta() copied the buffer that blosc2_vlmeta_get() mallocs into a bytes object and never freed it, so every vlmeta read (including `name in vlmeta`, `vlmeta[:]` and NDArray attribute reads) leaked the decompressed content. vlmeta.get_names() and to_dict() leaked their array of name pointers in the same way. to_dict() also kept those pointers across its get_vlmeta() calls, which re-sync (free and reload) the vlmetalayers when another handle has rewritten the frame, so it read freed names. It now copies the names (as the bytes keys it returns) and frees the array before reading. meta__getitem__() ignored the return code of blosc2_meta_get(): for a missing name it copied and freed an uninitialized pointer, so `schunk.meta["missing"] = x` (which does not check the name first) aborted the process. It now raises KeyError, or RuntimeError for other errors, and meta__setitem__() passes the name as given so the message names it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DiPFztdpnKMJkzrjT36BuP
vlmeta.get_vlmeta() called blosc2_vlmeta_exists() and then blosc2_vlmeta_get(), which does the same lookup again. Each lookup re-syncs a file-backed frame (frame_check_stale() stats the file and reads the frame header, plus the lock calls with locking), so every vlmeta read, `name in vlmeta` hit and vlmeta.get() hit did that twice. Call blosc2_vlmeta_get() alone. It returns DATA, MEMORY_ALLOC, NULL_POINTER or READ_BUFFER when reading the content fails, which stay RuntimeError; every other error comes from its lookup (name not found, name too long, frame re-sync or lock) and stays KeyError, as when the separate blosc2_vlmeta_exists() call rejected it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DiPFzsfE5i74cBSxZN13M8
This was referenced Nov 6, 2026
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Depends on #771 (stacked; its commit shows here until #771 is merged). Both change the same lines of
vlmeta.get_vlmeta(); only the last commit (Look up a vlmeta name once per read) is new here.vlmeta.get_vlmeta()callsblosc2_vlmeta_exists()and thenblosc2_vlmeta_get(), which starts with the sameblosc2_vlmeta_exists()call. For a file-backed frame every lookup re-syncs the cached vlmetalayers (frame_check_stale()): it stats the file and reads the frame header (a sparse frame opens and closes its index file for that), and withlocking=Trueit also takes and releases the shared lock and reads the lock counter. So everyvlmeta[name], everyname in vlmeta/vlmeta.get(name)that finds the name,vlmeta[:](once per name) and every NDArray attribute read did all of that twice.Syscalls per
vlmeta["small"]on a frame opened withmode="r"(strace):statx+ 2pread-> 1 + 1locking=True: 4flock+ 4pread+ 2statx-> 2 + 2 + 1openat+ 2pread+ 2close(+ 4fcntl) -> 1 + 1 + 1 (+ 2)"missing" in vlmeta) already did one lookup and still do.This PR calls
blosc2_vlmeta_get()alone. The exceptions stay as they were:blosc2_vlmeta_get()returnsDATA,MEMORY_ALLOC,NULL_POINTERorREAD_BUFFERwhen reading the content fails (stillRuntimeError, after re-raising a callback error); every other error comes from its lookup (name not found, name longer thanBLOSC2_METALAYER_NAME_MAXLEN, frame re-sync or lock errors) and is still aKeyError, as when the separateblosc2_vlmeta_exists()call rejected it. (One corner cannot be told apart by return code: a locked handle whose metalayer reload hits a corrupt frame withREAD_BUFFER/DATA/MEMORY_ALLOCnow getsRuntimeErrorinstead ofKeyError.) Two small side effects: withBLOSC_TRACEset, a miss now prints theUser metalayer "..." not found.trace ofblosc2_vlmeta_get(); and a miss costs onegetenv("BLOSC_TRACE")more (that trace macro), see below.Tests (
tests/test_vlmeta.py):test_vlmeta_read_looks_name_up_once(contiguous and sparse): 100 reads through amode="r"handle must take fewer than 150 read syscalls (syscrfrom/proc/self/io); main: 201, this PR: 101. Skipped where/proc/self/iodoes not exist (non-Linux, WASM).test_vlmeta_name_too_long_is_keyerror: a name longer than the maximum isnot inthe vlmeta and raisesKeyError(as before; pins the error mapping). Missing names are already covered by the existingdelete()checks.Full suite: same results as #771 (+3 new tests); fork CI green on every job that ran (macOS got no runner); the only local failures are the 2 b2view tests that need aiohttp.
Perf, µs per call, 1 thread, median over 5 interleaved rounds; "main" is main with #771's commit (measured on main abed944, before #741; the syscall counts above are the same on current main), built twice for the noise floor (within 2%, 4% for NFS sparse frames), reproduced in 3 runs on the nct node and 2 on the workstation. "NFS"/"local (EPYC)": EPYC 9554 node, frames on its NFS home and on local XFS; "local (Ryzen)": Ryzen 9 7950X workstation, ext4. Frames are read through a
blosc2.open(path, mode="r")handle:v["small"]v["big"](100 kB)"small" in vv[:](8 names)v["small"]locking=True,v["small"]"missing" in vv["small"]"missing" in vA read that finds its name saves one whole re-sync: about 2-3 µs on a local disk, and on NFS about 230 µs for a sparse frame (it opens the index file per re-sync) and 300 µs with locking (lock round trips), so those halve. Misses are 1-5% (20-220 ns) slower, mostly from the
getenv()above: with a minimal environment the miss gap is 10-65 ns, with 500 extra environment variables about 300 ns, while reads that find their name do not change.I used Claude Code (Anthropic) to find the double lookup, write the change and the tests, and run the measurements; I reviewed the changes and the numbers.
🤖 Generated with Claude Code