add lock around all API calls #1021
Conversation
|
Would it make sense to provide this in two layers
Basically we run the generator twice and produce two separate modules. For the higher level API we may want to engage the locks for a series of low-level API calls rather than locking and unlocking for each API call. |
|
We could even use a Preferences.jl mechanism to enable locks everywhere or not by default. |
|
That's what I was originally thinking, but it seems like the overhead is small enough that it might not be worth it? |
|
I've tried some more simple test cases (e.g. reading/writing single values in a loop) and I don't see any observable overhead from the lock. |
| jlfuncbody = Expr( | ||
| :block, __source__, :(lock(liblock)), :($statsym = try | ||
| $ccallexpr | ||
| finally | ||
| unlock(liblock) | ||
| end) | ||
| ) |
There was a problem hiding this comment.
| jlfuncbody = Expr( | |
| :block, __source__, :(lock(liblock)), :($statsym = try | |
| $ccallexpr | |
| finally | |
| unlock(liblock) | |
| end) | |
| ) | |
| jlfuncbody = Expr( | |
| :block, __source__, :(use_lock() && lock(liblock)), :($statsym = try | |
| $ccallexpr | |
| finally | |
| use_lock && unlock(liblock) | |
| end) | |
| ) |
There was a problem hiding this comment.
This adds two runtime lookups at each call
There was a problem hiding this comment.
If use_lock is constant, (e.g. use_lock() = false), it gets inlined. The generated code does not actually call use_lock at runtime.
There was a problem hiding this comment.
Ah, I saw that's what you did in #1022. That's an option, but I would argue that using the lock should be the default.
| @@ -13,6 +13,8 @@ else | |||
| ) | |||
| end | |||
|
|
|||
| const liblock = ReentrantLock() | |||
There was a problem hiding this comment.
| const liblock = ReentrantLock() | |
| const liblock = ReentrantLock() | |
| const _USE_LOCK = Ref(true) | |
| use_lock() = _USE_LOCK[] |
|
@simonbyrne What's your reading of JuliaLang/julia#35689 ? It sounds to me like there may be an issue if finalizers acquire locks at the moment, with that issue still open. |
I think that is addressed by JuliaLang/julia#38487, which prevents finalizers from running on the current thread until the lock is released. |
|
That's strange that a pull request with a lower number resolves an issue with a higher number. |
|
@mkitti I haven't fully investigated the Preferences.jl solution, but this seems a lot simpler and since there's no tradeoff with these calls, I don't see a strong reason to make it configurable. Are there any use cases for users to want to turn it off other than devs wanting to experiment? |
|
The statement that "there is no tradeoff" still needs to be clearly established. Anecdotal testing does suggest that the effect is small, but we do not have a way of comprehensively establishing this for all applications. Thus, we need to provide a way for the user to do the experiment with their specific applications. The result "there is no tradeoff" does seem surprising. A single HDF5.jl function usually makes several C API calls. Locking and unlocking several times within a high level function call does seem inefficient. Is there really no cost to this at all? If there were no cost, then why not enable thread safety within the C API by default? |
|
With lock: Without lock: That suggests the tests with the lock in place could take 15% longer than without the lock in place. I think that's at the high end. Over a number of tests it looks like 5% with a range of 2% to 16%. |
|
Should we bump to 0.17 if we merge this? It does not look directly breaking to me, but it is a significant change that it could be. |
|
I think we can probably keep things at 0.16, this PR should be a strict improvement (sans minor speed regression) |
As a potential fix to #835 and #990: this adds a simple lock around all API calls: I was initially worried this would cause a performance bottleneck, but some simple examples didn't show any real difference.