Avoid calling non-public h5fd_xxx_init() functions #967

david-macmahon · 2022-06-15T02:51:34Z

The h5fd_xxx_init() functions are not part of the HDF5 public API. A
method of retrieving the hid_t value of drivers that uses
public/supported API calls was suggested in a comment on a related
github issue and is implemented in this commit. For more details see:

HDFGroup/hdf5#1809 (comment)

The h5fd_xxx_init() functions are not part of the HDF5 public API. A method of retrieving the hid_t value of drivers that uses public/supported API calls was suggested in a comment on a related github issue and is implemented in this commit. For more details see: HDFGroup/hdf5#1809 (comment)

david-macmahon · 2022-06-15T02:55:56Z

Fixes #960

david-macmahon · 2022-06-15T03:20:33Z

OK, something weird is happening to the mmap.jl test. Even if I revert this commit and just add the two lines marked with # NEW to the existing __init__() function, then the mmap test breaks.

fapl = HDF5.FileAccessProperties(fclose_degree=:default) # NEW
close(fapl) # NEW
DRIVERS[API.h5fd_core_init()] = Core # EXISTING
DRIVERS[API.h5fd_sec2_init()] = POSIX # EXISTING

I don't understand why that would break the mmap.jl test.

david-macmahon · 2022-06-15T03:38:20Z

Even putting the # NEW lines at the end of HDF5.Drivers.__init__() breaks the mmap.jl test:

function __init__()
    DRIVERS[API.h5fd_core_init()] = Core
    DRIVERS[API.h5fd_sec2_init()] = POSIX
    @require MPI="da04e1cc-30fd-572f-bb4f-1f8673147195" include("mpio.jl")
    fapl = HDF5.init!(HDF5.FileAccessProperties()) # NEW
    close(fapl) # NEW
end

Is there some other part of the HDF5 libraries that need to be initialized before creating a file access property list?

mkitti · 2022-06-15T03:49:56Z

Note that there is also the MPIO driver:

HDF5.jl/src/drivers/mpio.jl

Line 47 in cfa5a24

DRIVERS[API.h5fd_mpio_init()] = MPIO

mkitti · 2022-06-15T03:54:08Z

My suspicion is that you accidentally initialized the HDF5 library before we could disable file locking in HDF5.__init__().

HDF5.jl/src/HDF5.jl

Lines 559 to 561 in cfa5a24

 if !haskey(ENV, "HDF5_USE_FILE_LOCKING") 

 ENV["HDF5_USE_FILE_LOCKING"] = "FALSE" 

 end

This is why I pointed out the potential problem in #959.

david-macmahon · 2022-06-15T03:57:23Z

Thanks for the pointer to the MPIO driver. And you are exactly right about the file locking messing with the mmap test. I had just found that out, but you beat me to it!

mkitti · 2022-06-15T03:58:21Z

I see two paths forward:

Create a new method to initialize DRIVERS and call that at the end of HDF5.__init__().
Make this lazy, and set the key for DRIVERS when someone invokes set_driver!.

david-macmahon · 2022-06-15T04:32:58Z

OK, yeah, I think the lazy approach is the way to go. I tried adding similar fapl logic to the MPIO driver using this:

# Check whether the HDF5 libraries were compiled with parallel support.
try
    fapl = HDF5.init!(HDF5.FileAccessProperties())
    comm = info = reinterpret(H5MPIHandle, 0)
    API.h5p_set_fapl_mpio(fapl, comm, info)
    DRIVERS[API.h5p_get_driver(fapl)] = MPIO
    close(fapl)
    HDF5.HAS_PARALLEL[] = true
catch e
    @show e
end

but that resulted in an MPI error: "The MPI_Comm_dup() function was called before MPI_INIT was invoked." It's obviously best to let the user call MPI_INIT, so the non-lazy approach seems fatally flawed in this case.

david-macmahon · 2022-06-15T06:15:53Z

One issue with the lazy approach is that a FileAccessProperties object can be created without calling set_driver!, but h5p_get_driver will still return a driver (i.e. SEC2/POSIX) for it. So maybe the DRIVERS entries for the sec2 and core drivers could be set non-lazily, but then set the entry for mpio lazily in set_driver!(fapl, mpio::MPIO). The problem with lazy initializing the mpio entry is that mpio.jl currently uses a non-throwing call to h5fd_mpio_init() as the way to detect that HAS_PARALLEL should be set to true (i.e. only sets it to true if the function call does not throw an exception). If we no longer call h5fd_mpio_init() then we will need some other way to determine whether the HDF5 libraries have parallel support.

mkitti · 2022-06-15T13:59:57Z

Perhaps the only driver we need to initialize is driver is the default one. That may be platform dependent. This may need to be done in HDF5.__init__.

Unless the user has already set `HDF5_USE_FILE_LOCKING` in the environment, set it to `FALSE` to avoid causing problems with mmap'ing. This needs to be done before the first call to the HDF5 APIs.

