Repository navigation
Resolve basic keys in Cython and lower per-call overhead (part 2/4 of #744) - #747
Open
FabianIsensee wants to merge 12 commits into
Open
FabianIsensee wants to merge 12 commits into
FabianIsensee wants to merge 12 commits into
Conversation
NDArray.get_slice_numpy() decompressed with the GIL held, so threads reading separate arrays, or separate handles of one file, ran one at a time; on NFS this included the wait for the server. Release the GIL around b2nd_get_slice_cbuffer(). The GIL was also what kept two threads from using one super-chunk at the same time. Replace the per-NDArray read_lock with one reentrant lock per super-chunk, shared by the SChunk wrapper, the NDArray, its views and vlmeta, and hold it in every entry point into C. Keep the GIL when a Python postfilter, codec or filter could run during the read, and on wasm32. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SwMZj7Wi7Sc8HtQpELyNXB
Hold the operands' handle locks while the miniexpr prefilter of a lazy expression reads their super-chunks from C. Count the reads that run without the GIL, so that exit and the registration of Python codecs and filters wait for them, and free handle locks that other threads held at a fork in the child. Lock the metalayer counters too. Skip the new tests on wasm32, shorten them, retry the GIL check on busy machines, and add tests for lazy expressions and fork. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SwMZj7Wi7Sc8HtQpELyNXB
The miniexpr prefilter read the chunks of every in-memory operand from sc->data, and the fast matmul prefilter did so for every operand. Frames have no chunk table there: in-memory arrays created with contiguous=True, arrays from ndarray_from_cframe() (copy=False, the default) and, for matmul, any on-disk array. Any lazy expression or matmul with such an operand dereferenced a NULL table and crashed the interpreter. Read frame-backed in-memory operands through blosc2_schunk_get_chunk(), as on-disk ones already are, and keep frame-backed operands off the fast matmul path: in its Python checks, and in _set_pref_matmul() itself, which now raises ValueError for them before changing any state. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SwMZj6jNaqsesMKnBde8Lr
The general indexing path (ndindex) raised, or returned a wrong shape, for keys that NumPy indexes fine: 554 of 18000 random keys in a comparison with NumPy. Fix the classes found: - slice bounds beyond the index range, and bool bounds or steps, next to ..., None or NumPy integers raised IndexError or TypeError; normalize them as NumPy reads them (clamped, bools as ints) before ndindex sees them; - 0-d integer arrays index as integers (AttributeError before); - scalar bools are 0-d boolean indices, as in NumPy: their dimension goes among adjacent advanced indices (integers included), else in front, and next to array indices they broadcast (True: no effect, False: an empty result or IndexError). The result shape comes from NumPy on a stride-0 stand-in for the array, so it costs no memory; - None next to (adjacent) array indices is supported; - 0-d arrays take NumPy scalar bools (AttributeError before) and raise IndexError for integer lists (ValueError before). Advanced indices separated by slices, ... or None still raise NotImplementedError (NumPy moves their dimensions to the front there). After this, none of the 18000 keys gives another result than NumPy; the remaining differences are that limitation and which error a key that is invalid for several reasons reports first. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SwMZj6jNaqsesMKnBde8Lr
- update_special() built its special chunk in a temporary SChunk with default cparams, so on an NDArray's super-chunk the chunk header had another block size than the array (e.g. 20000 instead of 800 bytes) and every later read of the array failed: b2nd reads blocks selectively and checks their number per chunk. The new SChunk._special_chunk() makes zero, NaN, uninitialized and repeated-value chunks with the super-chunk's own cparams. - update_data() and insert_data() with copy=False did not free their compressed chunk when the C call failed (nor on a compression error), so it leaked wherever c-blosc2 did not free it itself. They now free it whenever the call fails. This needs the next c-blosc2 release (or the companion c-blosc2 PR) for its caller-keeps-the-chunk-on-failure rule; with an older c-blosc2, a failed frame update can free the chunk twice. - decompress_chunk() without dst fetched the whole compressed chunk only to read its size from the header, then read it again to decompress it; it now gets the lazy chunk (header and block offsets) for the size. astype(float64) of a 300x512x512 float32 array on disk: 385 -> 366 ms. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SwMZj6jNaqsesMKnBde8Lr
LazyExpr._collect_flat_indices_from_bool_ndarray(), used for 1-D masks, took every chunk as chunk_len items, so the padding of the last chunk could yield positions past the end of the array: blosc2.full(1000, True, chunks=(300,)) gave 1200 positions. Clip each chunk to the items inside the array. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SwMZj6jNaqsesMKnBde8Lr
C-Blosc2 uses BLOSC_NTHREADS, when set to a number it accepts, for every compression and decompression context, whatever nthreads the parameters ask for. blosc2.nthreads, and so the CParams/DParams defaults and numexpr's thread count, ignored it: with BLOSC_NTHREADS=4, Python reported 28 threads and gave numexpr 28 while C used 4. Start blosc2.nthreads from what C-Blosc2 gives a compression context (the new blosc2_ext.context_nthreads(), via blosc2_ctx_get_cparams()), and say in set_nthreads()'s docs that the variable wins at the C level. Importing blosc2 also overrode NUMEXPR_NUM_THREADS, which set_nthreads() recommends setting; it now leaves numexpr alone then, and otherwise gives it at most numexpr.MAX_THREADS (BLOSC_NTHREADS can go beyond that, which numexpr refuses with an error). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SwMZj6jNaqsesMKnBde8Lr
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Keys made of integers (Python or NumPy, not bools), unit-step slices, one Ellipsis and, for reads, None went through the general path: 8-30 us of Python per call (ndindex as soon as a NumPy integer, ... or None appears), more than decompressing a small selection. NDArray.get_basic_numpy() classifies such keys in Cython, resolves them against the shape and reads the result under one acquisition of the handle lock, through _get_slice_cbuffer(), the GIL-releasing read that get_slice_numpy() now shares with it. get_basic_start_stop() resolves the same keys (without None) for __setitem__. Any other key, or one that does not fit the shape, gives None and takes the unchanged general path, which raises the same exceptions as before. NumPy integers without __index__ (timedelta64) are left to it too, where they raise IndexError as in NumPy. SWMR: a read resolves the key against the cached shape and relies on the refresh inside the C read; if b2nd_array_t.last_tick shows that this refresh changed the shape (or the key did not fit, or the read failed), it refreshes, resolves and reads again, so a key is never resolved against a stale shape, at one file check per on-disk read instead of two. A write refreshes under the handle lock before resolving. Keys with None now return arrays that own their data (they were views of a temporary). Measured (Ryzen 9 7950X): a[3, 5, 7] of a 300x512x512 float32 array 34 -> 25 us in memory and 48 -> 35 us on disk, a[500] of a 1000x1000 array 21.5 -> 13.9 us, a[np.int64(7), np.int32(9)] 37.5 -> 7.3 us, iterating over 2000 rows 64 -> 44 ms; a[np.int64(i), j] = v 22 us less. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SwMZj6jNaqsesMKnBde8Lr
Creating a small array cost ~30 us, mostly Python bookkeeping: - compute_chunks_blocks() and _check_ndarray_kwargs() converted CParams and Storage with dataclasses.asdict(), which deep-copies; a shallow dict (vars() for Storage, whose fields are plain values) is enough; - every SChunk wrapper, so every NDArray, built its cparams and dparams eagerly under the handle lock; they are now built on first use; - create_cparams_from_kwargs() looked up Filter.BYTEDELTA.value (a slow enum descriptor) twice per filter slot; now once per call; - is_inside_new_expr() and is_inside_ne_evaluate(), which arange, linspace, eye, reshape, fromiter and lazy functions such as clip run on every call, used inspect.stack(), which reads the source of every frame; walking the frames gives the same answer in under 1 us. __setitem__ also wrapped plain NumPy values in a SimpleProxy, whose constructor runs compute_chunks_blocks(); other array types keep the wrapping, NumPy arrays give the same outcome without it (checked for contiguous, strided, Fortran-order, 0-d, broadcast and converted values). Measured (Ryzen 9 7950X): zeros((100, 100)) 31 -> 20 us, eye() 311 -> 179 us, reshape() 437 -> 276 us, clip(a, 0, 1) 84 -> 12 us; a 3x3 write into a 100x100 array ~12 us less (asdict() ~5 us, SimpleProxy ~7 us). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SwMZj6jNaqsesMKnBde8Lr
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Drop this commit before merging: once Blosc/c-blosc2#835 is merged and released, point BLOSC2_BUNDLED_VERSION back to Blosc/c-blosc2 at a release tag that contains it and bump BLOSC2_MIN_VERSION, since update_data() and insert_data() rely on its rule that a failed chunk write without a copy leaves the chunk with the caller. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.
Hi Francesc,
this is part 2 of 4 of the split of #744: lower per-call overhead of indexing and of creating arrays.
Depends on #746 (part 1, the fixes, which is stacked on #743). The Cython fast path reads through #743's GIL-free
_get_slice_cbuffer(), and its tests build on part 1's__getitem__fixes. Until those are merged, this PR also shows their commits. The new ones:e8f72afResolve basic keys of NDArray.getitem/setitem in Cython6c47400Lower the per-call overhead of creating arrays and of small writesThe last commit (TEMP) builds against Blosc/c-blosc2#835, like part 1. This part needs nothing new from c-blosc2.
What changed (Ryzen 9 7950X; numbers from the commit messages)
...,None) in__getitem__/__setitem__.a[3, 5, 7]of a 300x512x512 float32 array went from 34 to 25 µs in memory and from 48 to 35 µs on disk, anda[np.int64(7), np.int32(9)]from 37.5 to 7.3 µs.dataclasses.asdict(), noinspect.stack().blosc2.zeros((100, 100))went from 31 to 20 µs, andclip(a, 0, 1)from 84 to 12 µs.Behaviour changes
Nonereturn arrays that own their data (they were views of a temporary).Found, not fixed (questions for you)
locking=True, a read can still fail withnchunk exceeds the number of chunkswhen another process shrinks the array: the slice is computed from a shape read under an earlier lock. c-blosc2 only exports the exclusiveblosc2_schunk_lock(). A shared variant would let us hold one lock across refresh, slice computation and read.Tests
pytest tests: 9559 passed, 280 skipped, 2 failed (the same 2b2viewhttp failures as on main). New: randomized comparisons of the fast paths of__getitem__and__setitem__against the general path and NumPy, on disk and in memory, and an SWMR reader that follows resizes.AI disclosure: I used Claude Code (Anthropic) for the investigation, the code and the tests of #744, which I reviewed and tested. Claude Code also did this split and opened this PR on my behalf, as agreed in #744 (comment). The description is assembled from the one of #744.
Best,
Fabian
🤖 Generated with Claude Code