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-112529: Use atomic operations for `gcstate->collecting` by colesbury · Pull Request #112533 · python/cpython

Conversation

@colesbury

@colesbury colesbury commented Nov 29, 2023 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

The collecting field in GCState is used to prevent overlapping garbage collections within the same interpreter. This is updated to use atomic operations in order to be thread-safe in --disable-gil builds.

The GC code is refactored a bit to support this. More of the logic is pushed down to gc_collect_main() so that we can safely order the logic setting collecting, the selection of the generation, and the invocation of callbacks with respect to the atomic operations and the (future) stop-the-world pauses.

The change uses atomic operations for both --disable-gil and the default build (with the GIL) to avoid extra #ifdef guards and ease the maintenance burden.

  • Issue: Make the garbage collector thread-safe in --disable-gil builds #112529

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@colesbury
pythongh-112529: Use atomic operations for gcstate->collecting
a88fd5e 
The `collecting` field in `GCState` is used to prevent overlapping garbage
collections within the same interpreter. This is updated to use atomic
operations in order to be thread-safe in `--disable-gil` builds.

The GC code is refactored a bit to support this. More of the logic is pushed
down to `gc_collect_main()` so that we can safely order the logic setting
`collecting`, the selection of the generation, and the invocation of callbacks
with respect to the atomic operations and the (future) stop-the-world pauses.