Instead of detecting parallel HDF5 support by whether `h5fd_mpio_init()` throws an exception, parallel support is detected by finding the `H5Pset_fapl_mpio` symbol in any loaded library whose name matches `r"hdf5"i`. The documentation for `H5Pset_fapl_mpio()` states: > H5Pset_fapl_mpio() is available only in the parallel HDF5 library The detection has also been moved into `Drivers.__init__()` and the inclusion of `mpio.jl` (upon loading of the MPI package) will only happen if the HDF5 libraries have parallel support.

simonbyrne · 2022-06-15T16:26:05Z

I believe SEC2 is the default on all platforms.

As for checking HAS_PARALLEL, we might be able to just check if the symbol exists?

This maintains compatibility with pre-1.5 versions of Julia.

mkitti · 2022-06-15T17:01:58Z

You probably should just do using Libdl.

mkitti · 2022-06-15T17:08:43Z

src/drivers/drivers.jl

+ loaded_libs = filter(s->match(r"hdf5"i, s) !== nothing, Libc.Libdl.dllist())
+ for libname in loaded_libs
+ has_mpio = Libc.Libdl.dlopen(libname) do lib
+ Libc.Libdl.dlsym(lib, :H5Pset_fapl_mpio; throw_error=false) !== nothing
+ end
+ if has_mpio
+ HDF5.HAS_PARALLEL[] = true
+ break
+ end
+ end


Suggested change

loaded_libs = filter(s->match(r"hdf5"i, s) !== nothing, Libc.Libdl.dllist())

for libname in loaded_libs

has_mpio = Libc.Libdl.dlopen(libname) do lib

Libc.Libdl.dlsym(lib, :H5Pset_fapl_mpio; throw_error=false) !== nothing

end

if has_mpio

HDF5.HAS_PARALLEL[] = true

break

end

end

API.Libdl.dlopen(API.libhdf5) do lib

HDF5.HAS_PARALLEL[] = Libdl.dlsym(lib, :H5Pset_fapl_mpio; throw_error=false) !== nothing

end

I suppose you could also just reference API.Libdl

I don't think this will work with a custom HDF5 library either

I revised it to use API.libhdf5handle[] instead of HDF5_jll.libhdf5_handle.

Why are we not using the standard JLL override mechanism?

Oops, that's not going to work either. HDF5.API.libhdf5 should work though.

Nonetheless, I think we should make sure to use the exact same shared library that the API module loaded rather than doing a search through the loaded libraries.

david-macmahon · 2022-06-15T18:18:04Z

I pushed that latest commit before I saw the "use HDF5.API.libhdf5" comment. I've made that change and will push once the current CI run finishes.

david-macmahon · 2022-06-15T18:48:47Z

Not sure why 57c0e52 didn't get a green check, but CI did run after that was pushed. I think this is "done" other than possibly changing using Libdl: dlopen, dlsym to API.Libdl.dlopen etc. I'm agnostic about that.

simonbyrne · 2022-06-15T19:02:58Z

Why are we not using the standard JLL override mechanism?

