gh-112529: Use atomic operations for `gcstate->collecting` by colesbury · Pull Request #112533 · python/cpython

colesbury · 2023-11-29T18:39:23Z

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

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.

colesbury · 2023-11-29T18:41:22Z

 void
 _Py_ScheduleGC(PyInterpreterState *interp)
 {
- GCState *gcstate = &interp->gc;


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

colesbury · 2023-11-29T18:48:18Z

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

pablogsal · 2023-11-29T23:59:22Z

@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 👍

colesbury · 2023-12-06T15:07:10Z

@pablogsal - gentle reminder, this is awaiting your review

nascheme · 2023-12-08T19:49:51Z

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.

chris-eibl · 2023-12-09T06:59:02Z

 {
- GC_STAT_ADD(generation, collections, 1);
 #ifdef Py_STATS
 if (_Py_stats) {


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

Yes, thanks!

chris-eibl · 2023-12-09T06:59:35Z

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

nascheme · 2023-12-09T21:52:25Z

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.

nascheme · 2023-12-09T22:03:23Z

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.

pablogsal

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

pablogsal · 2023-12-11T17:56:54Z

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.

nascheme · 2023-12-11T18:01:38Z

The forward ref version was meant only for review purposes. I think we should merge Sam's (this) one.

colesbury · 2023-12-11T18:12:06Z

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.

pablogsal · 2023-12-11T18:12:40Z

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!

chris-eibl · 2023-12-11T18:17:22Z

The forward ref version was meant only for review purposes.

Exactly that was my indention :)

…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.

bedevere-appBot mentioned this pull request Nov 29, 2023
Make the garbage collector thread-safe in --disable-gil builds #112529
Closed
3 tasks

colesbury added 3.13 bugs and security fixes topic-free-threading skip news labels Nov 29, 2023

colesbury commented Nov 29, 2023
View reviewed changes

colesbury marked this pull request as ready for review November 29, 2023 18:48

colesbury requested a review from pablogsal as a code owner November 29, 2023 18:48

bedevere-appBot added the awaiting review label Nov 29, 2023

chris-eibl reviewed Dec 9, 2023
View reviewed changes

pablogsal approved these changes Dec 11, 2023
View reviewed changes

bedevere-appBot added awaiting merge and removed awaiting review labels Dec 11, 2023

Move stats zeroing after check
5c2f809

pablogsal enabled auto-merge (squash) December 11, 2023 18:13

pablogsal merged commit d70e27f into python:mainDec 11, 2023

bedevere-appBot removed the awaiting merge label Dec 11, 2023

colesbury deleted the gc-collecting branch December 11, 2023 19:38

Uh oh!

Uh oh!

Conversation

colesbury commented Nov 29, 2023• edited by bedevere-app BotLoading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

colesburyNov 29, 2023

Choose a reason for hiding this comment

Uh oh!

colesbury commented Nov 29, 2023

Uh oh!

pablogsal commented Nov 29, 2023

Uh oh!

colesbury commented Dec 6, 2023

Uh oh!

nascheme commented Dec 8, 2023

Uh oh!

chris-eiblDec 9, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

colesburyDec 11, 2023

Choose a reason for hiding this comment

Uh oh!

chris-eibl commented Dec 9, 2023

Uh oh!

nascheme commented Dec 9, 2023

Uh oh!

nascheme commented Dec 9, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pablogsal left a comment

Choose a reason for hiding this comment

Uh oh!

pablogsal commented Dec 11, 2023

Uh oh!

nascheme commented Dec 11, 2023

Uh oh!

colesbury commented Dec 11, 2023

Uh oh!

pablogsal commented Dec 11, 2023

Uh oh!

chris-eibl commented Dec 11, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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

chris-eiblDec 9, 2023•
edited
Loading

nascheme commented Dec 9, 2023•
edited
Loading

chris-eibl commented Dec 11, 2023•
edited
Loading