pre-size chunk buffer to avoid realloc fragmentation; fix nbit/scaleoffset no-op return values#6389
pre-size chunk buffer to avoid realloc fragmentation; fix nbit/scaleoffset no-op return values#6389brtnfld wants to merge 10 commits into
Conversation
brtnfld
requested review from
bmribler,
derobins,
fortnern,
glennsong09,
jhendersonHDF,
lrknox,
mattjala,
qkoziol and
vchoi-hdfgroup
as code owners
brtnfld
added
the
Component - C Library
|
I will need to do a PR to netcdf for a fix. |
| * This avoids repeated realloc() inside the filter pipeline (e.g., deflate's | ||
| * doubling strategy when the output buffer fills), which on Windows fragments | ||
| * the heap and causes read time to increase steadily across iterations. */ | ||
| if ((old_pline && old_pline->nused) && buf_alloc < chunk_size) |
There was a problem hiding this comment.
I'm not sure you need the first condition - you can probably assert that chunk_nbytes == chunk_size if there are no filters (I think it was checked earlier):
if (buf_alloc < chunk_size) {
assert(old_pline && old_pline->nused);
buf_alloc = chunk_size;
}
fb096a0 to
6ab8428
Compare
brtnfld
force-pushed
the
4481
branch
2 times, most recently
from
f1f1b72 to
bc4511b
Compare
…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 HDFGroup#4481 and HDFGroup#4513.
… 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.
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.
…pacity 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 HDFGroup#4481 / HDFGroup#4513).
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.
Not suitable for the test suite; intended for manual Windows validation only. Keep locally if needed.
…pacity 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 HDFGroup#4481 / HDFGroup#4513). Also fixes the scaleoffset bounds-check regression where *buf_size was used as an end-of-buffer sentinel.
Reverts the deflate in-place decompression experiment and the H5Dchunk.c pre-sizing changes back to the state at 9ab5e19, keeping the nbit and scaleoffset no-op path fixes.
…ipeline 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.
|
This pull request has had no activity for 30 days and has been marked stale. Push a commit or comment to keep it open, or it will be flagged for maintainer review. |
Review ChecklistThis PR touches the following areas. Each needs a sign-off
|
|
Will benchmark on Windows before merging |
Windows benchmark: PR #6389 chunk-buffer pre-sizing fixSetup: Windows 11, Intel oneAPI 2026.1 —
Mechanism, verified at the source level: Benchmark: Mirrors issue #4481's repro shape — each iteration alternates between a full read of dataset A and a strided (stride-2) hyperslab read of a differently-chunked dataset B, both deflate-compressed with highly compressible (~constant) data so
Run sequentially, 1000 iterations each (open file → read A → read B → close, per iteration):
Prior, simpler benchmarks (single dataset, few large chunks) showed a similar, consistent effect at smaller scale:
~21–28% faster chunked reads, consistently, across every scale tested — directly attributable to PR #6389 pre-sizing the chunk buffer to the full uncompressed Memory note (hypothesis, not confirmed): the fix's process working set ramps from ~47 MiB to a steady ~176 MiB within the first ~50 iterations, then stays flat through iteration 1000 (not a leak — a one-time plateau, confirmed by sampling working set at 100-iteration intervals across the full run). A plausible explanation is Windows' Low-Fragmentation Heap growing a dedicated bucket in response to the fix's steady, fixed-size allocation pattern, while baseline's varying realloc-growth sizes may not trigger the same bucket promotion — but this wasn't verified with a heap profiler, so treat it as a working theory rather than a demonstrated mechanism. Real trade-off either way: worth being aware of on memory-constrained Windows deployments, though the speed win is the clear, verified headline. I did not reproduce the original issue's escalating multi-second slowdown across iterations (#4481/#4513 saw read time climb from 17s→22s over 30 iterations on datasets orders of magnitude larger) — regression slope restricted to the steady-state window (iters 50–999) shows both variants drifting <2% of their average, i.e. essentially flat/noise. That escalating effect likely needs a much larger, longer-running dataset to visibly fragment the heap to the point of measurably slower allocation, rather than just a larger steady-state reservation. |
Attempted reproduction of #4481/#4513's escalating-slowdown symptomFollowing up on the benchmark results above: I attempted a dedicated reproduction of the specific symptom reported in #4481 (read time climbing over successive iterations, 17s→22s over 30 iterations) — not just the general throughput improvement. Methodology: scaled a synthetic two-dataset benchmark to match the original repro's actual regime as closely as possible without the original files (hosted externally, inaccessible):
Result: no escalation in either variant.
An initial smaller run (150 iterations, ~3.3M cycles) showed a modest baseline drift (+3.7%) that looked like a promising signal — but it did not hold up or compound at the larger scale; it was noise from an undersized sample. Conclusion: PR #6389 gives a consistent, real speedup here too (7%, in line with the 21–28% seen at other chunk sizes elsewhere in this thread), but I was not able to reproduce the specific escalating-degradation symptom from #4481/#4513, even after scaling chunk sizes and total cycle count to match or exceed the original report's regime. Plausible reasons this doesn't rule out the bug being real and fixed:
Net assessment: the fix is well-motivated (the realloc/copy churn it removes is real, confirmed at the source level in |
Faithful reproduction of #4481's exact repro code — escalation confirmed and measurably reducedFollowing up further: I rewrote the benchmark to use the issue's actual repro code (not just a size-matched synthetic approximation) —
Files were synthesized locally (same chunk shapes/dims as the repro:
This is a real reproduction of the reported effect (unlike an earlier, simpler synthetic benchmark that showed no escalation at all) — both variants climb early then plateau, consistent with a heap-fragmentation-style effect rather than noise. PR #6389 roughly halves the escalation but doesn't eliminate it. Why it's reduced, not eliminatedThis PR changed the size passed in, not the allocator. Every chunk-cache eviction during these reads is still a raw allocator round-trip. The fix removes the 4-9 reallocs-per-chunk it used to cost; it doesn't remove the fact that it's still raw Net assessment: PR #6389 is a real, verified, substantial improvement — both in raw throughput (21-28% faster reads across every benchmark scale) and in reducing (not eliminating) the specific escalating-slowdown behavior from #4481/#4513. |
Full investigation summary: PR #6389, the escalating-slowdown symptom, and what's actually happeningThis is a complete write-up of an extended investigation into whether PR #6389 resolves the specific behavior reported in #4481/#4513 (read time escalating over successive iterations), beyond the general performance benchmarks posted earlier in this thread. 1. Baseline performance win (high confidence, independently verified)PR #6389's chunk-buffer pre-sizing gives a real, consistent 21-28% faster read throughput for compressed chunked datasets, verified across multiple scales and confirmed at the source level: 2. Reproducing the escalating-slowdown symptom specificallyA first synthetic benchmark (matched read/storage types, single-axis stride) showed no escalation in either baseline or fix — flat in both. It was missing key ingredients. A faithful reproduction, using the issue's actual
PR #6389 roughly halves the escalation but doesn't eliminate it, in this repeated- 3. Isolating where the residual escalation actually livesRuled out two plausible culprits by reading the code: type-conversion buffers ( An experimental patch extending the free list to filtered chunk buffers (not shippable — it breaks third-party filter plugins that manage their own buffers via raw The decisive diagnostic: open the file once and read repeatedly, vs. reopening every iteration.
This proved PR #6389 fully resolves the chunk-read-path escalation — the residual only appears when repeatedly reopening, meaning it's tied to something that runs on every 4. The real remaining driver: speculative metadata loads, same pattern, different subsystem
Two fixed, small initial-size guesses were found and tested by simply increasing them:
Both are plausibly undersized for any dataset with real complexity (chunked + filtered + named + grouped, as ours are). Increasing both to 4096 bytes progressively closed the gap:
Dataset A (unlimited-dim, Extensible Array index) is now fully resolved. Dataset B (no unlimited dim, simpler structure, but larger ~547KB chunks and a 3D strided selection) continues to shrink with each fix but retains a smaller residual (~5%), suggesting at least one more small contributor exists, not yet identified — the pattern of roughly halving with each fix suggests diminishing but real remaining sources of the same kind. Net assessment
|
4 participants
When reading a filtered chunk, the decompression buffer was initialized at the compressed on-disk size. Filters like deflate grow this buffer via repeated realloc() calls, doubling its size when the output exceeds capacity. On Windows, this fragments the heap and causes read times to increase steadily across successive iterations.
The fix pre-sizes the buffer to the full uncompressed chunk_size when a filter pipeline is active, so the output buffer is large enough from the start and no realloc is needed.
Fixes #4481 and Fixes #4513.
Per the HDF5 filter API, nbytes is the count of valid data bytes in the buffer; *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 above makes *buf_size > nbytes on compressed reads for the first time, exposing two latent bugs:
H5Znbit.c: the no-compress pass-through path (cd_values[1] == 1) returned *buf_size instead of nbytes. The downstream filter (e.g., Fletcher32) then treated the full pre-sized buffer as valid input and computed a checksum over uninitialized bytes, causing filter pipeline failures.
H5Zscaleoffset.c: the no-process path (when the data already spans the full bit range and no scaling is needed) had the same *buf_size vs nbytes mistake. 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.