-
-
Notifications
You must be signed in to change notification settings - Fork 272
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
_merge_chunk_array could allocate memory more carefully #2038
Comments
Related: this zarr-python/src/zarr/codecs/pipeline.py Lines 361 to 362 in b8baa68
When writing, this will trigger unnecessary trips to the store to read chunks, even though the data will be completely overwritten. |
good catch @rabernat, I don't know this code very well but have a few high-level additions
|
Tracemalloc should allow some tracing of allocations. A good target would be the following: if I create a memorystore with uncompressed data, I should be able to get this data all the way to a numpy array without copying any bytes. |
Ive used the memory_profiler ipython extension to good effect. More recently scalene is getting a lot of attention |
maybe it would make sense to create a specific hatch environment with useful benchmarking / performance monitoring tools, so we could do stuff like
|
I was reviewing the Pipeline and has some observations. A key function is
_merge_chunk_array
zarr-python/src/zarr/codecs/pipeline.py
Lines 298 to 332 in b8baa68
There are two issues with this implementation
if existing_chunk_array is None:
this code path always allocates a new empty array and then copies data from the original selection into it? Why? This seems like a great opportunity to avoid and extra memory copy. Can't we just use a view of the original data (values
) when appropriate? This happens e.g. when writing to chunks which are smaller than the selectionchunk_spec.shape
, even for the last chunk of an array, which might be much smaller than the chunk shape.An example which highlights both of these inefficiencies is as follows:
The text was updated successfully, but these errors were encountered: