Fix problems with vlens and refs inside compound using H5VLget_file_type() #274
Conversation
Merge from canonical
Merge from canonical
fortnern
requested review from
bljhdf,
byrnHDF,
derobins,
jhendersonHDF,
jrmainzer,
lrknox,
qkoziol,
soumagne and
vchoi-hdfgroup
as code owners
src/H5T.c
Outdated
| /* Reset VOL fields */ | ||
| /* Increment ref count on owned VOL object */ | ||
| if (new_dt->shared->owned_vol_obj) | ||
| new_dt->shared->owned_vol_obj->rc++; |
There was a problem hiding this comment.
Having other parts of the library modify fields in the H5VL structs is going to be difficult to maintain. Could you please add a "increment refcount" routine in the H5VL code and call that?
There was a problem hiding this comment.
I can do this, though looking through the code there are many other places that directly manipulate the H5VL_object_t struct outside the H5VL package
There was a problem hiding this comment.
No doubt! I would suggest that you just herd as many cats as you have time for now and we'll keep tracking them down over time.
src/H5T.c
Outdated
| @@ -6295,6 +6349,7 @@ H5T_own_vol_obj(H5T_t *dt, H5VL_object_t *vol_obj) | |||
|
|
|||
| /* Take ownership */ | |||
| dt->shared->owned_vol_obj = vol_obj; | |||
| vol_obj->rc++; | |||
There was a problem hiding this comment.
(Another refcount increment for a H5VL routine)
src/H5Tcommit.c
Outdated
| @@ -148,6 +148,7 @@ H5T__commit_api_common(hid_t loc_id, const char *name, hid_t type_id, hid_t lcpl | |||
| new_obj->connector = (*vol_obj_ptr)->connector; | |||
| new_obj->connector->nrefs++; | |||
| new_obj->data = data; | |||
| new_obj->rc = 1; | |||
There was a problem hiding this comment.
Same with this piece of code (which I admit wasn't originally written by you) - could you please add a small routine in the H5VL code that encapsulated this setup and then call it here (and down below in this routine)?
There was a problem hiding this comment.
I can replace this with H5VL_create_object, though again there are still other places in the library that peek into H5VL_object_t. In fact this code would still need to do that ((*vol_obj_ptr)->connector), so we would need to write another routine to retrieve that, then propagate it to everywhere that currently gets the connector directly, which includes many (all?) of the *_api_common() routines.
There was a problem hiding this comment.
Are you thinking we should move H5VL_object_t to H5VLpkg.h? This is kind of outside the scope of this change.
There was a problem hiding this comment.
Agree - I would volunteer to pick up more of the changes like this in another PR, based on the start you make here.
There was a problem hiding this comment.
Are you thinking we should move H5VL_object_t to H5VLpkg.h? This is kind of outside the scope of this change.
I don't think that's necessary (yet?) and agree, would be far outside of the scope for your PR.
src/H5Tdeprec.c
Outdated
| @@ -139,6 +139,7 @@ H5Tcommit1(hid_t loc_id, const char *name, hid_t type_id) | |||
| new_obj->connector = vol_obj->connector; | |||
| new_obj->connector->nrefs++; | |||
| new_obj->data = data; | |||
| new_obj->rc = 1; | |||
There was a problem hiding this comment.
(Another candidate for the "setup" H5VL setup routine)
src/H5VLprivate.h
Outdated
| @@ -37,6 +37,7 @@ typedef struct H5VL_t { | |||
| typedef struct H5VL_object_t { | |||
| void * data; /* Pointer to connector-managed data for this object */ | |||
| H5VL_t *connector; /* Pointer to VOL connector struct */ | |||
| hsize_t rc; /* Reference count */ | |||
There was a problem hiding this comment.
This should be a 'size_t', since you are counting objects in memory.
|
This is a very nice change and if you could spend 15 minutes with a little refactoring, would be improved further. :-) |
|
@qkoziol I think I've addressed your concerns. |
…ype() (HDFGroup#274) * Fixed problems with vlens and refs inside compound using H5VLget_file_type() * Fix date in RELEASE.txt * Add assertions * Move some manipulation of H5VL_object_t struct fields into the H5VL package.
* Improve performance of multiple calls to H5Sget_select_elem_pointlist (#270) * Cache the pointer to the next point to process after the last call to H5S__get_select_elem_pointlist. This allows the normal process of iterating over the points in batches to be much more efficient, as the library does not need to traverse the entirety of the preceding points every time the funciton is re-entered. * Update RELEASE.txt for point selection iteration performance fix. * Fix problems with vlens and refs inside compound using H5VLget_file_type() (#274) * Fixed problems with vlens and refs inside compound using H5VLget_file_type() * Fix date in RELEASE.txt * Add assertions * Move some manipulation of H5VL_object_t struct fields into the H5VL package. * Add fix that was mistakenly left off merge commit. * Update src/H5Tprivate.h Co-authored-by: Larry Knox <lrknox@hdfgroup.org>
Modified library to properly ref count H5VL_object_t structs and only consider file vlen and reference types to be equal if their files are the same. Should prevent errors due to files being closed out from under cached types in the type conversion path table. There is still a potential error if the VOL connector closes the underlying file object before closing the datatype returned from H5VLget_file_type().