Re-implement the nc_get/put_vars operations for netcdf-4 using the corresponding HDF5 operations. #1001

DennisHeimbigner · 2018-05-22T22:52:39Z

re: github issue #908
also in reference to pydata/xarray#2004

The netcdf-c library has implemented the nc_get_vars and nc_put_vars
operations as element at a time. This has resulted in very slow
operation.

This pr attempts to improve the situation for netcdf-4/hdf5 files
by using the slab operations provided by the hdf5 library. The new
implementation passes the get/put vars stride information down to
the hdf5 slab operations.

The result appears to improve performance significantly. Some simple
tests on large 2-D arrays shows speedups in excess of 150.

Misc. other changes:

fix bug in ncgen/semantics.c; using a list's allocated length
instead of actual length.
Added a temporary hook in the netcdf library plus a performance
test case (tst_varsperf.c) to estimate the speedup. After users
have had some experience with this, I will remove it, probably
after the 4.7 release.

corresponding HDF5 operations. re: github issue #908 also in reference to pydata/xarray#2004 The netcdf-c library has implemented the nc_get_vars and nc_put_vars operations as element at a time. This has resulted in very slow operation. This pr attempts to improve the situation for netcdf-4/hdf5 files by using the slab operations provided by the hdf5 library. The new implementation passes the get/put vars stride information down to the hdf5 slab operations. The result appears to improve performance significantly. Some simple tests on large 2-D arrays shows speedups in excess of 150. Misc. other changes: 1. fix bug in ncgen/semantics.c; using a list's allocated length instead of actual length. 2. Added a temporary hook in the netcdf library plus a performance test case (tst_varsperf.c) to estimate the speedup. After users have had some experience with this, I will remove it, probably after the 4.7 release.

edhartnett · 2018-05-30T15:19:56Z

libsrc4/nc4var.c

+nc4_set_default_vars(int tf)
+{
+    defaultvars = (tf ? 1 : 0);
+}


Can this temporary flag be removed?

Changing to the old default methods can easily be done by changing the dispatch table.

edhartnett · 2018-05-30T15:24:50Z

libhdf5/nc4hdf.c

+#endif
+   int retval = NC_NOERR, range_error = 0, i, d2;
+   void *bufr = NULL;
+#ifndef HDF5_CONVERT


I'm not sure where this code came from. It looks like I wrote this code, but if so, why does it seem like it was just added?

In any case, the HDF5_CONVERT macro can never be defined. I have removed it from other code. I can be removed from this code as well. HDF5 never was able to convert data and generate the same output and error results that netCDF does, and HDF5_CONVERT is never set in the build. When it is set, it inevitably will result in failed tests.

DennisHeimbigner · 2018-05-30T16:36:27Z

Please leave it. When this gets released and actually used, I anticipate problems.
I may need to build test cases that switch between the two cases programmatically
to track down bugs.

edhartnett · 2018-05-30T16:39:00Z

@DennisHeimbigner you are referring to the defaultvars, not the HDF_CONVERT, correct?

I also believe their will be problems with this code, as it does not seem to do the correct thing for parallel I/O in all cases. After it is merged I will add some tests and probably submit a PR with additional code for parallel cases.

DennisHeimbigner · 2018-05-30T16:43:48Z

Yes, NCDEFAULT_get/put_vars.
I am not actually worried about the parallel case
WRT to debugging.
But the test case probably needs to be conditional
on not parallel.

edhartnett · 2018-05-30T16:46:05Z

You don't have to make the test case conditional on not parallel. The parallel issue is not one that any sequentially written code will active - the test has to do something trickier than that. So please just let your test run on parallel builds as normal.

DennisHeimbigner · 2018-05-30T16:49:25Z

I am a bit confused. Are you saying that NC4_put/get_vars code will not operate
correctly when parallel is used? If so, then that is a problem that needs to be fixed.

edhartnett · 2018-05-30T18:07:55Z

Yes, that is what I am saying. I believe they will not operate correctly in some cases.

I have some test code for a similar case for the vara get/put which is up in a PR right now. Once it gets merged I will add some tests to check similar behavior on vars.

Would you like me to make the necessary changes? I can take a branch and merge this branch, then add the tests and extra code needed.

DennisHeimbigner · 2018-05-30T18:17:44Z

I have some test code for a similar case for the vara get/put which is up in a PR right now.
I was about to ask about vara because the vars code is taken directly from the var code.
Would I be able to apply the vara changes to fix vars?

DennisHeimbigner · 2018-05-30T18:28:23Z

On second thought I think it would be best if you made the changes
since you understand the parallel code.

edhartnett · 2018-05-30T18:31:51Z

OK I'll have something tomorrow or the next day...

edhartnett · 2018-05-31T15:07:51Z

nc_test4/tst_varsperf.c

+#define DIMSIZE1 1024
+#define TOTALSIZE (DIMSIZE0*DIMSIZE1)
+
+#define CHECK(expr) assert((expr) == NC_NOERR)


Unfortunately we cannot use asserts in tests like this.

