fixes for netcdf/HDF5 file vars (strided) read/write performance and unlimited dimension issues#3322
Conversation
|
@WardF this is the next PR that should be merged. It is ready to go. |
|
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! |
|
@WardF would you like 32-bit testing in the CI? |
|
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. |
|
@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? |
|
@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]); |
There was a problem hiding this comment.
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.
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:
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