-
-
Notifications
You must be signed in to change notification settings - Fork 265
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor H5FD and package initialization #4934
Merged
derobins
merged 58 commits into
HDFGroup:develop
from
qkoziol:refactor_h5fd_and_packages
Oct 24, 2024
Merged
Refactor H5FD and package initialization #4934
derobins
merged 58 commits into
HDFGroup:develop
from
qkoziol:refactor_h5fd_and_packages
Oct 24, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
qkoziol
commented
Oct 6, 2024
•
edited
Loading
edited
- Reverts PR#1024, which (unnecessarily) switched from deferred package initialization to centralized initialization of all packages and introduced H5FDperform_init() to wrap an internal routine to initialize VFD plugins.
- Went back to deferred package initialization (primarily), to eliminate unnecessary resource use. (Performance has been verified to be the same either way)
- Switched VFD plugins to use “#define (H5OPEN, )” pattern, with registration of internal VFD plugins at library initialization time. Eliminates calling API routine (H5FDperform_init) from within library, which was deadlocking threadsafe concurrency. And also eliminates exposing internal library routines in a public header file.
- Removed copy-and-paste replicas of the H5OPEN macro and put a (better) single definition in H5public.h
- Separated API and internal routine calls in stdio and multi VFD plugins into separate source files, so that the library doesn’t invoke API routines internally (also a deadlock problem for threadsafe concurrency). Also needed a “private” header for these plugins.
- Separated registering/unregistering a VFD plugin from initializing/finalizing the plugin, instead of blurring those ideas together. Defers the VFD plugin init to when it’s actually used, which reduces resource usage, especially for the MPI-based plugins like the subfiling, etc.
- Refactored the copy-and-pasted check for locking into a central location in the H5FD.c code.
- Fixed a bunch of compiler warnings, especially ones that trigger CI failures with -Werror
Gets rid of public API call (H5FDperform_init) within the library code and returns to original way of initializing packages in the library. Signed-off-by: Quincey Koziol <[email protected]>
Signed-off-by: Quincey Koziol <[email protected]>
Signed-off-by: Quincey Koziol <[email protected]>
Signed-off-by: Quincey Koziol <[email protected]>
Update comment about interface initialization routines in H5_init_library Signed-off-by: Quincey Koziol <[email protected]>
Move all VFDs into the 'H5FD' package, instead of their own individual packages. Stop using VFD's 'term' callback to reset the ID for the VFD. Use explicit 'register' and 'unregister' calls to do that. Use the 'term' callback only for dealing with VFD internal resource cleanup. A _ton_ of other misc. changes, but mainly to update and align the VFDs more with current best practices in the library. Signed-off-by: Quincey Koziol <[email protected]>
Signed-off-by: Quincey Koziol <[email protected]>
Signed-off-by: Quincey Koziol <[email protected]>
Signed-off-by: Quincey Koziol <[email protected]>
Signed-off-by: Quincey Koziol <[email protected]>
Signed-off-by: Quincey Koziol <[email protected]>
Signed-off-by: Quincey Koziol <[email protected]>
Signed-off-by: Quincey Koziol <[email protected]>
Signed-off-by: Quincey Koziol <[email protected]>
Signed-off-by: Quincey Koziol <[email protected]>
Signed-off-by: Quincey Koziol <[email protected]>
Signed-off-by: Quincey Koziol <[email protected]>
hyoklee
approved these changes
Oct 17, 2024
Signed-off-by: Quincey Koziol <[email protected]>
derobins
changed the title
Refactor h5fd and package initialization
Refactor H5FD and package initialization
Oct 21, 2024
Ignores some of the older Autotools platform files, since the Autotools will be dropped in the next major release (and we don't have compilers on which to test, anyway). Also drops support for the old, non-compliant MSVC preprocessor.
The high-level GIF tools, h52gif and gif2h5, have unfixed CVE issues (with no proof-of-concept files). They are not critical tools, are not well maintained, and are an odd fit for building with the library. Because of this, they have been removed. We may move them to a separate repository in the future.
mattjala
reviewed
Oct 22, 2024
mattjala
reviewed
Oct 22, 2024
mattjala
reviewed
Oct 22, 2024
mattjala
reviewed
Oct 22, 2024
mattjala
reviewed
Oct 22, 2024
mattjala
reviewed
Oct 22, 2024
mattjala
reviewed
Oct 22, 2024
Signed-off-by: Quincey Koziol <[email protected]>
Signed-off-by: Quincey Koziol <[email protected]>
…packages Signed-off-by: Quincey Koziol <[email protected]>
mattjala
approved these changes
Oct 23, 2024
This VFD will need to be reworked. Disabling it for now.
H5FD__ros3_init() doesn't emit errors and needs FUNC_ENTER_PACKAGE_NOERR
Signed-off-by: Quincey Koziol <[email protected]>
brtnfld
pushed a commit
to brtnfld/hdf5
that referenced
this pull request
Oct 31, 2024
- Reverts PR#1024, which (unnecessarily) switched from deferred package initialization to centralized initialization of all packages and introduced H5FDperform_init() to wrap an internal routine to initialize VFD plugins. - Went back to deferred package initialization (primarily), to eliminate unnecessary resource use. (Performance has been verified to be the same either way) - Switched VFD plugins to use “#define (H5OPEN, )” pattern, with registration of internal VFD plugins at library initialization time. Eliminates calling API routine (H5FDperform_init) from within the library, which was deadlocking threadsafe concurrency. And also eliminates exposing internal library routines in a public header file. - Removed copy-and-paste replicas of the H5OPEN macro and put a (better) single definition in H5public.h - Separated API and internal routine calls in stdio and multi VFD plugins into separate source files, so that the library doesn’t invoke API routines internally (also a deadlock problem for threadsafe concurrency). Also needed a “private” header for these plugins. - Separated registering/unregistering a VFD plugin from initializing /finalizing the plugin, instead of blurring those ideas together. Defers the VFD plugin init to when it’s actually used, which reduces resource usage, especially for the MPI-based plugins like the subfiling, etc. - Refactored the copy-and-pasted check for locking into a central location in the H5FD.c code. - Fixed a bunch of compiler warnings, especially ones that trigger CI failures with -Werror
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
Component - C Library
Core C library issues (usually in the src directory)
Merge - Develop Only
This cannot be merged to previous versions of HDF5 (file format or breaking API changes)
Priority - 0. Blocker ⛔
This MUST be merged for the release to happen
Type - Bug / Bugfix
Please report security issues to [email protected] instead of creating an issue on GitHub
Type - Improvement
Improvements that don't add a new feature or functionality
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.