The reason is that this will cause the test to fail to compile with -DNDEBUG builds. The -DNDEBUG is usual in HPC builds (and also in code coverage builds, which is how my CI system caught this.)

So I have made minor modifications to the test to no longer use asserts, but instead to use the macros in err_macros.h.

edhartnett · 2018-06-01T10:21:10Z

OK, I have a version of this code which seems to work well for paralllel I/O. It depends on coord checking being relaxed. If we cannot eliminate the relax coord bound issue, then relaxed coord bounds will have to be turned on for all parallel builds anyway.

As soon as we get my current PR merged, we can move on with this issue.

@DennisHeimbigner I can confirm that this results in ~600 times speedup, which is great.

As part of my vars changes I removed the vara functions, which are just a special case of the vars functions. Otherwise this leads to a large amount of duplicated code.

WardF · 2018-06-08T21:58:53Z

Wires got crossed while merging master back in but I think that's fixed. Waiting for tests to finish, should be merged by EOD.

…es issue Unidata/netcdf-c#1001

shoyer · 2019-02-06T02:32:13Z

Just a heads up, this change seems to have slowed down a different path for indexing in netCDF4-Python:
Unidata/netcdf4-python#870

DennisHeimbigner requested a review from WardF as a code owner May 22, 2018 22:52

DennisHeimbigner changed the title ~~Re-Implement the nc_get/put_vars operations for netcdf-4 using the corresponding HDF5 operations.~~ Re-implement the nc_get/put_vars operations for netcdf-4 using the corresponding HDF5 operations. May 22, 2018

DennisHeimbigner mentioned this pull request May 22, 2018

Make netcdf-4 use the the stride > 1 facilities of hdf5 #908

Closed

edhartnett reviewed May 30, 2018

View reviewed changes

Merge branch 'master' into vars.dmh

2f1daaa

edhartnett reviewed May 31, 2018

View reviewed changes

WardF added this to the 4.7.0 milestone Jun 8, 2018

WardF self-assigned this Jun 8, 2018

WardF force-pushed the vars.dmh branch from 29d5eee to 2f1daaa Compare June 8, 2018 21:18

WardF added 2 commits June 8, 2018 15:50

Merge branch 'master' into vars.dmh

df4942d

Additional reconciliation

1d789d9

WardF merged commit b84afc7 into master Jun 8, 2018

WardF deleted the vars.dmh branch June 8, 2018 22:50

czender added a commit to nco/nco that referenced this pull request Sep 21, 2018

Allow USE_NC4_SRD_WORKAROUND only for netCDF <= 4.6.1 since 4.6.2 fix…

3a370bd

…es issue Unidata/netcdf-c#1001

shoyer mentioned this pull request Feb 6, 2019

3000x slow down for outer indexing between netCDF 4.6.1 and 4.6.2 Unidata/netcdf4-python#870

Closed

DennisHeimbigner mentioned this pull request Apr 2, 2019

HDF5 stride has different semantics than netcdf stride semantics wrt unlimited. #1380

Open

sebastic mentioned this pull request May 20, 2019

Segmentation fault in grdpaste GenericMappingTools/gmt#746

Closed

jswhit mentioned this pull request Dec 6, 2022

Writing using hyperslabs is extremely slow compared to netCDF-C Unidata/netcdf4-python#1222

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-implement the nc_get/put_vars operations for netcdf-4 using the corresponding HDF5 operations. #1001

Re-implement the nc_get/put_vars operations for netcdf-4 using the corresponding HDF5 operations. #1001

DennisHeimbigner commented May 22, 2018

edhartnett May 30, 2018

edhartnett May 30, 2018

DennisHeimbigner commented May 30, 2018

edhartnett commented May 30, 2018

DennisHeimbigner commented May 30, 2018

edhartnett commented May 30, 2018

DennisHeimbigner commented May 30, 2018

edhartnett commented May 30, 2018

DennisHeimbigner commented May 30, 2018

DennisHeimbigner commented May 30, 2018

edhartnett commented May 30, 2018

edhartnett May 31, 2018

edhartnett commented Jun 1, 2018

WardF commented Jun 8, 2018

shoyer commented Feb 6, 2019

Re-implement the nc_get/put_vars operations for netcdf-4 using the corresponding HDF5 operations. #1001

Re-implement the nc_get/put_vars operations for netcdf-4 using the corresponding HDF5 operations. #1001

Conversation

DennisHeimbigner commented May 22, 2018

edhartnett May 30, 2018

Choose a reason for hiding this comment

edhartnett May 30, 2018

Choose a reason for hiding this comment

DennisHeimbigner commented May 30, 2018

edhartnett commented May 30, 2018

DennisHeimbigner commented May 30, 2018

edhartnett commented May 30, 2018

DennisHeimbigner commented May 30, 2018

edhartnett commented May 30, 2018

DennisHeimbigner commented May 30, 2018

DennisHeimbigner commented May 30, 2018

edhartnett commented May 30, 2018

edhartnett May 31, 2018

Choose a reason for hiding this comment

edhartnett commented Jun 1, 2018

WardF commented Jun 8, 2018

shoyer commented Feb 6, 2019