gh-110481: Implement inter-thread queue for biased reference counting #114824
Conversation
colesbury
requested review from
a team,
ericsnowcurrently,
methane and
markshannon
as code owners
|
I will take a look by tomorrow :) |
colesbury
force-pushed
the
gh-110481-inter-thread-queue
branch
from
2373401 to
544adf2
Compare
|
FTR, biased refcounting was implemented in gh-110764. |
|
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 And if you don't make the requested changes, you will be put in the comfy chair! |
|
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? |
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.
|
The "Ubuntu (free-threading)" build crashes due to the problem described in #115035. I think that will need to be fixed before this. |
|
Hi @ericsnowcurrently, I believe I've addressed the comments from your review. Would you please take another look? |
|
!buildbot nogil |
|
🤖 New build scheduled with the buildbot fleet by @colesbury for commit 459638f 🤖 The command will test the builders whose names match following regular expression: The builders matched are:
|
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.
colesbury
added
the
🔨 test-with-refleak-buildbots
|
🤖 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. |
bedevere-bot
removed
the
🔨 test-with-refleak-buildbots
colesbury
added
the
🔨 test-with-refleak-buildbots
|
🤖 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. |
bedevere-bot
removed
the
🔨 test-with-refleak-buildbots
colesbury
added
the
🔨 test-with-refleak-buildbots
|
🤖 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. |
bedevere-bot
removed
the
🔨 test-with-refleak-buildbots
colesbury
added
the
🔨 test-with-refleak-buildbots
|
🤖 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. |
bedevere-bot
removed
the
🔨 test-with-refleak-buildbots
|
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 😅 |
…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_localandob_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.
--disable-gilbuilds #110481