Regions: Unfailable RemoveRef, more barriers in dicts, cleanup and better GC support

Include/internal/pycore_cown.h

@@ -26,7 +26,9 @@ typedef uint64_t _PyCown_thread_id_t;
 PyAPI_FUNC(_PyCown_ipid_t) _PyCown_ThisInterpreterId(void);
 PyAPI_FUNC(_PyCown_thread_id_t) _PyCown_ThisThreadId(void);
 PyAPI_FUNC(int) _PyCown_RegionOpen(_PyCownObject *self, _PyRegionObject* region, _PyCown_ipid_t ip);
-PyAPI_FUNC(int) _PyCown_RegionClose(_PyCownObject *self, _PyRegionObject* region, _PyCown_ipid_t ip);
+PyAPI_FUNC(int) _PyCown_AcquireGC(_PyCownObject *self, Py_region_t *region);
+PyAPI_FUNC(int) _PyCown_ReleaseGC(_PyCownObject *self);
+
 
 #ifdef __cplusplus
 }

Include/internal/pycore_ownership.h

@@ -44,6 +44,13 @@ typedef struct _Py_ownership_state {
     //     unfreezable fields or thread local wrappers.
     PyObject *module_locks;
     PyObject *blocking_on;
+#ifdef Py_DEBUG
+    /* The name of the last type that marked all open regions as dirty.
+    *
+    * This is only intended for debugging
+    */
+    PyObject* last_dirty_reason;
+#endif
 #ifdef Py_OWNERSHIP_INVARIANT
     /* Tracks the state of the ownership invariant. Some ownership-related
      * operations may temporarily violate the invariant. To handle this safely,
@@ -79,7 +86,7 @@ typedef struct _Py_ownership_state {
 PyAPI_FUNC(Py_ssize_t) _PyOwnership_get_current_tick(void);
 
 /* Returns the tick which should be used for `region.open_tick` or 0 if the
-* ownerstate is currently unavialble.
+* ownerstate is currently unavailble.
 */
 PyAPI_FUNC(Py_ssize_t) _PyOwnership_get_open_region_tick(void);
 
@@ -88,8 +95,8 @@ PyAPI_FUNC(Py_ssize_t) _PyOwnership_get_open_region_tick(void);
 *
 * It can fail, if the ownership state is currently unavailable
 */
-PyAPI_FUNC(int) _PyOwnership_notify_untrusted_code(void);
-
+PyAPI_FUNC(int) _PyOwnership_notify_untrusted_code(const char* reason);
+PyAPI_FUNC(PyObject*) _PyOwnership_get_last_dirty_region(void);
 
 PyAPI_FUNC(int) _PyOwnership_is_c_wrapper(PyObject *obj);
 

Include/internal/pycore_region.h

@@ -144,6 +144,7 @@ PyAPI_FUNC(int) _PyRegion_ClosesWithLrc(Py_region_t region, Py_ssize_t lrc);
 PyAPI_FUNC(Py_region_t) _PyRegion_GetParent(Py_region_t child);
 PyAPI_FUNC(int) _PyRegion_Clean(Py_region_t region);
 PyAPI_FUNC(void) _PyRegion_MakeDirty(Py_region_t region);
+PyAPI_FUNC(PyObject*) _PyRegion_GetSubregions(Py_region_t region);
 
 PyAPI_FUNC(int) _PyRegion_IsBridge(PyObject *obj);
 PyAPI_FUNC(PyObject*) _PyRegion_GetBridge(Py_region_t region);

Include/internal/pycore_stackref.h

@@ -687,6 +687,11 @@ PyStackRef_MakeHeapSafe(_PyStackRef ref)
         return ref;
     }
     PyObject *obj = BITS_TO_PTR_MASKED(ref);
+
+    // This should always succeed, since we already have a reference on the stack
+    int res = PyRegion_AddLocalRef(obj);
+    assert(res == 0);
+    (void)res;
     Py_INCREF(obj);
     ref.bits = (uintptr_t)obj;
     PyStackRef_CheckValid(ref);

Include/region.h

@@ -31,15 +31,15 @@ PyAPI_FUNC(int) _PyRegion_AddRefs(PyObject *src, int tgt_count, ...);
 #define PyRegion_AddRef(src, tgt) _PyRegion_AddRef(_PyObject_CAST(src), _PyObject_CAST(tgt))
 #define PyRegion_AddRefs(src, ...) _PyRegion_AddRefs(_PyObject_CAST(src), _PyRegion_COUNT_ARGS(__VA_ARGS__), __VA_ARGS__)
 