The original Overrides.toml approach (https://docs.binarybuilder.org/stable/jll/#Overriding-the-artifacts-in-JLL-packages) is way too restrictive to be useful (it requires that the libraries have the same .so version as the one in the jll)

The Preferences.jl approach (https://docs.binarybuilder.org/stable/jll/#Overriding-specific-products) should work though

simonbyrne · 2022-06-15T21:28:40Z

src/drivers/drivers.jl

+ API.h5p_set_fapl_core(fapl, 4096, false)
+ DRIVERS[API.h5p_get_driver(fapl)] = Core


we could lazily initialize Core as well?

This way seemed closer to the original, so it's a slightly smaller change, but I don't think there would be a problem lazily initializing Core as well if that's the consensus.

Perhaps all the set_driver! methods should uniformly register their hid_t in DRIVERS. From this __init__, we just call set_driver!(fapl, POSIX()).

mkitti · 2022-06-16T13:28:26Z

I would also be willing to merge this as is and revise later.

musm · 2022-06-17T15:18:21Z

src/drivers/drivers.jl

+ end
+
+ fapl = HDF5.init!(HDF5.FileAccessProperties())
+ API.h5p_set_fapl_core(fapl, 4096, false)


Where does 4096 come from?

Nowhere in particular (other than typical Linux page size). TBH, I suspect this could be set to 0 here because it's only used to setup fapl, which in turn is only used to get the ID of the Core driver. This fapl is never used to actually create anything. FWIW, I think this value defaults to 8192 elsewhere in HDF5.jl, but it's not clear where that value comes from.

musm · 2022-06-17T15:25:04Z

src/drivers/drivers.jl

- DRIVERS[API.h5fd_sec2_init()] = POSIX
- @require MPI="da04e1cc-30fd-572f-bb4f-1f8673147195" include("mpio.jl")
+ # disable file locking as that can cause problems with mmap'ing
+ if !haskey(ENV, "HDF5_USE_FILE_LOCKING")


What's the reason we need to add this check here as well? Is it related to Drivers being in its own module?

What we really need to do is address #959. Basically what happens is that the Drivers module is initialized before the HDF5 module. However, if we create a FAPL during Drivers initialization, then the environment variable no longer has the desired effect. Then memory mapping breaks before the files are now locked.

Thanks that makes sense.

musm · 2022-06-17T15:26:18Z

For future work, I think we should also try to get a build with HDF5 1.13 in our CI.

mkitti · 2022-06-18T01:52:28Z

I would like to merge this and setup another pull request for revisions.

mkitti · 2022-06-18T02:08:43Z

The original Overrides.toml approach (https://docs.binarybuilder.org/stable/jll/#Overriding-the-artifacts-in-JLL-packages) is way too restrictive to be useful (it requires that the libraries have the same .so version as the one in the jll)

The Preferences.jl approach (https://docs.binarybuilder.org/stable/jll/#Overriding-specific-products) should work though

Perhaps we should deprecate deps.jl and recommend the Preferences.jl approach?

david-macmahon added 3 commits June 15, 2022 08:59

Initialize fapl minimally in Drivers.__init__()

e4917b7

Disable file locking in Drivers.__init()

afca05a

Unless the user has already set `HDF5_USE_FILE_LOCKING` in the environment, set it to `FALSE` to avoid causing problems with mmap'ing. This needs to be done before the first call to the HDF5 APIs.

david-macmahon added 2 commits June 15, 2022 09:26

Add DRIVERS entry for MPIO lazily in set_driver!

7760124

Replace contains() with call to older match()

e9d513f

This maintains compatibility with pre-1.5 versions of Julia.

mkitti reviewed Jun 15, 2022

View reviewed changes

david-macmahon added 2 commits June 15, 2022 11:10

Improve usage of Libdl

811c29d

Use HDF5.API.libhdf5 rather than dllist()

57c0e52

simonbyrne reviewed Jun 15, 2022

View reviewed changes

mkitti approved these changes Jun 16, 2022

View reviewed changes

simonbyrne approved these changes Jun 17, 2022

View reviewed changes

musm reviewed Jun 17, 2022

View reviewed changes

musm merged commit 470eabc into JuliaIO:master Jun 18, 2022

mkitti mentioned this pull request Jun 18, 2022

Make Core init lazy, revise FileAccessProperty interface, fix file locking #972

Merged

mkitti mentioned this pull request Aug 24, 2022

Initialization of file drivers , H5FD_core_init, H5FDperform_init HDFGroup/hdf5#1809

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid calling non-public h5fd_xxx_init() functions #967

Avoid calling non-public h5fd_xxx_init() functions #967

david-macmahon commented Jun 15, 2022

david-macmahon commented Jun 15, 2022

david-macmahon commented Jun 15, 2022

david-macmahon commented Jun 15, 2022

mkitti commented Jun 15, 2022

mkitti commented Jun 15, 2022

david-macmahon commented Jun 15, 2022

mkitti commented Jun 15, 2022

david-macmahon commented Jun 15, 2022

david-macmahon commented Jun 15, 2022

mkitti commented Jun 15, 2022

simonbyrne commented Jun 15, 2022

mkitti commented Jun 15, 2022

mkitti Jun 15, 2022 •

edited

Loading

mkitti Jun 15, 2022

simonbyrne Jun 15, 2022

mkitti Jun 15, 2022

mkitti Jun 15, 2022

mkitti Jun 15, 2022

david-macmahon commented Jun 15, 2022

david-macmahon commented Jun 15, 2022

simonbyrne commented Jun 15, 2022

simonbyrne Jun 15, 2022

david-macmahon Jun 15, 2022

mkitti Jun 16, 2022

mkitti commented Jun 16, 2022

musm Jun 17, 2022

david-macmahon Jun 18, 2022

musm Jun 17, 2022

mkitti Jun 17, 2022

musm Jun 17, 2022

musm commented Jun 17, 2022

mkitti commented Jun 18, 2022

mkitti commented Jun 18, 2022

		API.h5p_set_fapl_core(fapl, 4096, false)
		DRIVERS[API.h5p_get_driver(fapl)] = Core

Avoid calling non-public h5fd_xxx_init() functions #967

Avoid calling non-public h5fd_xxx_init() functions #967

Conversation

david-macmahon commented Jun 15, 2022

david-macmahon commented Jun 15, 2022

david-macmahon commented Jun 15, 2022

david-macmahon commented Jun 15, 2022

mkitti commented Jun 15, 2022

mkitti commented Jun 15, 2022

david-macmahon commented Jun 15, 2022

mkitti commented Jun 15, 2022

david-macmahon commented Jun 15, 2022

david-macmahon commented Jun 15, 2022

mkitti commented Jun 15, 2022

simonbyrne commented Jun 15, 2022

mkitti commented Jun 15, 2022

mkitti Jun 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-macmahon commented Jun 15, 2022

david-macmahon commented Jun 15, 2022

simonbyrne commented Jun 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkitti commented Jun 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

musm commented Jun 17, 2022

mkitti commented Jun 18, 2022

mkitti commented Jun 18, 2022

mkitti Jun 15, 2022 •

edited

Loading