gh-112529: Use atomic operations for `gcstate->collecting` #112533

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

Modules/gcmodule.c

@@ -2271,10 +2262,6 @@ PyObject_IS_GC(PyObject *obj)
 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

Modules/gcmodule.c

 {
-    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-app bot 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-app bot 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-app bot 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:main Dec 11, 2023
30 checks passed

bedevere-app bot removed the awaiting merge label Dec 11, 2023

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-112529: Use atomic operations for `gcstate->collecting` #112533

gh-112529: Use atomic operations for `gcstate->collecting` #112533

colesbury commented Nov 29, 2023 •

edited by bedevere-app bot

Loading

colesbury Nov 29, 2023

colesbury commented Nov 29, 2023

pablogsal commented Nov 29, 2023

colesbury commented Dec 6, 2023

nascheme commented Dec 8, 2023

chris-eibl Dec 9, 2023 •

edited

Loading

colesbury Dec 11, 2023

chris-eibl commented Dec 9, 2023

nascheme commented Dec 9, 2023

nascheme commented Dec 9, 2023 •

edited

Loading

pablogsal left a comment

pablogsal commented Dec 11, 2023

nascheme commented Dec 11, 2023

colesbury commented Dec 11, 2023

pablogsal commented Dec 11, 2023

chris-eibl commented Dec 11, 2023 •

edited

Loading

gh-112529: Use atomic operations for gcstate->collecting #112533

gh-112529: Use atomic operations for gcstate->collecting #112533

Conversation

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

colesbury Nov 29, 2023

Choose a reason for hiding this comment

colesbury commented Nov 29, 2023

pablogsal commented Nov 29, 2023

colesbury commented Dec 6, 2023

nascheme commented Dec 8, 2023

chris-eibl Dec 9, 2023 • edited Loading

Choose a reason for hiding this comment

colesbury Dec 11, 2023

Choose a reason for hiding this comment

chris-eibl commented Dec 9, 2023

nascheme commented Dec 9, 2023

nascheme commented Dec 9, 2023 • edited Loading

pablogsal left a comment

Choose a reason for hiding this comment

pablogsal commented Dec 11, 2023

nascheme commented Dec 11, 2023

colesbury commented Dec 11, 2023

pablogsal commented Dec 11, 2023

chris-eibl commented Dec 11, 2023 • edited Loading

gh-112529: Use atomic operations for `gcstate->collecting` #112533

gh-112529: Use atomic operations for `gcstate->collecting` #112533

colesbury commented Nov 29, 2023 •

edited by bedevere-app bot

Loading

chris-eibl Dec 9, 2023 •

edited

Loading

nascheme commented Dec 9, 2023 •

edited

Loading

chris-eibl commented Dec 11, 2023 •

edited

Loading