-PyAPI_FUNC(int) _PyRegion_RemoveRef(PyObject *src, PyObject *tgt);
+PyAPI_FUNC(void) _PyRegion_RemoveRef(PyObject *src, PyObject *tgt);
 #define PyRegion_RemoveRef(src, tgt) _PyRegion_RemoveRef(_PyObject_CAST(src), _PyObject_CAST(tgt))
 
 PyAPI_FUNC(int) _PyRegion_AddLocalRef(PyObject *tgt);
 PyAPI_FUNC(int) _PyRegion_AddLocalRefs(int tgt_count, ...);
 #define PyRegion_AddLocalRef(tgt) _PyRegion_AddLocalRef(_PyObject_CAST(tgt))
 #define PyRegion_AddLocalRefs(...) _PyRegion_AddLocalRefs(_PyRegion_COUNT_ARGS(__VA_ARGS__), __VA_ARGS__)
 
-PyAPI_FUNC(int) _PyRegion_RemoveLocalRef(PyObject *tgt);
+PyAPI_FUNC(void) _PyRegion_RemoveLocalRef(PyObject *tgt);
 #define PyRegion_RemoveLocalRef(tgt) _PyRegion_RemoveLocalRef(_PyObject_CAST(tgt))
 
 static inline PyObject* _PyRegion_NewRef(PyObject* tgt) {
@@ -60,7 +60,7 @@ static inline PyObject* _PyRegion_XNewRef(PyObject* tgt) {
 
 static inline int _PyRegion_TakeRef(PyObject *src, PyObject *tgt) {
     int res = _PyRegion_AddRef(src, tgt);
-    if (res) {
+    if (res != 0) {
         return res;
     }
 
@@ -79,9 +79,12 @@ static inline int _PyRegion_TakeRef(PyObject *src, PyObject *tgt) {
     //      modify the OSC of X but not close X. This ensures that no cown is
     //      released or send off, while we still have remaining references into
     //      X and Y.
-    return _PyRegion_RemoveLocalRef(tgt);
+    _PyRegion_RemoveLocalRef(tgt);
+    return 0;
 }
+PyAPI_FUNC(int) _PyRegion_TakeRefs(PyObject *src, int tgt_count, ...);
 #define PyRegion_TakeRef(src, tgt) _PyRegion_TakeRef(_PyObject_CAST(src), _PyObject_CAST(tgt))
+#define PyRegion_TakeRefs(src, ...) _PyRegion_TakeRefs(_PyObject_CAST(src), _PyRegion_COUNT_ARGS(__VA_ARGS__), __VA_ARGS__)
 
 static inline int _PyRegion_XSetRef(PyObject *src, PyObject **field, PyObject *val) {
     PyObject *old = *field;
@@ -118,6 +121,7 @@ static inline void _PyRegion_Clear(PyObject *src, PyObject **field) {
 }
 #define PyRegion_CLEAR(src, dst) _PyRegion_Clear(_PyObject_CAST(src), (PyObject **)&(dst))
 
+PyAPI_FUNC(void) PyRegion_NotifyTypeUse(PyTypeObject* type);
 
 #ifdef __cplusplus
 }

Lib/test/test_regions/test_clean.py

@@ -1,23 +1,42 @@
 import unittest
 from regions import Region, is_local
-import sys
-
 
 class TestCleanRegion(unittest.TestCase):
     def mark_region_as_dirty(self, region: Region):
         region._make_dirty()
 
         self.assertTrue(region.is_dirty, "Region should be dirty here")
 
-    def test_try_close_dirty_with_local_ref(self):
+    def test_clean_marks_region_as_clean(self):
         region = Region()
 
         self.mark_region_as_dirty(region)
-        region.clean()
+        cleaned = region.clean()
 
         self.assertFalse(region.is_dirty)
+        self.assertEqual(cleaned, 1)
+
+    def test_clean_ignores_clean_subregions(self):
+        region = Region()
+        self.mark_region_as_dirty(region)
+        region.sub = Region()
+        detached_object = {}
+        region.sub.obj = detached_object
+        region.sub.obj = None
 
-    def test_try_close_sub_region(self):
+        # Precondition
+        self.assertFalse(region.sub.is_dirty, "The subregion should be clean")
+        self.assertTrue(region.sub.owns(detached_object))
+
+        # Action - Only dirty regions should be effected by this clean call
+        cleaned = region.clean()
+
+        # Postcondition
+        self.assertEqual(cleaned, 1)
+        self.assertFalse(region.is_dirty, "The parent region should be cleaned")
+        self.assertTrue(region.sub.owns(detached_object), "The subregion should remain uncleaned")
+
+    def test_cleaning_also_cleans_dirty_subregion(self):
         region = Region()
         region.sub = Region()
         self.mark_region_as_dirty(region)
@@ -26,18 +45,40 @@ def test_try_close_sub_region(self):
         sub = region.sub
 
         # Cleaning a dirty parent region should clean the child as well
-        region.clean()
+        cleaned = region.clean()
 
-        # The regions should now be clean
+        # The regions should now be clean and the LRC should be correct
+        self.assertEqual(cleaned, 2)
         self.assertFalse(sub.is_dirty)
-        self.assertEqual(sub._lrc, 1, "The local should be the only known reference")
+        self.assertEqual(sub._lrc, 1, "The sub region should only have an LRC of 1")
         self.assertEqual(sub._osc, 0, "No subregions should be present")
         self.assertEqual(region._osc, 1)
 
         # Removing the reference into sub, should close it and inform the parent
         sub = None
         self.assertEqual(region._osc, 0)
 
+    def test_cleaning_finds_dirty_subregion(self):
+        region = Region()
+        region.sub = Region()
+        region.sub.sub = Region()
+        detached_object = {}
+        region.sub.sub.obj = detached_object
+        region.sub.sub.obj = None
+        self.mark_region_as_dirty(region.sub.sub)
+
+        # Precondition
+        self.assertFalse(region.is_dirty, "The region should be clean")
+        self.assertFalse(region.sub.is_dirty, "The subregion should be clean")
+        self.assertTrue(region.sub.sub.owns(detached_object))
+
+        # Action: Clean should find the dirty subsubregion
+        cleaned = region.clean()
+
+        # Postcondition
+        self.assertEqual(cleaned, 1)
+        self.assertTrue(is_local(detached_object))
+        self.assertFalse(region.sub.sub.is_dirty, "The subsubregion should be clean")
 
     def test_clean_removes_unreachable(self):
         region = Region()

Lib/test/test_regions/test_core.py

@@ -15,15 +15,15 @@ def test_region_construction(self):
         # The region should be open since r points into it
         self.assertTrue(r.is_open)
 
-        # FIXME(regions): xFrednet: Regions currently default to being dirty
-        # while most write barriers are missing
-        #
         # A new region should be clean
-        # self.assertFalse(r.is_dirty)
+        self.assertFalse(r.is_dirty)
 
         # A new region has no parent
         self.assertIsNone(r.parent)
 
+        # A new region should have not subregions
+        self.assertListEqual(r._subregions, [])
+
     def test_fields_read_only(self):
         r = Region()
 
@@ -37,6 +37,15 @@ def test_fields_read_only(self):
         with self.assertRaises(AttributeError):
             r.parent = None
 
+        with self.assertRaises(AttributeError):
+            r._lrc = None
+
+        with self.assertRaises(AttributeError):
+            r._osc = None
+
+        with self.assertRaises(AttributeError):
+            r._subregions = None
+
     def test_instance_dict_is_owned(self):
         r = Region()
 
@@ -202,3 +211,56 @@ def test_get_parent(self):
 
         # Check that r2 has no parent
         self.assertIsNone(r2.parent)
+
+    def test_subregions(self):
+        r1 = Region()
+        r2 = Region()
+        r3 = Region()
+        r4 = Region()
+
+        # r1 starts with no children
+        self.assertEqual(len(r1._subregions), 0)
+
+        # This should add r2 as a subregion
+        r1.r2 = r2
+        self.assertEqual(len(r1._subregions), 1)
+        self.assertIn(r2, r1._subregions)
+
+        # This should add r3 as a subregion
+        r1.r3 = r3
+        self.assertEqual(len(r1._subregions), 2)
+        self.assertIn(r2, r1._subregions)
+        self.assertIn(r3, r1._subregions)
+
+        # This should replace r3 as a subregion
+        r1.r3 = r4
+        self.assertEqual(len(r1._subregions), 2)
+        self.assertIn(r2, r1._subregions)
+        self.assertIn(r4, r1._subregions)
+        self.assertNotIn(r3, r1._subregions)
+
+        # The subregions list should be temporary and clear the LRC after
+        self.assertEqual(r2._lrc, 1)
+        self.assertEqual(r3._lrc, 1)
+        self.assertEqual(r4._lrc, 1)
+
+    def test_region_dissolve_bumps_subregion_lrc(self):
+        r1 = Region()
+        r2 = Region()
+        obj = self.A()
+
+        # Make r2 a subregion of 1
+        r1.obj = obj
+        obj.r2 = r2
+
+        # Precondition
+        self.assertEqual(r2.parent, r1)
+        r2_lrc = r2._lrc
+
+        # Dissolve parent region
+        r1 = None
+
+        # Postcondition
+        self.assertEqual(r2._lrc, r2_lrc + 1)
+        self.assertIsNone(r2.parent)
+        self.assertTrue(is_local(obj))