Acquire GIL around every Python API call - #1097
topolarity wants to merge 3 commits into
Conversation
b3aea39 to
dd3cc6d
Compare
|
Before this can be considered complete, it needs an equivalent to JuliaMath/FFTW.jl#160 to handle:
edit: Done! |
49be855 to
bd8d220
Compare
Majority of the API is covered by updating the `@pycheck(v/n)` macros. The rest is covered manually by `@with_GIL`. Care should be taken to avoid yielding to the Julia scheduler, which could cause task migration and lead to deadlocks.
This is required to avoid deadlocking in the GC as it executes finalizers.
Required to avoid starving the scheduler or deadlocking against the GC.
|
Is there any known issue with this currently? Are we going to use this approach? I've rebased this on master at https://github.com/JuliaPy/PyCall.jl/tree/ct/with-gil-rebase with a few fixes/tweaks.
One of the few issues I could think of is maybe if there are a lot of object that became garbage they can only be free'd the next time the user calls a PyCall API, which may not happen for a while. For example, the following code can place unbound number of objects in the deferred list, [PyObject(i) for i in 1:100000];Maybe the finalizer should detect some cases where the GIL is safe to acquire (e.g. try to acquire without waiting) and clear the queue? |
|
I've force pushed my rebased to the original branch. As I said in #1096 (comment), unless someone come up with a better solution soon, I'd like to merge this in a few days. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1097 +/- ##
==========================================
+ Coverage 68.02% 70.28% +2.25%
==========================================
Files 20 21 +1
Lines 2036 1992 -44
==========================================
+ Hits 1385 1400 +15
+ Misses 651 592 -59
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks for revisiting / rebasing @yuyichao I am not aware of any problems with this, other than your latest fixups / tweaks. I think it's the direction we want to go in. I'll try to reboot the context and take a look at the changes on Monday to see if we need anything else. Otherwise, sounds good to me! |
This should resolve #1096 by wrapping every Python API usage in a GIL acquire-release.
If this leads to overhead, we can probably re-export a "manual GIL" version of the API that allows consumers to manage the GIL themselves. In any case, it's worth having proper support for multi-threading.
Fixes #1096. Fixes #1095. Fixes #1090. Fixes #1088.
Fixes #1078. Fixes #1077. Fixes #1072. Fixes #1083.
Fixes timholy/Revise.jl#989.