Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Avoid calling non-public h5fd_xxx_init() functions #967

Merged
merged 8 commits into from
Jun 18, 2022
Merged

Avoid calling non-public h5fd_xxx_init() functions #967

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e1bd495
Avoid calling non-public h5fd_xxx_init() functions
david-macmahon Jun 14, 2022
e4917b7
Initialize fapl minimally in Drivers.__init__()
david-macmahon Jun 15, 2022
afca05a
Disable file locking in Drivers.__init()
david-macmahon Jun 15, 2022
3db79a9
Change how parallel HDF5 suppport is detected
david-macmahon Jun 15, 2022
7760124
Add DRIVERS entry for MPIO lazily in set_driver!
david-macmahon Jun 15, 2022
e9d513f
Replace `contains()` with call to older `match()`
david-macmahon Jun 15, 2022
811c29d
Improve usage of Libdl
david-macmahon Jun 15, 2022
57c0e52
Use HDF5.API.libhdf5 rather than dllist()
david-macmahon Jun 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 19 additions & 3 deletions src/drivers/drivers.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ export POSIX
import ..API
import ..HDF5: HDF5, Properties, h5doc

using Libdl: dlopen, dlsym
using Requires: @require


Expand Down Expand Up @@ -85,9 +86,24 @@ function set_driver!(p::Properties, ::POSIX)
end

function __init__()
DRIVERS[API.h5fd_core_init()] = Core
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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks that makes sense.

ENV["HDF5_USE_FILE_LOCKING"] = "FALSE"
end

fapl = HDF5.init!(HDF5.FileAccessProperties())
API.h5p_set_fapl_core(fapl, 4096, false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does 4096 come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

DRIVERS[API.h5p_get_driver(fapl)] = Core
Comment on lines +95 to +96
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could lazily initialize Core as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

API.h5p_set_fapl_sec2(fapl)
DRIVERS[API.h5p_get_driver(fapl)] = POSIX
close(fapl)

# Check whether the libhdf5 was compiled with parallel support.
HDF5.HAS_PARALLEL[] = dlopen(HDF5.API.libhdf5) do lib
dlsym(lib, :H5Pset_fapl_mpio; throw_error=false) !== nothing
end

@require MPI="da04e1cc-30fd-572f-bb4f-1f8673147195" (HDF5.has_parallel() && include("mpio.jl"))
end

end # module
10 changes: 3 additions & 7 deletions src/drivers/mpio.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,6 @@ end
MPIO(comm::MPI.Comm; kwargs...) =
MPIO(comm, MPI.Info(;kwargs...))

# Check whether the HDF5 libraries were compiled with parallel support.
try
DRIVERS[API.h5fd_mpio_init()] = MPIO
HDF5.HAS_PARALLEL[] = true
catch e
end

function set_driver!(fapl::Properties, mpio::MPIO)
HDF5.has_parallel() || error(
"HDF5.jl has no parallel support." *
Expand All @@ -60,6 +53,9 @@ function set_driver!(fapl::Properties, mpio::MPIO)
GC.@preserve mpio begin
API.h5p_set_fapl_mpio(fapl, mpi_to_h5(mpio.comm), mpi_to_h5(mpio.info))
end

DRIVERS[API.h5p_get_driver(fapl)] = MPIO
return nothing
end

function get_driver(fapl::Properties, ::Type{MPIO})
Expand Down