Repository navigation
Do not leak the sliced array in NDArray.slice() - #781
Open
FabianIsensee wants to merge 2 commits into
Open
FabianIsensee wants to merge 2 commits into
FabianIsensee wants to merge 2 commits into
Conversation
concat() and the other functions that build a b2nd context (uninit, nans, empty, zeros, full, from_buffer, asarray, NDArray.copy and NDArray.get_slice) raised before b2nd_free_ctx() when the C call failed, leaking the context (about 1 kB per call). create_b2nd_context() also malloc'd the cparams and dparams of every new array and the names and contents of its metalayers, and never freed them (192 bytes per array plus the metalayers, also on success). b2nd_create_ctx() copies the cparams, so they are a local now; the context keeps pointing to the dparams and the metalayers. create_b2nd_context() now returns a small _B2ndContext object that owns the context, the dparams and the metalayer buffers it points to and frees them when it goes away, on success and on every error path, so the callers no longer free the context themselves. Its metalayer table has B2ND_MAX_METALAYERS entries, like the stack table before; more metalayers now raise ValueError instead of overflowing it (the process aborted with "stack smashing detected"). Other error paths that kept a resource: - asarray(), full(), compress2(), NDArray.set_slice(), NDArray.get/set_oindex_numpy(), schunk_from_cframe(), ndarray_from_cframe(), SChunk.decompress_chunk(dst=...) and SChunk.get_slice(out=...) kept a buffer export of their input, which was then never freed (a bytearray could not be resized any more) - open() kept the super-chunk and its open file when the b2nd metalayer could not be decoded or check_schunk_params() raised - NDArray.get_slice() kept the sliced array when b2nd_squeeze_index() failed - NDArray.get/set_oindex_numpy() kept the index arrays The exceptions and their messages do not change (the "Error while freeing the context" checks are gone: b2nd_free_ctx() cannot fail). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DiPFztdpnKMJkzrjT36BuP
get_slice() built the slice with the int-indexed dims of length 1 and then replaced it with b2nd_squeeze_index(array, &array, mask). That function returns a view which reads the chunks of the array it was given, so the sliced array (super-chunk, contexts, chunks) was never freed: every slice() that was not chunk-aligned leaked it (about 75 KiB for a small slice, 93 KiB with nnInteractive's undo-log pattern), also without int indices, since the squeeze ran with an all-False mask too. The view also lost what the caller asked for: urlpath and contiguous did not reach the result (an urlpath file got the unsqueezed shape), and meta= always raised, because b2nd_squeeze_index() refuses non-b2nd metalayers. The chunk-aligned fast path had the same view problems (no leak there, the view kept its base). Now slice() always returns a new array that owns its data: - Without int indices, the array from b2nd_get_slice() is the result. - With int indices, the slice is built in memory with chunk and block 1 in the dropped dims, so that its chunks are also chunks of the squeezed result. They are moved into an array created with the squeezed shape and the caller's storage (no recompression; no copy either for an in-memory result), and the slice is freed. - The aligned fast path creates its array with the squeezed shape directly and copies the chunks of self into it, as before. Shapes, chunks, blocks and data of the results are unchanged. .base stays None for non-aligned slices and is now None for aligned ones too (was the full-rank temporary array). test_error_path_leaks.py loses its slice-squeeze case: slice(meta=...) does not fail any more. On the way: chunks/blocks given as tuples raised AttributeError, chunks for the result were padded at every length-1 dim instead of at the int-indexed ones only, chunks= or blocks= with an aligned key raised TypeError, and int indices with a step raised IndexError. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DiPFzsfE5i74cBSxZN13M8
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 #770 (stacked; its commit shows here until #770 is merged).
NDArray.slice()leaked the array it sliced into on every call that was not chunk-aligned.NDArray.get_slice()builds the slice withb2nd_get_slice()(int indices as length-1 dims) and then callsb2nd_squeeze_index(array, &array, mask). That function returns a view that reads the chunks of the array it was given, so the sliced array (super-chunk, contexts, chunks) was never freed. It ran with an all-False mask too, so slices without int indices leaked as well. Example:a.slice((2, slice(37, 141), slice(55, 190), slice(66, 230)))on a (7, 200, 300, 300) array with chunks (1, 64, 64, 64) keeps 90 KiB of malloc'ed memory per call (the array is mostly zeros); with random values in the box it keeps the whole compressed slice, 4.6 MB per call; a 10x20 int32 slice keeps 75 KiB. nnInteractive callsslice()like this for every interaction (the pre-image for its undo).The view also dropped what the caller asked for:
urlpathandcontiguousdid not reach the result: it was an in-memory view, and the file aturlpathhad the unsqueezed shape (blosc2.open()returned(1, 14, 24)for a(14, 24)slice).meta=always raisedRuntimeError: Error while squeezing sliced array(b2nd_squeeze_index()refuses non-b2nd metalayers).newarr.squeeze(...), a view of the full-rank temporary: no leak there, but the same urlpath/meta=problems, andchunks=/blocks=raisedTypeError(passed twice toblosc2.empty()).What this PR changes:
b2nd_get_slice()(created with the caller's storage) is the result.empty()with the squeezed shape and the caller's kwargs, the chunks are moved into it (blosc2_schunk_update_chunk(..., copy=False), no recompression; no copy either for an in-memory result) and the slice is freed.chunksorblocksare given.AttributeError; chunks/blocks for the result were padded at every length-1 dim instead of at the int-indexed ones (a.slice((2, slice(5, 6), slice(5, 29)), chunks=[1, 12])raisedValueError); full-rank chunks with a value > 1 in an int-indexed dim gave an unreadable result; an int index plus a step raisedIndexError.slice-squeezegoes: it expectsslice(meta=...)to fail, which it no longer does.Results keep their shape, chunks, blocks, cparams/dparams and data. They are never views now:
.basestays None for non-aligned slices and is now None for aligned ones too (it was the full-rank temporary).Tests (
tests/ndarray/test_slice.py, 32 new cases, 23 fail without the change): int-index rows in the existingtest_slicetable (dropped dims, 0-d result, aligned, step;.base is None),test_slice_storage(urlpath + meta + mode, reopened with the squeezed shape; contiguous),test_slice_cparams_dparams,test_slice_int_indices_chunks_blocks, andtest_slice_frees_sliced_array, which counts malloc's in-use bytes per call with glibc'smallinfo2()(skipped elsewhere), bound 256 bytes: #770 keeps 75 KB per call, this PR 0. Full suite: same results as #770's branch. The touched test files also ran under AddressSanitizer without reports, and with LeakSanitizer the 5.2 MB that 100 slices left viab2nd_get_slice()are gone (nothing left from slicing).Perf (1 thread on an EPYC 9554, nnInteractive's array and call, per call, interleaved rounds; measured on main 126fa8f, built twice for the noise floor (within 3%), and re-checked on top of #770): a (30, 40, 40) slice takes 66-92 µs instead of 121-127 µs; with random values, a (104, 135, 164) slice takes 1.1-1.2 ms instead of 2.2-2.3 ms (the leaking version faulted in the 4.6 MB it leaked on every call). For a mostly-zero (104, 135, 164) slice (one 4.6 MB chunk) the time is the same within code layout (700-729 µs vs 729-731 µs here; two builds of this PR that differ only by an unused function were 18% apart). One pattern gets slower: when each big slice is dropped right away, glibc now trims the freed chunk-sized temporaries of
b2nd_get_slice()back to the kernel and the next call faults them in again (about 1.8-2.0 ms instead of 0.72-0.76 ms; the leaking version never shrank the heap). Blosc/c-blosc2#866 removes that by taking that buffer from the per-thread scratch cache (0.75 ms then); this PR does not depend on it.I used Claude Code (Anthropic) to find the cause, write the fix and the tests, and measure them; I reviewed the changes and the numbers.
🤖 Generated with Claude Code