Repository navigation
gh-110481: Implement inter-thread queue for biased reference counting - #114824
colesbury merged 11 commits into
Conversation
Sorry, something went wrong.
|
I will take a look by tomorrow :) |
Sorry, something went wrong.
|
FTR, biased refcounting was implemented in gh-110764. |
Sorry, something went wrong.
ericsnowcurrently
left a comment
There was a problem hiding this comment.
The approach generally makes sense. I'm pretty sure I was able to follow along. There were a few places where I'm concerned there may be some extra complexity with multiple interpreters but they may be non-issues. I've left a handful of other minor comments.
Sorry, something went wrong.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request. And if you don't make the requested changes, you will be put in the comfy chair! |
Sorry, something went wrong.
|
FWIW, every once in a while I'm reminded about the extra complexity that no-gil brings to GC and refcounting. I still worry that the extra complexity will have a meaningful long-term impact on maintainability for CPython and community extension modules. Is there anything we can do to help contributors/maintainers understand how it all works under no-gil and especially how to track down leaks/crashes without breaking their brains? Is there any extra tooling that would help? |
Sorry, something went wrong.
colesbury
left a comment
There was a problem hiding this comment.
Is there anything we can do to help contributors/maintainers understand how it all works under no-gil and especially how to track down leaks/crashes without breaking their brains? Is there any extra tooling that would help?
There are some updates to the devguide (for GC) under review. We'll want to do more along with user facing docs, but I think these are best addressed holistically once the integration is farther along.
On tooling, we already have a nogil refleaks buildbot. We'll have a thread sanitizer buildbot after we get to the point where we can actually disable the GIL.
Sorry, something went wrong.
|
The "Ubuntu (free-threading)" build crashes due to the problem described in #115035. I think that will need to be fixed before this. |
Sorry, something went wrong.
|
Hi @ericsnowcurrently, I believe I've addressed the comments from your review. Would you please take another look? |
Sorry, something went wrong.
ericsnowcurrently
left a comment
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
|
!buildbot nogil |
Sorry, something went wrong.
|
🤖 New build scheduled with the buildbot fleet by @colesbury for commit 459638f 🤖 The command will test the builders whose names match following regular expression: nogil The builders matched are:
|
Sorry, something went wrong.
Revert back to Py_DECREF() instead of the Py_SET_REFCNT(dict, 0) and fix check that this thread owns the only reference to the dict. That ensures that the final Py_DECREF call immediately frees the dict instead of possibly enqueuing it to be merged.
|
🤖 New build scheduled with the buildbot fleet by @colesbury for commit 51c6625 🤖 If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again. |
Sorry, something went wrong.
|
🤖 New build scheduled with the buildbot fleet by @colesbury for commit c631856 🤖 If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again. |
Sorry, something went wrong.
|
🤖 New build scheduled with the buildbot fleet by @colesbury for commit 8311e18 🤖 If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again. |
Sorry, something went wrong.
|
🤖 New build scheduled with the buildbot fleet by @colesbury for commit 97cf964 🤖 If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again. |
Sorry, something went wrong.
|
There were some issues with refleak tracking. After some failed attempts at whack-a-mole, I addressed the refleak issues in a separate PR (#115188), and re-merged main back into this PR. It looks like the buildbots are finally passing 😅 |
Sorry, something went wrong.
…unting (python#114824) Biased reference counting maintains two refcount fields in each object: `ob_ref_local` and `ob_ref_shared`. The true refcount is the sum of these two fields. In some cases, when refcounting operations are split across threads, the ob_ref_shared field can be negative (although the total refcount must be at least zero). In this case, the thread that decremented the refcount requests that the owning thread give up ownership and merge the refcount fields.
Biased reference counting maintains two refcount fields in each object: ob_ref_local and ob_ref_shared. The true refcount is the sum of these two fields. In some cases, when refcounting operations are split across threads,
the ob_ref_shared field can be negative (although the total refcount must be at least zero). In this case, the thread that decremented the refcount requests that the owning thread give up ownership and merge the refcount fields.