Change decompress_chunk to get_chunk into cbuffer#663
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors chunk concatenation to retrieve chunks into a buffer instead of decompressing them first and adds an optional “arange” mode to fill tests with increasing values.
- Introduce
arangeparameter and support functions (increment_large_value,fill_recursive_arange, updatedfill_buffer_region) - Change
b2nd_concatenateto useblosc2_schunk_get_chunkfor aligned chunks andb2nd_get_slice_cbufferfor unaligned ones - Minor updates in tests, benchmark (copy flag, timing), build CI matrix, and propagate
chunksizeinschunk_copy
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/b2nd/test_b2nd_copy.c | Remove stray blank line |
| tests/b2nd/test_b2nd_concatenate.c | Add arange helper, update test logic to use cbuffer, new params |
| blosc/schunk.c | Propagate chunksize in blosc2_schunk_copy |
| blosc/directories.c | Relocate free(fname) after error check |
| blosc/b2nd.c | Replace decompress+set with get_chunk or cbuffer paths |
| bench/b2nd/bench_concatenate.c | Default copy=false, split timing variables, print speedup |
| .github/workflows/cmake.yml | Comment out cross-compile, add native AARCH64 job |
Comments suppressed due to low confidence (2)
tests/b2nd/test_b2nd_concatenate.c:59
- [nitpick] This comment is in Spanish; please translate to English to match the rest of the codebase (e.g. "// If no overflow occurs, break").
// Si no hay desbordamiento, salimos
tests/b2nd/test_b2nd_concatenate.c:269
- The inline comment says
valueis not used in arange mode, butfill_recursive_arangeactually reads and updates it. Please correct or remove this misleading comment.
// start_, shapes.shape2, value, typesize, arange); // value is not used in this case
| _findclose(file); | ||
| return BLOSC2_ERROR_FAILURE; | ||
| } | ||
| free(fname); |
There was a problem hiding this comment.
On error paths (e.g. when remove() fails) fname is not freed, resulting in a memory leak. Move free(fname) before the early return.
| buffsize *= shapes.shape2[i]; // Multiply by each dimension | ||
| } | ||
| // Allocate a buffer for the src2 array | ||
| uint8_t *buff = malloc(buffsize); |
There was a problem hiding this comment.
The allocated buffer (and value pointer) are not freed after use in the test. Consider calling free(buff); and free(value); to avoid memory leaks.
FrancescAlted
left a comment
FrancescAlted
left a comment
There was a problem hiding this comment.
Other than suggestions for some cleanup, this looks good to me.
| // TODO: the above makes some tests to crash, so always force a copy; try to optimize this later | ||
| int64_t nchunk_dest = 0; | ||
| nchunk_ndim[axis] += src1_shape[axis] / (*array)->chunkshape[axis]; | ||
| for ( int i =0; i< src2->ndim; i++){nchunk_dest += nchunk_ndim[i] * chunks_in_array_strides[i];} |
There was a problem hiding this comment.
This is against our style. FYI, we are using K&R standard internally.
Add arange to test for concatenate. Also - rather than decompressing chunk before passing to get_set_slice to be loaded into target array - simply retrieve chunk into buffer and then load. This avoids mangling of entries owing to block shapes that was happening in previous PR.