Improve performance of h5trav interfaces for links to objects#6400
Improve performance of h5trav interfaces for links to objects#6400jhendersonHDF wants to merge 3 commits into
Conversation
For objects with multiple hard links, use hash table to map between object tokens and pathnames during traversal to avoid linear scan over all previous objects for each hard link seen Use separate hash table for h5trav "table" interface to map between object tokens and an index into the table of visited objects. This facilitates quick lookups of objects when adding hard link name aliases for h5repack processing
jhendersonHDF
added
the
Component - Tools
| #include "hdf5.h" | ||
|
|
||
| /* Typedefs for visiting objects */ | ||
| typedef herr_t (*h5trav_obj_func_t)(const char *path_name, const H5O_info2_t *oinfo, const char *first_seen, |
There was a problem hiding this comment.
These typedefs had to be moved down for the new trav_seen_t parameter to be available
| trav_obj_t *objs; | ||
|
|
||
| /* Private data for this trav_table_t */ | ||
| void *priv_data; |
There was a problem hiding this comment.
This is really a pointer to a structure with a UT_hash_handle, but exposing the uthash API at the h5trav.h level is problematic due to its header-only nature and already being used in H5private.h. This is just a quick hack to hide the implementation details.
There was a problem hiding this comment.
I wonder if it makes sense to just include H5private.h here
There was a problem hiding this comment.
Due to the header-only nature of uthash.h, it being included in H5private.h meant that having different settings for it in different parts of the library became somewhat difficult. In h5trav.c only, I needed to change change HASH_KEYCMP to use H5Otoken_cmp() to keep compatibility, but it became a first-to-include-uthash.h-wins scenario if I included H5private.h in h5trav.h since H5private.h is also including uthash.h with its own settings. The scope of including the uthash.h header should probably be reduced from H5private.h to where it's actually needed.
| * where a visited object was placed to facilitate quicker | ||
| * lookups when adding path aliases | ||
| */ | ||
| typedef struct trav_table_hash_t { |
There was a problem hiding this comment.
This structure is mostly for h5repack, which now has its own hash table separate from the main one used by other h5trav interfaces. I considered modifying the h5trav interfaces to allow h5repack to share a single hash table, but it would have been fairly awkward to do so and would have imposed unnecessary memory overhead for other tools that wouldn't need the extra information that would have been stored, so I instead just used a separate hash table specifically for the h5trav "table" interface.
| *------------------------------------------------------------------------- | ||
| */ | ||
| static int | ||
| trav_token_visited_cmp(hid_t loc_id, const H5O_token_t *token1, const H5O_token_t *token2) |
There was a problem hiding this comment.
This function is called by HASH_FIND and is just used to wrap around H5Otoken_cmp() instead of a plain memcmp of object token bytes
| udata.fields = fields; | ||
|
|
||
| /* Check for multiple links to top group */ | ||
| if (oinfo.rc > 1) |
There was a problem hiding this comment.
Moved this down below the udata initialization in case the hash table gets initialized by the call to trav_token_add().
jhendersonHDF
requested review from
bmribler,
brtnfld,
derobins,
fortnern,
glennsong09,
lrknox,
mattjala,
qkoziol and
vchoi-hdfgroup
as code owners
mattjala
left a comment
mattjala
left a comment
There was a problem hiding this comment.
Only a couple minor issues
| /* HASH_ADD modifies what's pointed to by objects_seen_ptr when it | ||
| * initializes the hash table after being called for the first time | ||
| */ | ||
| HASH_ADD(hh, (*objects_seen_ptr), obj.token, sizeof(H5O_token_t), entry); |
There was a problem hiding this comment.
Are we certain that tokens never contain garbage bytes? I suspect so just want to make sure, since they are stored as "MAX_TOKEN_SIZE", implying that some connectors don't use all of the bytes.
There was a problem hiding this comment.
I see that H5VL_native_addr_to_token zeroes the unused bytes, just curious how strong our documentation is that third party VOLs should do the same.
There was a problem hiding this comment.
That's true, I did make an assumption here and I haven't found any specific documentation mentioning that the bytes should be zeroed out or set to a specific value yet. We do provide H5O_TOKEN_UNDEF for undefined token values which in theory would be an initializer, but is not really documented as such. Without that assumption and without knowing how many valid bytes there are, I suppose you can't do much with an object token other than pass it to the token APIs. To be 100% sure here, we would need to either document that, or may need to have a callback for returning a hashed version of the token.
| udata.fields = fields; | ||
|
|
||
| /* Check for multiple links to top group */ | ||
| if (oinfo.rc > 1) { |
There was a problem hiding this comment.
This is the same logic as above, just had to be moved down a bit.
| return FAIL; | ||
| } | ||
| else { | ||
| for (i = 0; i < table->nobjs; i++) { |
There was a problem hiding this comment.
Would it be worth adding a hash table for the path names to accelerate this?
There was a problem hiding this comment.
Probably yes. I mostly tried to limit the scope of this to the performance problem I directly observed, but there's also likely a similar issue with soft links, as well as possibly with the path names here.
4 participants
For objects with multiple hard links, use hash table to map between object tokens and pathnames during traversal to avoid linear scan over all previous objects for each hard link seen
Use separate hash table for h5trav "table" interface to map between object tokens and an index into the table of visited objects. This facilitates quick lookups of objects when adding hard link name aliases for h5repack processing.
See the linked issue for context