From cb334ca16898d89a3f6de8d2d6e05bb05cc73dc0 Mon Sep 17 00:00:00 2001 From: MilesCranmer Date: Mon, 8 Jan 2024 19:07:13 +0000 Subject: [PATCH 1/5] Create ReentrantLock version of garbage collector --- src/PyCall.jl | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/PyCall.jl b/src/PyCall.jl index f1987a29..f22094c5 100644 --- a/src/PyCall.jl +++ b/src/PyCall.jl @@ -76,11 +76,26 @@ mutable struct PyObject o::PyPtr # the actual PyObject* function PyObject(o::PyPtr) po = new(o) - finalizer(pydecref, po) + finalizer(_pydecref_locked, po) return po end end +const PYDECREF_LOCK = ReentrantLock() + +function _pydecref_locked(po::PyObject) + if !islocked(PYDECREF_LOCK) + # If available, we lock and decref + lock(PYDECREF_LOCK) do + pydecref_(po) + end + else + # Add back to queue to be decref'd later + finalizer(_pydecref_locked, po) + end + return nothing +end + PyPtr(o::PyObject) = getfield(o, :o) """ From 9dbe833c23840ab91577399a8e26cd99db3b40df Mon Sep 17 00:00:00 2001 From: MilesCranmer Date: Mon, 8 Jan 2024 20:05:24 +0000 Subject: [PATCH 2/5] Use test-and-test-set pattern --- src/PyCall.jl | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/PyCall.jl b/src/PyCall.jl index f22094c5..5d2165fa 100644 --- a/src/PyCall.jl +++ b/src/PyCall.jl @@ -84,15 +84,13 @@ end const PYDECREF_LOCK = ReentrantLock() function _pydecref_locked(po::PyObject) - if !islocked(PYDECREF_LOCK) - # If available, we lock and decref - lock(PYDECREF_LOCK) do - pydecref_(po) - end - else - # Add back to queue to be decref'd later - finalizer(_pydecref_locked, po) - end + # If available, we lock and decref + !islocked(PYDECREF_LOCK) && + trylock(() -> pydecref_(po), PYDECREF_LOCK) && + return nothing + + # Add back to queue to be decref'd later + finalizer(_pydecref_locked, po) return nothing end From 0a7de84203e5458b0ddd31404a0abe5470ddd63b Mon Sep 17 00:00:00 2001 From: MilesCranmer Date: Mon, 8 Jan 2024 20:14:06 +0000 Subject: [PATCH 3/5] Earlier julia expects trylock to be passed `true` --- src/PyCall.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/PyCall.jl b/src/PyCall.jl index 5d2165fa..76b041c0 100644 --- a/src/PyCall.jl +++ b/src/PyCall.jl @@ -86,7 +86,7 @@ const PYDECREF_LOCK = ReentrantLock() function _pydecref_locked(po::PyObject) # If available, we lock and decref !islocked(PYDECREF_LOCK) && - trylock(() -> pydecref_(po), PYDECREF_LOCK) && + trylock(() -> (pydecref_(po); true), PYDECREF_LOCK) && return nothing # Add back to queue to be decref'd later From d7181d55f20a36fdc5cfc913a046939dafa15992 Mon Sep 17 00:00:00 2001 From: MilesCranmer Date: Mon, 8 Jan 2024 20:26:39 +0000 Subject: [PATCH 4/5] Make `pydecref` more idiomatic --- benchmarks/pywrapfn.jl | 2 +- src/PyCall.jl | 12 ++++++------ src/pyfncall.jl | 4 ++-- src/pyinit.jl | 2 +- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/benchmarks/pywrapfn.jl b/benchmarks/pywrapfn.jl index 9fd2149b..4a20c424 100644 --- a/benchmarks/pywrapfn.jl +++ b/benchmarks/pywrapfn.jl @@ -81,7 +81,7 @@ See check_pyargsptr(nargs::Int) above """ function check_pyargsptr(pf::PyWrapFn{N, RT}) where {N, RT} if unsafe_load(pf.pyargsptr).ob_refcnt > 1 - pydecref_(pf.pyargsptr) + pydecref_unsafe_(pf.pyargsptr) pf.pyargsptr = @pycheckn ccall((@pysym :PyTuple_New), PyPtr, (Int,), nargs) end diff --git a/src/PyCall.jl b/src/PyCall.jl index 76b041c0..f0d7a45f 100644 --- a/src/PyCall.jl +++ b/src/PyCall.jl @@ -76,21 +76,21 @@ mutable struct PyObject o::PyPtr # the actual PyObject* function PyObject(o::PyPtr) po = new(o) - finalizer(_pydecref_locked, po) + finalizer(pydecref_safe_, po) return po end end const PYDECREF_LOCK = ReentrantLock() -function _pydecref_locked(po::PyObject) +function pydecref_safe_(po::PyObject) # If available, we lock and decref !islocked(PYDECREF_LOCK) && - trylock(() -> (pydecref_(po); true), PYDECREF_LOCK) && + trylock(() -> (pydecref_unsafe_(po); true), PYDECREF_LOCK) && return nothing # Add back to queue to be decref'd later - finalizer(_pydecref_locked, po) + finalizer(pydecref_safe_, po) return nothing end @@ -127,13 +127,13 @@ it is equivalent to a `PyNULL()` object. """ ispynull(o::PyObject) = o ≛ PyPtr_NULL -function pydecref_(o::Union{PyPtr,PyObject}) +function pydecref_unsafe_(o::Union{PyPtr,PyObject}) _finalized[] || ccall(@pysym(:Py_DecRef), Cvoid, (PyPtr,), o) return o end function pydecref(o::PyObject) - pydecref_(o) + pydecref_unsafe_(o) setfield!(o, :o, PyPtr_NULL) return o end diff --git a/src/pyfncall.jl b/src/pyfncall.jl index 11e81287..a70aad05 100644 --- a/src/pyfncall.jl +++ b/src/pyfncall.jl @@ -28,7 +28,7 @@ function _pycall!(ret::PyObject, o::Union{PyObject,PyPtr}, args, nargs::Int=leng end return __pycall!(ret, pyargsptr, o, kw) #::PyObject finally - pydecref_(pyargsptr) + pydecref_unsafe_(pyargsptr) end end @@ -42,7 +42,7 @@ function __pycall!(ret::PyObject, pyargsptr::PyPtr, o::Union{PyObject,PyPtr}, disable_sigint() do retptr = @pycheckn ccall((@pysym :PyObject_Call), PyPtr, (PyPtr,PyPtr,PyPtr), o, pyargsptr, kw) - pydecref_(ret) + pydecref_unsafe_(ret) setfield!(ret, :o, retptr) end return ret #::PyObject diff --git a/src/pyinit.jl b/src/pyinit.jl index b3371a43..0f3b2987 100644 --- a/src/pyinit.jl +++ b/src/pyinit.jl @@ -110,7 +110,7 @@ end ######################################################################### const _finalized = Ref(false) -# This flag is set via `Py_AtExit` to avoid calling `pydecref_` after +# This flag is set via `Py_AtExit` to avoid calling `pydecref_unsafe_` after # Python is finalized. function _set_finalized() From 2cb63045193593662f3b4b58500c998b88490058 Mon Sep 17 00:00:00 2001 From: MilesCranmer Date: Mon, 8 Jan 2024 20:39:23 +0000 Subject: [PATCH 5/5] Thread safety for buffer deallocation --- src/PyCall.jl | 6 +++--- src/pybuffer.jl | 15 ++++++++++++++- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/src/PyCall.jl b/src/PyCall.jl index f0d7a45f..57dbc0a6 100644 --- a/src/PyCall.jl +++ b/src/PyCall.jl @@ -81,12 +81,12 @@ mutable struct PyObject end end -const PYDECREF_LOCK = ReentrantLock() +const PYDECREF_PYOBJECT_LOCK = ReentrantLock() function pydecref_safe_(po::PyObject) # If available, we lock and decref - !islocked(PYDECREF_LOCK) && - trylock(() -> (pydecref_unsafe_(po); true), PYDECREF_LOCK) && + !islocked(PYDECREF_PYOBJECT_LOCK) && + trylock(() -> (pydecref_unsafe_(po); true), PYDECREF_PYOBJECT_LOCK) && return nothing # Add back to queue to be decref'd later diff --git a/src/pybuffer.jl b/src/pybuffer.jl index a2cec72d..21ed99cd 100644 --- a/src/pybuffer.jl +++ b/src/pybuffer.jl @@ -32,11 +32,24 @@ mutable struct PyBuffer 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) + finalizer(pydecref_safe_, b) return b end end +const PYDECREF_PYBUFFER_LOCK = ReentrantLock() + +function pydecref_safe_(b::PyBuffer) + # If available, we lock and decref + !islocked(PYDECREF_PYBUFFER_LOCK) && + trylock(() -> (pydecref(b); true), PYDECREF_PYBUFFER_LOCK) && + return nothing + + # Add back to queue to be decref'd later + finalizer(pydecref_safe_, b) + return nothing +end + """ `pydecref(o::PyBuffer)` Release the reference to buffer `o`