GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-110481: Implement inter-thread queue for biased reference counting by colesbury · Pull Request #114824 · python/cpython

Conversation

@colesbury

@colesbury colesbury commented Jan 31, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

  • Issue: Implement biased reference counting in --disable-gil builds #110481

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@colesbury
colesbury requested review from DinoV and corona10 January 31, 2024 22:45
@colesbury
colesbury requested review from a team, ericsnowcurrently, markshannon and methane as code owners January 31, 2024 22:45
@bedevere-app bedevere-app Bot mentioned this pull request Jan 31, 2024
Implement biased reference counting in --disable-gil builds #110481
Closed
@bedevere-app bedevere-app Bot added the awaiting review label Jan 31, 2024
@colesbury colesbury added topic-free-threading skip news and removed awaiting review labels Jan 31, 2024
@corona10

corona10 commented Feb 1, 2024

Copy link
Copy Markdown
Member

I will take a look by tomorrow :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@colesbury
pythongh-110481: Implement inter-thread queue for biased reference co…
544adf2 
…unting
@colesbury
colesbury force-pushed the gh-110481-inter-thread-queue branch from 2373401 to 544adf2 Compare February 1, 2024 16:53
@ericsnowcurrently

ericsnowcurrently commented Feb 1, 2024

Copy link
Copy Markdown
Member

FTR, biased refcounting was implemented in gh-110764.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ericsnowcurrently
ericsnowcurrently requested changes Feb 1, 2024
View reviewed changes

@ericsnowcurrently ericsnowcurrently left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Uh oh!

There was an error while loading. Please reload this page.
gpshead reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Include/internal/pycore_object_stack.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/brc.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/brc.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/brc.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/brc.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Include/internal/pycore_brc.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Include/internal/pycore_brc.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Include/internal/pycore_brc.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Include/internal/pycore_brc.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/brc.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app

bedevere-app Bot commented Feb 1, 2024

Copy link
Copy Markdown

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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot added the awaiting changes label Feb 1, 2024
@ericsnowcurrently

ericsnowcurrently commented Feb 1, 2024

Copy link
Copy Markdown
Member

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
colesbury added 2 commits February 2, 2024 16:32
@colesbury
Update comments from review
ee01891
@colesbury
Fix _Py_brc_remove_thread
cbfaf6b
colesbury
colesbury commented Feb 2, 2024
View reviewed changes

@colesbury colesbury left a comment

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Uh oh!

There was an error while loading. Please reload this page.
ericsnowcurrently and gpshead reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
Comment thread Include/internal/pycore_brc.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Include/internal/pycore_object_stack.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_code.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/brc.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/brc.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/brc.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/brc.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/gc_free_threading.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/gc_free_threading.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/brc.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@colesbury
Include pycore_freelist.h in pycore_object_stack.h
127c595
@colesbury
colesbury requested a review from ericsnowcurrently February 5, 2024 17:53
@colesbury

colesbury commented Feb 5, 2024

Copy link
Copy Markdown
Contributor Author

The "Ubuntu (free-threading)" build crashes due to the problem described in #115035. I think that will need to be fixed before this.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
colesbury added 3 commits February 5, 2024 21:32
@colesbury
Move _Py_brc_after_fork up
f00252a
@colesbury
Merge branch 'main' into pythongh-110481-inter-thread-queue
bc68b9a
@colesbury
Merge branch 'main' into pythongh-110481-inter-thread-queue
459638f
@colesbury

colesbury commented Feb 7, 2024

Copy link
Copy Markdown
Contributor Author

Hi @ericsnowcurrently, I believe I've addressed the comments from your review. Would you please take another look?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ericsnowcurrently
ericsnowcurrently approved these changes Feb 8, 2024
View reviewed changes

@ericsnowcurrently ericsnowcurrently left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
colesbury and gpshead reacted with heart emoji
All reactions
  • ❤️ 2 reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting changes labels Feb 8, 2024
@colesbury

colesbury commented Feb 8, 2024

Copy link
Copy Markdown
Contributor Author

!buildbot nogil
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot

bedevere-bot commented Feb 8, 2024

Copy link
Copy Markdown

🤖 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:

  • x86-64 MacOS Intel NoGIL PR
  • AMD64 Ubuntu NoGIL Refleaks PR
  • ARM64 MacOS M1 Refleaks NoGIL PR
  • AMD64 Ubuntu NoGIL PR
  • AMD64 Windows Server 2022 NoGIL PR
  • x86-64 MacOS Intel ASAN NoGIL PR
  • ARM64 MacOS M1 NoGIL PR
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@colesbury
Fix refleak tests.
51c6625 
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 colesbury added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Feb 8, 2024
@bedevere-bot

bedevere-bot commented Feb 8, 2024

Copy link
Copy Markdown

🤖 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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Feb 8, 2024
@colesbury
Fix test_weakref.test_threaded_weak_key_dict_copy refleak
c631856
@colesbury colesbury added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Feb 8, 2024
@bedevere-bot

bedevere-bot commented Feb 8, 2024

Copy link
Copy Markdown

🤖 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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Feb 8, 2024
@colesbury colesbury mentioned this pull request Feb 8, 2024
Fix refleak tracking in free-threaded build #115184
Closed
corona10
corona10 approved these changes Feb 8, 2024
View reviewed changes
@colesbury colesbury mentioned this pull request Feb 9, 2024
gh-115184: Fix refleak tracking issues in free-threaded build #115188
Merged
@colesbury
Merge branch 'main' into pythongh-110481-inter-thread-queue
8311e18
@colesbury colesbury added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Feb 9, 2024
@bedevere-bot

bedevere-bot commented Feb 9, 2024

Copy link
Copy Markdown

🤖 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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Feb 9, 2024
@colesbury
Remove duplciate _Py_AddRefTotal from merge commit
97cf964
@colesbury colesbury added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Feb 9, 2024
@bedevere-bot

bedevere-bot commented Feb 9, 2024

Copy link
Copy Markdown

🤖 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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Feb 9, 2024
@colesbury

colesbury commented Feb 9, 2024

Copy link
Copy Markdown
Contributor Author

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 😅
gpshead reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@colesbury
colesbury merged commit a3af3cb into python:main Feb 9, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Feb 9, 2024
@colesbury
colesbury deleted the gh-110481-inter-thread-queue branch February 9, 2024 22:09
fsc-eriker pushed a commit to fsc-eriker/cpython that referenced this pull request Feb 14, 2024
@colesbury @fsc-eriker
pythongh-110481: Implement inter-thread queue for biased reference co…
78819a9 
…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.
@colesbury colesbury mentioned this pull request Apr 9, 2024
ThreadPoolExecutorTest.test_no_stale_references may hang with GIL disabled #117688
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ericsnowcurrently ericsnowcurrently ericsnowcurrently approved these changes
@corona10 corona10 corona10 approved these changes
@DinoV DinoV Awaiting requested review from DinoV
@methane methane Awaiting requested review from methane methane is a code owner
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news topic-free-threading

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@colesbury @corona10 @ericsnowcurrently @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.