Skip to content

fixes for netcdf/HDF5 file vars (strided) read/write performance and unlimited dimension issues#3322

Merged
WardF merged 8 commits intoUnidata:mainfrom
edhartnett:ejh_0315
Mar 30, 2026
Merged

fixes for netcdf/HDF5 file vars (strided) read/write performance and unlimited dimension issues#3322
WardF merged 8 commits intoUnidata:mainfrom
edhartnett:ejh_0315

Conversation

@edhartnett
Copy link
Copy Markdown
Contributor

@edhartnett edhartnett commented Mar 15, 2026

This is a very small set of bugfixes to two code files (nc4hdf5.c and nc4var.c) which addresses the strided read and write performance and unlimited dimension problems which have been reported over the years. These are problems that both Dennis and I spent many hours trying to fix, but to no avail. Claude 4.6 found the problem in minutes.

These performance problems resulted in many issues over the years. I believe I got all of them, but more historical ones may pop up.

It should be noted that this AI assist was enabled by:

  • existing strong unit tests of netcdf-c
  • good documentation in (these parts of) the code
  • the detailed issues, in which we reported to each other and the community our (lack of) progress, what we tried, what tests we added

Given this rich context, the AI was able to find the answer by piecing together the many tiny clues scattered in our results and attempts. Pulling together disparate details from different sources, claude was able to pinpoint the exact bug.

Fixes #448
Fixes #1381
Fixes #1757
Fixes #2668
Fixes #1380
Fixes #3123
Fixes #2721

To confirm this test, I first committed the test to this PR, which then failed CI, as expected. Then I applied the fix and now the PR passes, demonstrating that the test catches the bug (at last!)

These related issues were already resolved in previous fixes (mostly by Dennis), as demonstrated by tests or other investigations in this PR:
Fixes #1977
Fixes #1947
Fixes #1877
Fixes #3134
Fixes #1374
Fixes #2354

@edhartnett edhartnett changed the title fixes for vars read/write performance issues fixes for netcdf/HDF5 file vars (strided) read/write performance and unlimited dimension issues Mar 15, 2026
@edhartnett edhartnett marked this pull request as ready for review March 15, 2026 14:31
@edhartnett
Copy link
Copy Markdown
Contributor Author

@WardF this is the next PR that should be merged. It is ready to go.

@edhartnett
Copy link
Copy Markdown
Contributor Author

@WardF hopefully this PR can be merged soon. In addition to the fixes in PR #3330. I have a bunch of VLEN and compoound type fixes waiting to go...

@WardF
Copy link
Copy Markdown
Member

WardF commented Mar 27, 2026

Yup, I was evaluating it last night/looking at it again now. I have a couple things in regards to 32-bit platforms I want to double check and then we can see about getting it merged!

@edhartnett
Copy link
Copy Markdown
Contributor Author

@WardF would you like 32-bit testing in the CI?

Can GitHub Actions Test 32-bit Platforms?
Yes, it is feasible via two approaches:

Option 1 — QEMU emulation (same approach as big-endian)
The run_tests_bigendian.yml already does this using uraimo/run-on-arch-action@v2. The same technique works for 32-bit Linux:

yaml
- uses: uraimo/run-on-arch-action@v2
  with:
    arch: armv7   # or i386
    distro: ubuntu22.04
Supported 32-bit arches by that action: armv6, armv7, i386. This runs natively on the GitHub ubuntu-latest runner — no special hardware needed. The big-endian workflow already proves the pattern works.

Limitation: QEMU is slow (2–5× compile time penalty) and for i386 specifically it can be tricky because the HDF5 configure runs test programs under QEMU — same problem mentioned in the big-endian workflow comments, which is why they use the apt-packaged HDF5 there.

Option 2 — MinGW 32-bit (Windows runner, native execution)
MSYS2 supports MINGW32 and CLANG32 environments. The run_tests_win_mingw.yml already has CLANG64 commented out; adding MINGW32 to the matrix would give native 32-bit Windows testing with no emulation overhead:

yaml
matrix:
  msystem: [ MINGW64, UCRT64, MINGW32 ]
MINGW32 gives a real i686-w64-mingw32 GCC toolchain. HDF5 packages are available from the MSYS2 repos. This is probably the lowest-effort path to 32-bit CI coverage.

Recommendation
The two most practical additions:

MINGW32 in run_tests_win_mingw.yml — one line added to the matrix, native execution, covers off_t/size_t truncation bugs, pointer width assumptions, and 32-bit Windows ABI issues. Low cost.
i386 QEMU job (modeled on the s390x workflow) — covers 32-bit Linux, tests off_t handling in the autotools/classic builds, good complement to the Windows 32-bit job. Moderate cost due to emulation speed.
The main class of bugs these would catch: size_t/off_t implicit truncations from 64-bit values, pointer-sized integer assumptions, and any printf/scanf format string mismatches (%ld vs %lld). These are exactly the kinds of latent bugs that have historically surfaced in netCDF-C when users try to deploy on 32-bit embedded or legacy systems.

@WardF
Copy link
Copy Markdown
Member

WardF commented Mar 27, 2026

The issue is not the 32-bit testing, although I'll be wiring it into our docker-based infrastructure. It took me a good chunk of time to put my finger on what was bothering me here, I'll go add a review. There's an overflow we might hit more readily on 32-bit platforms with small-but-reasonable chunk sizes; once that's corrected, we'll be able to get this merged.

@edhartnett
Copy link
Copy Markdown
Contributor Author

@WardF is there some other testing (that is, other than the GitHub CI for netcdf-c?)

For example, are you testing with HDF5-2.0.0 anywhere?

@edhartnett edhartnett requested a review from WardF March 30, 2026 12:27
@edhartnett
Copy link
Copy Markdown
Contributor Author

@WardF this failed due to intermittent DAP4 test error. I'm rerunning.

This PR has been modified as requested and is read to merge.

if (start[d2] >= (hssize_t)fdims[d2])
fill_value_size[d2] = count[d2];
else if (endindex >= fdims[d2])
fill_value_size[d2] = count[d2] - ((fdims[d2] - start[d2])/stride[d2]);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For future reference, AI is often overly verbose. Instructions to remain informative but also emphasizing conciseness benefit. GenAI tools are great, but the benefit the most when the output is bite-sized and easy to digest.

@WardF WardF merged commit 983d263 into Unidata:main Mar 30, 2026
104 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment