Pyrona: Proper LRC decs and incs

Author: xFrednet

Lib/test/test_veronapy_writebarrier.py

@@ -1,4 +1,5 @@
 import unittest
+import dis
 
 class TestCellWriteBarrier(unittest.TestCase):
     class A:
@@ -75,5 +76,30 @@ def test_cell_replace_same_bridge(self):
         # since this writes replaces the previous owning reference
         c.cell_contents = child
 
+class TestBytecodeWriteBarrier(unittest.TestCase):
+    def test_delete_deref(self):
+        r = Region()
+        r.field = {}
+        x = r.field
+
+        # Make sure the region knows about local reference from x
+        self.assertFalse(r.try_close())
+
+        def del_x():
+            nonlocal x
+            del x # This triggers the DELETE_DEREF opcode
+
+        # Create the function and disassemble to confirm DELETE_DEREF is present
+        bytecode = dis.Bytecode(del_x)
+        self.assertIn("DELETE_DEREF", [instr.opname for instr in bytecode])
+
+        self.assertTrue(r.is_open())
+
+        # Call the function forcing the deletion of x which should
+        # close the region.
+        del_x()
+
+        self.assertFalse(r.is_open())
+
 if __name__ == "__main__":
     unittest.main()

Objects/regions.c

@@ -247,6 +247,33 @@ static void regionmetadata_inc_osc(Py_region_ptr_t self_ptr)
 #define regionmetadata_inc_osc(self) \
     (regionmetadata_inc_osc(REGION_PTR_CAST(self)))
 
+/// This function checks the internal state, meaning the LRC, OSC and dirty
+/// status, to determine if the region should be closed. If possible, it will
+/// close the region and propagate the state to any parent region or cowns.
+///
+/// Returns `0` on success. An error might come from closing the region
+/// see `regionmetadata_close` for potential errors.
+static int regionmetadata_check_for_close(regionmetadata* self) {
+    // If the region is marked as dirty, we can't trust the LRC or OSC
+    // to update the status. The region has to be cleaned before it can
+    // be closed
+    if (regionmetadata_is_dirty(self)) {
+        return 0;
+    }
+
+    // The region has to remain open, if there are open subregions
+    if (self->osc != 0) {
+        return 0;
+    }
+
+    // The region has to remain open, if there are references into the region.
+    if (self->osc != 0) {
+        return 0;
+    }
+
+    return regionmetadata_close(self);
+}
+
 /// Decrements the OSC of the region. This might close the region if the LRC
 /// and ORC both hit zero and the region is not marked as dirty.
 ///
@@ -262,11 +289,7 @@ static int regionmetadata_dec_osc(Py_region_ptr_t self_ptr)
     self->osc -= 1;
 
     // Check if the OSC decrease has closed this region as well.
-    if (self->osc == 0 && self->lrc == 0 && !regionmetadata_is_dirty(self)) {
-        return regionmetadata_close(self);
-    }
-
-    return 0;
+    return regionmetadata_check_for_close(self);
 }
 #define regionmetadata_dec_osc(self) \
     (regionmetadata_dec_osc(REGION_PTR_CAST(self)))
@@ -316,6 +339,31 @@ static int regionmetadata_dec_rc(Py_region_ptr_t self_ptr)
 #define regionmetadata_dec_rc(self) \
     (regionmetadata_dec_rc(REGION_PTR_CAST(self)))
 
+static void regionmetadata_inc_lrc(regionmetadata* self)
+{
+    assert(HAS_METADATA(self));
+
+    self->lrc += 1;
+    regionmetadata_open(self);
+}
+#define regionmetadata_inc_lrc(self) \
+    (regionmetadata_inc_lrc(REGION_DATA_CAST(self)))
+
+/// Decrements the LRC of the region. This might close the region if the LRC
+/// and ORC both hit zero and the region is not marked as dirty.
+///
+/// Returns `0` on success. An error might come from closing the region
+/// see `regionmetadata_close` for potential errors.
+static int regionmetadata_dec_lrc(regionmetadata* self)
+{
+    assert(HAS_METADATA(self));
+
+    self->lrc -= 1;
+    return regionmetadata_check_for_close(self);
+}
+#define regionmetadata_dec_lrc(self) \
+    (regionmetadata_dec_lrc(REGION_DATA_CAST(self)))
+
 static void regionmetadata_set_parent(regionmetadata* self, regionmetadata* parent) {
     // Just a sanity check, since these cases should never happen
     assert(HAS_METADATA(self) && "Can't set the parent on the immutable and local region");
@@ -1928,7 +1976,7 @@ bool _Py_RegionAddReference(PyObject *src, PyObject *tgt) {
     if (_Py_IsLocal(src)) {
         // Record the borrowed reference in the LRC of the target region
         // _Py_VPYDBG("Added borrowed ref %p --> %p (owner: '%s')\n", tgt, new_ref, get_region_name(tgt));
-        Py_REGION_DATA(tgt)->lrc += 1;
+        regionmetadata_inc_lrc(Py_REGION_DATA(tgt));
         return true;
     }
 
@@ -1991,9 +2039,8 @@ void _Py_RegionRemoveReference(PyObject *src, PyObject *tgt) {
     regionmetadata* tgt_md = Py_REGION_DATA(tgt);
     if (_Py_IsLocal(src)) {
         // Dec LRC of the previously referenced region
-        // TODO should this decrement be a function, if it hits zero,
-        // then a region could become unreachable.
-        tgt_md->lrc -= 1;
+        // TODO should errors be propagated?
+        regionmetadata_dec_lrc(tgt_md);
         return;
     }