Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Thread-safe garbage collection v3 (ReentrantLock version) #1074

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

MilesCranmer
Copy link
Contributor

@MilesCranmer MilesCranmer commented Jan 8, 2024

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
Copy link

codecov-commenter commented Jan 8, 2024

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
Copy link
Member

mkitti commented Jan 8, 2024

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
Copy link
Contributor Author

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

@mkitti
Copy link
Member

mkitti commented Jan 8, 2024

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
Copy link
Contributor Author

Done!

@MilesCranmer
Copy link
Contributor Author

MilesCranmer commented Jan 8, 2024

(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
Copy link
Member

mkitti commented Jan 8, 2024

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
Copy link
Member

mkitti commented Jan 8, 2024

It might be time to drop Python 2.7 CI

@MilesCranmer
Copy link
Contributor Author

It might be time to drop Python 2.7 CI

Done! #1075

@MilesCranmer
Copy link
Contributor Author

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
Copy link
Member

mkitti commented Jan 8, 2024

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
Copy link
Member

mkitti commented Jan 8, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@mkitti
Copy link
Member

mkitti commented Jan 16, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants