search

JuliaIO / HDF5.jl

Save and load data in the HDF5 file format from Julia
https://juliaio.github.io/HDF5.jl
MIT License
386 stars 140 forks source link

API Lock Preferences #1022

Closed mkitti closed 1 year ago

mkitti commented 1 year ago

This builds on the sb/lock branch by using Preferences.jl to configure the lock usage at compile time.

The preference, use_api_lock can be set to either true or false.

The preference is set via the API.set_use_lock_pref! function and can be quered by API.get_use_lock_pref().

This mainly meant to help assess the sb/lock branch.

Other features such as the RawAPI module have been withdrawn.

simonbyrne commented 1 year ago

This adds a bunch of complexity, and 2 additional runtime lookups at each call. Personally i don't think it's worth it (especially since I haven't been able to see any noticeable overhead with the locking).

I think if you want to make it optional, just make it an all-or-nothing: use_api_lock is true/false with

const use_api_lock = @load_preference("use_api_lock", true)

and make the API function wrappers:

    use_api_lock && lock(liblock)
    var"#status#" = try
            # ccall
        finally
            use_api_lock && unlock(liblock)
        end
    var"#status#" < 0 && @h5error ...
    return nothing
mkitti commented 1 year ago

After testing, I am getting this error. I suspect this is because the finalizer is trying to run after we call h5_close(). https://github.com/JuliaIO/HDF5.jl/blob/af62ebf5b87a347edab6f903c2f7dced3ed03e25/test/runtests.jl#L77

error in running finalizer: HDF5.API.H5Error(msg="Error closing file", id=1008806316530991104)
macro expansion at /home/mkitti/.julia/dev/HDF5/src/api/error.jl:18 [inlined]
h5f_close at /home/mkitti/.julia/dev/HDF5/src/api/functions.jl:1054
close at /home/mkitti/.julia/dev/HDF5/src/file.jl:120 [inlined]
lock_and_close at /home/mkitti/.julia/dev/HDF5/src/api/lock.jl:58
unknown function (ip: 0x7f8eb5b0b122)
_jl_invoke at /cache/build/default-amdci5-0/julialang/julia-release-1-dot-8/src/gf.c:2367 [inlined]
ijl_apply_generic at /cache/build/default-amdci5-0/julialang/julia-release-1-dot-8/src/gf.c:2549
jl_apply at /cache/build/default-amdci5-0/julialang/julia-release-1-dot-8/src/julia.h:1838 [inlined]
run_finalizer at /cache/build/default-amdci5-0/julialang/julia-release-1-dot-8/src/gc.c:280
jl_gc_run_finalizers_in_list at /cache/build/default-amdci5-0/julialang/julia-release-1-dot-8/src/gc.c:367
run_finalizers at /cache/build/default-amdci5-0/julialang/julia-release-1-dot-8/src/gc.c:410
ijl_atexit_hook at /cache/build/default-amdci5-0/julialang/julia-release-1-dot-8/src/init.c:236
jl_repl_entrypoint at /cache/build/default-amdci5-0/julialang/julia-release-1-dot-8/src/jlapi.c:720
main at /cache/build/default-amdci5-0/julialang/julia-release-1-dot-8/cli/loader_exe.c:59
unknown function (ip: 0x7f8f0cec8d8f)
__libc_start_main at /lib/x86_64-linux-gnu/libc.so.6 (unknown line)
unknown function (ip: 0x401098)
simonbyrne commented 1 year ago

Is there a reason why the tests call HDF5.API.h5_close()?

mkitti commented 1 year ago

@simonbyrne I simplified this along the lines of your https://github.com/JuliaIO/HDF5.jl/pull/1022#issuecomment-1306547077 . I think this will allow us to more easily diagnose any potential issues or regressions that might emerge from this.

I also removed the RawAPI module. Something I would like to pay particular attention to is the use of the iterate functions where the C library is calling back into Julia code. Do we have sufficient guarantees on what thread the calls back will be run on?

mkitti commented 1 year ago

Is there a reason why the tests call HDF5.API.h5_close()?

I don't know, but it sure does help catch unclosed handles.

mkitti commented 1 year ago

Closing and resubmitting PR directly to master.