The change uses atomic operations for both `--disable-gil` and the default
build (with the GIL) to avoid extra `#ifdef` guards and ease the maintenance
burden.
@bedevere-app bedevere-app Bot mentioned this pull request Nov 29, 2023
Make the garbage collector thread-safe in --disable-gil builds #112529
Closed
3 tasks
@colesbury colesbury added 3.13 only security fixes topic-free-threading skip news labels Nov 29, 2023
colesbury
colesbury commented Nov 29, 2023
View reviewed changes
Comment thread Modules/gcmodule.c
void
_Py_ScheduleGC(PyInterpreterState *interp)
{
GCState *gcstate = &interp->gc;

@colesbury colesbury Nov 29, 2023

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.

fyi: _Py_ScheduleGC not public and only called from _PyObject_GC_Link, which already checks gcstate->collecting.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@colesbury
colesbury marked this pull request as ready for review November 29, 2023 18:48
@colesbury
colesbury requested a review from pablogsal as a code owner November 29, 2023 18:48
@bedevere-app bedevere-app Bot added the awaiting review label Nov 29, 2023
@colesbury

colesbury commented Nov 29, 2023

Copy link
Copy Markdown
Contributor Author

@pablogsal, would you be able to review this? The important bit is the atomically setting gcstate->collecting for thread-safety in --disable-gil builds, but there's a bunch of code that's moved around to support this.

cc @nascheme in case you have time and are interested in the GC changes, I would appreciate your feedback as well.
All reactions

Sorry, something went wrong.

Uh oh!

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

pablogsal commented Nov 29, 2023

Copy link
Copy Markdown
Member

@pablogsal, would you be able to review this? The important bit is the atomically setting gcstate->collecting for thread-safety in --disable-gil builds, but there's a bunch of code that's moved around to support this.

will review this week 👍
All reactions

Sorry, something went wrong.

Uh oh!

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

colesbury commented Dec 6, 2023

Copy link
Copy Markdown
Contributor Author

@pablogsal - gentle reminder, this is awaiting your review
All reactions

Sorry, something went wrong.

Uh oh!

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

nascheme commented Dec 8, 2023

Copy link
Copy Markdown
Member

This looks okay to me. It's a bit hard to see from the diff which code has been moved vs what's been changed. However, it seems to be mostly re-organization with essentially no behaviour change, aside from using the atomics. I'd say it can be merged.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
chris-eibl
chris-eibl reviewed Dec 9, 2023
View reviewed changes
Comment thread Modules/gcmodule.c Outdated
{
GC_STAT_ADD(generation, collections, 1);
#ifdef Py_STATS
if (_Py_stats) {

@chris-eibl chris-eibl Dec 9, 2023 •
edited
Loading

Uh oh!

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

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.

Shouldn't object_visits be zeroed after the _Py_atomic_compare_exchange_int(&gcstate->collecting check or the check be moved up?

Sorry, something went wrong.

Uh oh!

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

@colesbury colesbury Dec 11, 2023

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.

Yes, thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@chris-eibl

chris-eibl commented Dec 9, 2023

Copy link
Copy Markdown
Member

I think using forward declarations might help to have less moved blocks and make the review easier?
All reactions

Sorry, something went wrong.

Uh oh!

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

nascheme commented Dec 9, 2023

Copy link
Copy Markdown
Member

I think using forward declarations might help to have less moved blocks and make the review easier?

Probably it would but I'd rather the code is cleaner (without the forward decs) and have the patch be messier.
All reactions

Sorry, something went wrong.

Uh oh!

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

nascheme commented Dec 9, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

I rewrote the commit to use forward declarations (as suggested by Chris), makes the review easier:

nascheme@d53784d

Aside from re-ordering the code and addition the forward defs, I didn't change anything.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pablogsal
pablogsal approved these changes Dec 11, 2023
View reviewed changes

@pablogsal pablogsal 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.

I finally had time to review this. Apologies for the delay.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Dec 11, 2023
@pablogsal

pablogsal commented Dec 11, 2023

Copy link
Copy Markdown
Member

We can probably go with the version with forward references although I don't think it makes a lot of improvements in the final version, is true that the review is easier to do on the diff alone.
All reactions

Sorry, something went wrong.

Uh oh!

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

nascheme commented Dec 11, 2023

Copy link
Copy Markdown
Member

The forward ref version was meant only for review purposes. I think we should merge Sam's (this) one.
chris-eibl 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
Move stats zeroing after check
5c2f809
@colesbury

colesbury commented Dec 11, 2023

Copy link
Copy Markdown
Contributor Author

I fixed the bug pointed out by @chris-eibl. Let me know if you prefer to land the forward references version. If so, I'll merge @nascheme's changes in. Otherwise, I think it's good to go now once the CI passes.
All reactions

Sorry, something went wrong.

Uh oh!

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

pablogsal commented Dec 11, 2023

Copy link
Copy Markdown
Member

I fixed the bug pointed out by @chris-eibl. Let me know if you prefer to land the forward references version. If so, I'll merge @nascheme's changes in. Otherwise, I think it's good to go now once the CI passes.

Nah, is good, let's go with this version. Thanks for the patience!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pablogsal
pablogsal enabled auto-merge (squash) December 11, 2023 18:13
@chris-eibl

chris-eibl commented Dec 11, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

The forward ref version was meant only for review purposes.

Exactly that was my indention :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pablogsal
pablogsal merged commit d70e27f into python:main Dec 11, 2023
@bedevere-app bedevere-app Bot removed the awaiting merge label Dec 11, 2023
@colesbury
colesbury deleted the gc-collecting branch December 11, 2023 19:38
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@colesbury @aisk
pythongh-112529: Use atomic operations for gcstate->collecting (pyt…
190a5f4 
…hon#112533)

* pythongh-112529: Use atomic operations for `gcstate->collecting`

The `collecting` field in `GCState` is used to prevent overlapping garbage
collections within the same interpreter. This is updated to use atomic
operations in order to be thread-safe in `--disable-gil` builds.

The GC code is refactored a bit to support this. More of the logic is pushed
down to `gc_collect_main()` so that we can safely order the logic setting
`collecting`, the selection of the generation, and the invocation of callbacks
with respect to the atomic operations and the (future) stop-the-world pauses.

The change uses atomic operations for both `--disable-gil` and the default
build (with the GIL) to avoid extra `#ifdef` guards and ease the maintenance
burden.
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@colesbury @Glyphack
pythongh-112529: Use atomic operations for gcstate->collecting (pyt…
8ae3ca8 
…hon#112533)

* pythongh-112529: Use atomic operations for `gcstate->collecting`

The `collecting` field in `GCState` is used to prevent overlapping garbage
collections within the same interpreter. This is updated to use atomic
operations in order to be thread-safe in `--disable-gil` builds.

The GC code is refactored a bit to support this. More of the logic is pushed
down to `gc_collect_main()` so that we can safely order the logic setting
`collecting`, the selection of the generation, and the invocation of callbacks
with respect to the atomic operations and the (future) stop-the-world pauses.

The change uses atomic operations for both `--disable-gil` and the default
build (with the GIL) to avoid extra `#ifdef` guards and ease the maintenance
burden.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chris-eibl chris-eibl chris-eibl left review comments
@pablogsal pablogsal pablogsal approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

3.13 only security fixes 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 @pablogsal @nascheme @chris-eibl
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.