Open MilesCranmer opened 8 months ago
Attention: 2 lines
in your changes are missing coverage. Please review.
Comparison is base (
e54c4ee
) 67.75% compared to head (d7181d5
) 67.73%. Report is 1 commits behind head on master.
Files | Patch % | Lines |
---|---|---|
src/PyCall.jl | 75.00% | 2 Missing :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
You might want to try a test-and-test-set pattern: https://github.com/JuliaIO/HDF5.jl/blob/master/src/api/lock.jl
Basically islocked
is cheap, so check that first. If it is not locked, then try to lock.
Wait isn't that what this already does? Or you mean replacing the lock
with trylock
?
Yes, use trylock
.
help?> trylock
search: trylock
trylock(lock) -> Success (Boolean)
Acquire the lock if it is available, and return true if successful. If the
lock is already locked by a different task/thread, return false.
Each successful trylock must be matched by an unlock.
Function trylock combined with islocked can be used for writing the
test-and-test-and-set or exponential backoff algorithms if it is supported
by the typeof(lock) (read its documentation).
Done!
(Edit: I implemented this as it seems the safer option. Not sure what happens when two threads try to write PyBuffer_Release
simultaneously.)
Old: do we need to make the PyBuffer
deallocator thread-safe as well? Right now it's this:
mutable struct PyBuffer
buf::Py_buffer
PyBuffer() = begin
b = new(Py_buffer(C_NULL, PyPtr_NULL, 0, 0,
0, 0, C_NULL, C_NULL, C_NULL, C_NULL,
C_NULL, C_NULL, C_NULL))
finalizer(pydecref, b)
return b
end
end
which references:
function pydecref(o::PyBuffer)
# note that PyBuffer_Release sets o.obj to NULL, and
# is a no-op if o.obj is already NULL
_finalized[] || ccall(@pysym(:PyBuffer_Release), Cvoid, (Ref{PyBuffer},), o)
o
end
Not sure if this is needed or not.
By the way, do you know what the threading advice actually is for the Python C API. Is it only one thread at a time, or only call from the same thread?
It might be time to drop Python 2.7 CI
It might be time to drop Python 2.7 CI
Done! #1075
By the way, do you know what the threading advice actually is for the Python C API. Is it only one thread at a time, or only call from the same thread?
I don't see anything related to multi-threading here https://docs.python.org/3/c-api/refcounting.html. Would they have a particular requirement?
https://docs.python.org/3/c-api/init.html#non-python-created-threads
If you need to call Python code from these threads (often this will be part of a callback API provided by the aforementioned third-party library), you must first register these threads with the interpreter by creating a thread state data structure, then acquiring the GIL, and finally storing their thread state pointer, before you can start using the Python/C API. When you are done, you should reset the thread state pointer, release the GIL, and finally free the thread state data structure.
The PyGILState_Ensure() and PyGILState_Release() functions do all of the above automatically. The typical idiom for calling into Python from a C thread is:
PyGILState_STATE gstate; gstate = PyGILState_Ensure();
/ Perform Python actions here. / result = CallSomeFunction(); / evaluate result or handle exception /
/ Release the thread. No Python API allowed beyond this point. / PyGILStateRelease(gstate); Note that the PyGILState functions assume there is only one global interpreter (created automatically by Py_Initialize()). Python supports the creation of additional interpreters (using Py_NewInterpreter()), but mixing multiple interpreters and the PyGILState_ API is unsupported.
Ok, we're going to need a redesign here since Julia GC is now multithreaded.
Now the question is do we really PyGILState_STATE
per thread, or can we manage per Task
.
Ok, we're going to need a redesign here since Julia GC is now multithreaded.
Now the question is do we really
PyGILState_STATE
per thread, or can we manage perTask
.
I feel like it would be quite a bit of work to properly incorporate the Python GIL into Julia code. That means you would need to thread-lock every single part of PyCall.jl, no?
Thread-locking the garbage collection is the major issue IMO, but it also seems easier to solve (with just this PR)?
Pinging this thread before my teaching starts (and I get buried in work) :)
@stevengj , have you had a chance to take a look?
This is an alternative strategy to make the GC thread-safe compared with #1073 (#883).
@stevengj @mkitti @marius311
My concern with #1073 is whether only allowing GC to run on a particular thread might cause an issue or lag in performing garbage collection. Instead this allows the GC to run on any thread but they must obtain a lock first. This is the logic:
One question I have is: does
islocked
work in a re-entrant context? (Or does it even matter, and I could just use a SpinLock?)