Thread-safe garbage collection v3 (ReentrantLock version)

[MilesCranmer]

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:

const PYDECREF_LOCK = ReentrantLock()

function _pydecref_locked(po::PyObject)
    # 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

---

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?)

[codecov-commenter]

Codecov Report

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 ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1074      +/-   ##
==========================================
- Coverage   67.75%   67.73%   -0.02%     
==========================================
  Files          20       20              
  Lines        2025     2030       +5     
==========================================
+ Hits         1372     1375       +3     
- Misses        653      655       +2     

Flag	Coverage Δ
unittests	67.73% <80.00%> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mkitti]

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.

[MilesCranmer]

Wait isn't that what this already does? Or you mean replacing the lock with trylock?

[mkitti]

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).

[MilesCranmer]

Done!

[MilesCranmer]

(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.

[mkitti]

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?

[mkitti]

It might be time to drop Python 2.7 CI

[MilesCranmer]

It might be time to drop Python 2.7 CI

Done! #1075

[MilesCranmer]

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?

[mkitti]

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. /
PyGILState_Release(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.

[mkitti]

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.

[MilesCranmer]

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.

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)?

[MilesCranmer]

Pinging this thread before my teaching starts (and I get buried in work) :)

[mkitti]

@stevengj , have you had a chance to take a look?