Region GC by kulisak12 · Pull Request #105 · fxpl/cpython

kulisak12 · 2026-06-12T09:35:50Z

Please check the last commit in particular, I still don't understand the write barrier enough 😅

xFrednet · 2026-06-13T10:18:59Z


+/* Create artificial local references to the bridges. */
+static int
+region_list_add_local_refs(_PyRegionObject *root) {


This looks correct. Where do you remove this RC and LRC that you've added here? I can't see it in this commit

In gc_collect_region_tree. It is not a part of this commit.
https://github.com/kulisak12/cpython/blob/22db1e57fda903be66e75d88c6ba57c7073a5e64/Python/gc.c#L2123

xFrednet · 2026-06-13T10:22:14Z

For now, I only checked the last commit (based) on the description. I plan to look at the rest later.

The fact that this PR is this small is super impressive and a testament for how well you understood the whole thing and wired it up! I'm looking forward to reviewing it :D

xFrednet

Excellent, the code is super readable and concise. I'm pretty confident that I can modify it or that we can give this to someone else to test different scheduling mechanics. Very well done, thank you for all the work you put into this!

I have some comments, but mainly some clarifying questions. I didn't find any logic bugs :D

xFrednet · 2026-06-22T12:12:05Z

+    cown_set_value_unchecked(self, Py_None);
+    // Should be able to release now.
+    int res = cown_release(self, this_ip);
+    assert(res == 0);


This produces a warning on non debug builds. It can be suppressed by adding (void)res in addition to the assert:

Suggested change

assert(res == 0);

assert(res == 0);

(void)res;

xFrednet · 2026-06-22T12:16:28Z

+    PyThreadState *tstate = PyThreadState_Get();
+    if (!_PyGC_CanRunRegionGC(tstate)) {


The docs of PyThreadState_Get(); state that it can return NULL. But this code doesn't null check. Does it expect _PyGC_CanRunRegionGC to do the NULL check? If so, can you add a comment about this?

xFrednet · 2026-06-22T12:24:17Z

+
+    // Lock without blocking, we only want the cown if it is released.
+    int res = cown_lock(self, NO_BLOCKING_TIMEOUT, this_ip, true);
+    if (res != COWN_ACQUIRE_SUCCESS) {


Shouldn't this check for COWN_ACQUIRE_ERROR and in that case surface the error to the caller? This is being called from CownObject_release which can still throw an exception.

If you want to explicitly ignore errors, can you add a comment about why this is safe?

xFrednet · 2026-06-22T12:59:10Z

+                // Regions: No barrier needed.
+                // If called from the local GC, op is local.
+                // If called from the region GC, the region is already open.
                Py_INCREF(op);
                // TODO(Immutable): This is only required until we have the SCC support working.
                _Py_CLEAR_IMMUTABLE(op);
                (void) clear(op);
                if (_PyErr_Occurred(tstate)) {
                    PyErr_FormatUnraisable("Exception ignored in tp_clear of %s",
                                           Py_TYPE(op)->tp_name);
                }
                Py_DECREF(op);


I'm considering if this is correct. If the clear code or a deallocator triggered by the clear code, triggers a region clean on the owner, it will include this RC in the LRC. The DECREF in line 1240 would only dec the RC and not the LRC.

So, I think this needs a write barrier to be safe. (I believe we talked about this previously, but I didn't think about this case until now.)

The barrier would look something like this:

+ // Regions: This barrier should always succeeds since: // If called from the local GC, op is local. // If called from the region GC, the region is already open. + int res = PyRegion_AddLocalRef(op); + assert(res == 0) + (void)res; Py_INCREF(op); // TODO(Immutable): This is only required until we have the SCC support working. _Py_CLEAR_IMMUTABLE(op); (void) clear(op); if (_PyErr_Occurred(tstate)) { PyErr_FormatUnraisable("Exception ignored in tp_clear of %s", Py_TYPE(op)->tp_name); } + PyRegion_RemoveLocalRef(op;) Py_DECREF(op);

xFrednet · 2026-06-23T06:58:30Z

+    /* Separate child regions from contained objects.
+     * Finalizers need them to be in the GC list, at the start of the list.
+     */
+    PyGC_Head contained;
+    gc_list_init(&contained);
+    gc_region_list_split(&data->gc_list, &contained);
+


With all of these additional splits and joins I see why you asked if we can split the contained object and child-region lists. For now, I want to keep this structure since it works, but this should definitely be on the "benchmark this" list for when the implementation is complete-ish.

The implementation thus far is super readable, so this change should be easy to in cooperate into your implementation.

xFrednet · 2026-06-23T08:08:04Z

+     */
+    gcstate->region_budget += 3;
+    if (gcstate->region_budget > gcstate->young.threshold) {
+        gcstate->region_budget = gcstate->young.threshold;


I assume this is one of the places we can modify to adjust the scheduling?

For a steady state heap, the amount of work to do is three times

Where does the "three times" come from, is this a different comment in this file or some benchmarking?

xFrednet · 2026-06-23T08:09:21Z

+        // TODO(regions): If callback moves op into a region,
+        // this function would start iterating objects in the region.


I assume this TODO is if op is local?

We can later mark the object as unmovable, when we support this on a per-instance level :)

xFrednet · 2026-06-23T08:10:22Z

+    def setUp(self):
+        # Need to run collection multiple times to clean up region chains
+        while gc.collect() > 0:
+            pass


I love the fact that you need this setup function :D

xFrednet · 2026-06-23T08:12:29Z

+    def tearDown(self):
+        gc.disable()


This confused me a bit, could you add a comment that the GC is enabled again once all tests in the module are done by tearDownModule

xFrednet · 2026-06-23T08:13:14Z

-    def test_owned_cycles_are_ignored(self):
+    def build_region_with_unreachable_cycle(self):


Happy to see that my old broken test got replaced :D

kulisak12 added 14 commits May 7, 2026 19:36

GC: Unify documentation of GC flags

9a3fe67

GC: Remove outdated comment

dd31edc

GC: Rename functions to not clash with regions

fc4f3db

Regions GC: Add gc.collect_region

2a94c85

Regions GC: Implement basic GC for regions

a8bbe0e

Regions GC: Unit tests

620c21c

Regions GC: Cown switching

0bfb472

Regions GC: Accept an acquired cown or bridge

b646fe2

Regions GC: Deal with cleaning

b1ef3c7

Regions GC: Avoid recursion and traversal problems

4750a2b

Regions GC: GIL release

7f1fdab

Cowns: Simplify GC functions

a430c2c

Regions GC: Scheduling on release

80fe748

Regions GC: Increase budget when moving an object

04243bb

xFrednet reviewed Jun 13, 2026

View reviewed changes

Regions GC: Write barriers

22db1e5

kulisak12 force-pushed the regions-gc branch from 3c47833 to 22db1e5 Compare June 13, 2026 10:59

xFrednet reviewed Jun 23, 2026

View reviewed changes

		PyThreadState *tstate = PyThreadState_Get();
		if (!_PyGC_CanRunRegionGC(tstate)) {

		// TODO(regions): If callback moves op into a region,
		// this function would start iterating objects in the region.

		def test_owned_cycles_are_ignored(self):
		def build_region_with_unreachable_cycle(self):

Conversation

kulisak12 commented Jun 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xFrednet commented Jun 13, 2026

Uh oh!

xFrednet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants