gh-112529: Use atomic operations for gcstate->collecting - #112533
Conversation
Sorry, something went wrong.
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.
| void | ||
| _Py_ScheduleGC(PyInterpreterState *interp) | ||
| { | ||
| GCState *gcstate = &interp->gc; |
There was a problem hiding this comment.
fyi: _Py_ScheduleGC not public and only called from _PyObject_GC_Link, which already checks gcstate->collecting.
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
will review this week 👍 |
Sorry, something went wrong.
|
@pablogsal - gentle reminder, this is awaiting your review |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
| { | ||
| GC_STAT_ADD(generation, collections, 1); | ||
| #ifdef Py_STATS | ||
| if (_Py_stats) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes, thanks!
Sorry, something went wrong.
|
I think using forward declarations might help to have less moved blocks and make the review easier? |
Sorry, something went wrong.
Probably it would but I'd rather the code is cleaner (without the forward decs) and have the patch be messier. |
Sorry, something went wrong.
|
I rewrote the commit to use forward declarations (as suggested by Chris), makes the review easier: Aside from re-ordering the code and addition the forward defs, I didn't change anything. |
Sorry, something went wrong.
pablogsal
left a comment
There was a problem hiding this comment.
I finally had time to review this. Apologies for the delay.
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
The forward ref version was meant only for review purposes. I think we should merge Sam's (this) one. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
Nah, is good, let's go with this version. Thanks for the patience! |
Sorry, something went wrong.
Exactly that was my indention :) |
Sorry, something went wrong.
…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.
…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.
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.