mirror of
https://github.com/HDFGroup/hdf5.git
synced 2026-09-25 04:09:44 +03:00
pre-size chunk buffer to avoid realloc fragmentation; fix nbit/scaleoffset no-op return values (#6389)
* fix: pre-size filter output buffer to avoid realloc fragmentation on Windows
When reading a filtered (compressed) chunk, filters like deflate initialize
their output buffer using *buf_size (the pipeline's buffer capacity hint).
With *buf_size set to chunk_disk_size (the compressed size), deflate grows
its output buffer via repeated realloc() doubling. On Windows this fragments
the heap and causes read times to increase steadily across iterations.
Set buf_alloc to MAX(chunk_disk_size, chunk_size) immediately before calling
H5Z_pipeline so filters pre-allocate their output at the full uncompressed
size and avoid realloc. The inbuf is still allocated at chunk_disk_size —
only the hint passed to the pipeline is enlarged — so there is no
double-large-allocation overhead and peak memory stays at
chunk_disk_size + chunk_size rather than 2 * chunk_size.
For incompressible chunks where chunk_disk_size > chunk_size the hint
remains at chunk_disk_size, satisfying H5Z_pipeline's post-filter size
validation.
Fixes #4481 and #4513.
* fix: return nbytes instead of *buf_size in nbit and scaleoffset no-op paths
Per the HDF5 filter API, nbytes is the count of valid data bytes in the
buffer, while *buf_size is the allocated buffer capacity. These two values
were historically always equal on the read path, so either worked in
practice. The pre-sizing change (which sets buf_alloc = chunk_size before
the filter pipeline) makes *buf_size > nbytes for compressed reads,
exposing the latent bug.
In H5Znbit.c: the no-compress pass-through (cd_values[1] == 1) was
returning *buf_size, causing the next filter (e.g. fletcher32) to treat
the full pre-sized buffer as valid data and compute a checksum over
uninitialised bytes.
In H5Zscaleoffset.c: the no-process path had the same pattern. No current
test exercises this path through a multi-filter pipeline where the size
mismatch would be observable, but the fix is correct by the same API
reasoning.
* experiment: deflate decompress reuses inbuf via H5resize_memory
Instead of allocating a fresh output buffer (H5MM_malloc), copy the
compressed input to a small temp buffer and resize *buf in-place via
H5resize_memory. On Windows, HeapReAlloc can often extend an existing
heap block without moving it, avoiding the cost of finding and committing
a fresh large allocation for every chunk read.
This is combined with the pre-sizing hint in H5Dchunk.c that sets
buf_alloc = chunk_size before the pipeline call, so the resize goes
directly to chunk_size in one step with no realloc loop.
* fix: pre-resize chunk buf in H5Dchunk.c so all filters see correct capacity
Replace the hint-only buf_alloc enlargement with an actual
H5D__chunk_mem_realloc() before calling H5Z_pipeline. Every filter now
sees *buf_size equal to the real buffer capacity, not just an advisory
hint. This fixes a bounds-check regression in H5Zscaleoffset where the
read path uses *buf_size as the end-of-buffer sentinel in
H5_IS_BUFFER_OVERFLOW and as the input-size argument to
H5Z__scaleoffset_decompress; an inflated hint caused false-negative
overflow checks and potential over-reads when scaleoffset is the on-disk
filter.
H5Zdeflate is simplified accordingly: the up-front H5resize_memory(*buf,
nalloc) is removed since the buffer is already at nalloc on entry. The
inbuf copy is retained (still needed to read compressed input while
writing uncompressed output into the same buffer), as is the
realloc-doubling loop for the uncommon case where output exceeds
chunk_size.
Also add test/chunk_deflate_perf.c, a standalone benchmark that times
per-chunk deflate reads over multiple passes to detect the steady read-time
increase caused by heap fragmentation on Windows (issues #4481 / #4513).
* Committing clang-format changes
* test: add file path argument to chunk_deflate_perf benchmark
Add optional 4th argument to specify the output HDF5 file path
(default: chunk_deflate_perf.h5 in CWD). Allows running develop
and fix builds against separate files so pass 1 is cold-cache for
both and the two runs don't share page-cache state.
* test: remove chunk_deflate_perf benchmark
Not suitable for the test suite; intended for manual Windows validation
only. Keep locally if needed.
* fix: pre-resize chunk buf in H5Dchunk.c so all filters see correct capacity
Allocate the chunk read buffer at MAX(chunk_disk_size, chunk_size) from
the start rather than allocating at chunk_disk_size and immediately
reallocating. The read only fills chunk_disk_size bytes regardless of
buffer size, so there is no cost to the larger initial allocation and the
separate realloc step is eliminated.
Every filter now receives *buf_size equal to the actual buffer capacity
with no additional allocation needed. On Windows a single HeapAlloc at
the correct size avoids the repeated realloc-doubling in the deflate
filter that fragments the heap and causes read times to increase over
successive iterations (issues #4481 / #4513). Also fixes the
scaleoffset bounds-check regression where *buf_size was used as an
end-of-buffer sentinel.
* revert: restore H5Dchunk.c and H5Zdeflate.c to pre-experiment state
Reverts the deflate in-place decompression experiment and the H5Dchunk.c
pre-sizing changes back to the state at 9ab5e191d7, keeping the nbit
and scaleoffset no-op path fixes.
* fix: allocate chunk buf at MAX(disk_size, chunk_size) before filter pipeline
The previous approach allocated the buffer at chunk_disk_size and then
bumped buf_alloc to chunk_size as a hint to the pipeline, causing *buf_size
to misrepresent the actual allocation. Any filter that writes up to
*buf_size bytes into *buf would overflow.
Allocate at MAX(chunk_disk_size, chunk_size) upfront so the buffer and
the hint given to filters are always consistent. For incompressible chunks
where chunk_disk_size >= chunk_size the allocation is unchanged.
* Use HGOTO_DONE(nbytes) for the scaleoffset no-op passthrough
Matches the macro convention used for the other early-return in this
function per bmribler's review comment on PR #6389.
---------
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
This commit is contained in:
co-authored by
github-actions
parent
33eac87eea
commit
400de1dc95
+8
-6
@@ -4814,15 +4814,17 @@ H5D__chunk_lock(const H5D_io_info_t H5_ATTR_NDEBUG_UNUSED *io_info, const H5D_ds
|
||||
size_t chunk_nbytes; /* Length of the chunk in memory */
|
||||
size_t buf_alloc; /* [Re-]allocated chunk buffer size */
|
||||
|
||||
/* Assign above variables and check for overflow */
|
||||
/* Assign chunk_nbytes and check for overflow */
|
||||
H5_CHECKED_ASSIGN(chunk_nbytes, size_t, chunk_disk_size, hsize_t);
|
||||
H5_CHECKED_ASSIGN(buf_alloc, size_t, chunk_disk_size, hsize_t);
|
||||
|
||||
/* Ideally we should allocate a buffer at least as large as chunk_size, to give the filter the
|
||||
* opportunity to avoid doing a realloc when uncompressing. This causes problems now, however,
|
||||
* and needs more investigation. -NAF */
|
||||
/* Allocate at MAX(chunk_disk_size, chunk_size) so the buffer is
|
||||
* large enough for the uncompressed output before filters run.
|
||||
* This ensures *buf_size accurately reflects the allocation that
|
||||
* filters receive, avoiding a buffer overflow if any filter writes
|
||||
* up to *buf_size bytes into *buf. For incompressible chunks
|
||||
* chunk_disk_size >= chunk_size so buf_alloc stays at chunk_nbytes. */
|
||||
buf_alloc = (chunk_nbytes < chunk_size) ? chunk_size : chunk_nbytes;
|
||||
|
||||
/* Allocate chunk buffer */
|
||||
if (NULL ==
|
||||
(chunk = H5D__chunk_mem_alloc(buf_alloc, (udata->new_unfilt_chunk ? old_pline : pline))))
|
||||
HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, NULL,
|
||||
|
||||
+1
-1
@@ -939,7 +939,7 @@ H5Z__filter_nbit(unsigned flags, size_t cd_nelmts, const unsigned cd_values[], s
|
||||
* cd_values[1] stores the flag if true indicating no need to compress
|
||||
*/
|
||||
if (cd_values[1])
|
||||
HGOTO_DONE(*buf_size);
|
||||
HGOTO_DONE(nbytes);
|
||||
|
||||
/* copy a filter parameter to d_nelmts */
|
||||
d_nelmts = cd_values[2];
|
||||
|
||||
@@ -1186,10 +1186,8 @@ H5Z__filter_scaleoffset(unsigned flags, size_t cd_nelmts, const unsigned cd_valu
|
||||
HGOTO_ERROR(H5E_ARGS, H5E_BADVALUE, 0, "minimum number of bits exceeds maximum");
|
||||
|
||||
/* no need to process data */
|
||||
if (scale_factor == (int)(cd_values[H5Z_SCALEOFFSET_PARM_SIZE] * 8)) {
|
||||
ret_value = *buf_size;
|
||||
goto done;
|
||||
}
|
||||
if (scale_factor == (int)(cd_values[H5Z_SCALEOFFSET_PARM_SIZE] * 8))
|
||||
HGOTO_DONE(nbytes);
|
||||
minbits = (uint32_t)scale_factor;
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user