Add WriteBarrier to dictobject

Include/internal/pycore_regions.h

@@ -57,15 +57,18 @@ PyObject* _Py_ResetInvariant(void);
 #define Py_ResetInvariant() _Py_ResetInvariant()
 
 // Invariant placeholder
-bool _Pyrona_AddReference(PyObject* target, PyObject* new_ref);
-#define Pyrona_ADDREFERENCE(a, b) _Pyrona_AddReference(a, b)
-#define Pyrona_REMOVEREFERENCE(a, b) // TODO
+bool _Py_RegionAddReference(PyObject* src, PyObject* new_tgt);
+#define Py_REGIONADDREFERENCE(a, b) _Py_RegionAddReference(_PyObject_CAST(a), b)
+
 // Helper macros to count the number of arguments
 #define _COUNT_ARGS(_1, _2, _3, _4, _5, _6, _7, _8, _9, _10, _11, _12, _13, _14, _15, _16, N, ...) N
 #define COUNT_ARGS(...) _COUNT_ARGS(__VA_ARGS__, 16, 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1)
 
-bool _Pyrona_AddReferences(PyObject* target, int new_refc, ...);
-#define Pyrona_ADDREFERENCES(a, ...) _Pyrona_AddReferences(a, COUNT_ARGS(__VA_ARGS__), __VA_ARGS__)
+bool _Py_RegionAddReferences(PyObject* src, int new_tgtc, ...);
+#define Py_REGIONADDREFERENCES(a, ...) _Py_RegionAddReferences(_PyObject_CAST(a), COUNT_ARGS(__VA_ARGS__), __VA_ARGS__)
+
+void _Py_RegionRemoveReference(PyObject* src, PyObject* old_tgt);
+#define Py_REGIONREMOVEREFERENCE(a, b) _Py_RegionRemoveReference(_PyObject_CAST(a), b)
 
 #ifdef NDEBUG
 #define _Py_VPYDBG(fmt, ...)

Lib/test/test_regions_dictobject.py

@@ -0,0 +1,50 @@
+import unittest
+
+class TestRegionsDictObject(unittest.TestCase):
+    def setUp(self):
+        enableinvariant()
+
+    def test_dict_insert_empty_dict(self):
+        # Create Region with Empty dictionary
+        r = Region()
+        d = {}
+        r.add_object(d)
+        n = {}
+        # Add local object to region
+        d["foo"] = n
+        self.assertTrue(r.owns_object(n))
+
+    def test_dict_insert_nonempty_dict(self):
+        # Create Region with Nonempty dictionary
+        r = Region()
+        d = {}
+        d["bar"] = 1
+        r.add_object(d)
+        # Add local object to region
+        n = {}
+        d["foo"] = n
+        self.assertTrue(r.owns_object(n))
+
+    def test_dict_update_dict(self):
+        # Create Region with Nonempty dictionary
+        r = Region()
+        d = {}
+        n1 = {}
+        d["foo"] = n1
+        r.add_object(d)
+        # Update dictionary to contain a local object
+        n2 = {}
+        d["foo"] = n2
+        self.assertTrue(r.owns_object(n2))
+
+    def test_dict_clear(self):
+        # Create Region with Nonempty dictionary
+        r = Region()
+        d = {}
+        n = {}
+        d["foo"] = n
+        r.add_object(d)
+        # Clear dictionary
+        d.clear()
+        # As LRC is not checked by the invariant, this test cannot
+        # check anything useful yet.

Objects/dictobject.c

@@ -122,6 +122,7 @@ As a consequence of this, split keys have a maximum size of 16.
 #include "pycore_regions.h"        // _PyObject_GC_TRACK()
 #include "pycore_pyerrors.h"      // _PyErr_GetRaisedException()
 #include "pycore_pystate.h"       // _PyThreadState_GET()
+#include "pycore_regions.h"       // Py_ADDREGIONREFERENCE(), ... (region)
 #include "stringlib/eq.h"         // unicode_eq()
 #include "regions.h"              // Py_IsImmutable()
 
@@ -666,6 +667,10 @@ new_keys_object(PyInterpreterState *interp, uint8_t log2_size, bool unicode)
 static void
 free_keys_object(PyInterpreterState *interp, PyDictKeysObject *keys)
 {
+    // TODO: This feels like it should remove the references in the regions
+    // but keys is not a Python object, so it's not clear how to do that.
+    // mjp: Leaving as a TODO for now.
+
     assert(keys != Py_EMPTY_KEYS);
     if (DK_IS_UNICODE(keys)) {
         PyDictUnicodeEntry *entries = DK_UNICODE_ENTRIES(keys);
@@ -830,6 +835,9 @@ clone_combined_dict_keys(PyDictObject *orig)
         if (value != NULL) {
             Py_INCREF(value);
             Py_INCREF(*pkey);
+            // TODO need to add_reference here.  We don't know the underlying region
+            // but it should be freshly allocated, so can we consider it the local region
+            // TODO discuss before merging PR.
         }
         pvalue += offs;
         pkey += offs;
@@ -1258,6 +1266,9 @@ insertdict(PyInterpreterState *interp, PyDictObject *mp,
     MAINTAIN_TRACKING(mp, key, value);
 
     if (ix == DKIX_EMPTY) {
+        if (!Py_REGIONADDREFERENCES((PyObject*)mp, key, value)) {
+            goto Fail;
+        }
         uint64_t new_version = _PyDict_NotifyEvent(
                 interp, PyDict_EVENT_ADDED, mp, key, value);
         /* Insert into new slot. */
@@ -1303,6 +1314,9 @@ insertdict(PyInterpreterState *interp, PyDictObject *mp,
     }
 
     if (old_value != value) {
+        if (!Py_REGIONADDREFERENCE((PyObject*)mp, value)) {
+            goto Fail;
+        }
         if(DK_IS_UNICODE(mp->ma_keys)){
             PyDictUnicodeEntry *ep;
             ep = &DK_UNICODE_ENTRIES(mp->ma_keys)[ix];
@@ -1336,6 +1350,7 @@ insertdict(PyInterpreterState *interp, PyDictObject *mp,
             else {
                 _PyDictEntry_SetValue(DK_ENTRIES(mp->ma_keys) + ix, value);
             }
+            Py_REGIONREMOVEREFERENCE((PyObject*)mp, old_value);
         }
         mp->ma_version_tag = new_version;
     }
@@ -1365,6 +1380,9 @@ insert_to_emptydict(PyInterpreterState *interp, PyDictObject *mp,
         return -1;
     }
 
+    if (!Py_REGIONADDREFERENCES((PyObject*)mp, key, value))
+        return -1;
+
     uint64_t new_version = _PyDict_NotifyEvent(
             interp, PyDict_EVENT_ADDED, mp, key, value);
 
@@ -2015,6 +2033,7 @@ delitem_common(PyDictObject *mp, Py_hash_t hash, Py_ssize_t ix,
         assert(ix < SHARED_KEYS_MAX_SIZE);
         /* Update order */
         delete_index_from_values(mp->ma_values, ix);
+        Py_REGIONREMOVEREFERENCE(mp, old_value);
         ASSERT_CONSISTENT(mp);
     }
     else {
@@ -2181,7 +2200,13 @@ PyDict_Clear(PyObject *op)
     if (oldvalues != NULL) {
         n = oldkeys->dk_nentries;
         for (i = 0; i < n; i++)
+        {
+            PyObject* old = oldvalues->values[i];
+            // TODO Should we build a combined macro for this
+            //    Py_CLEAR_WITH_REGION(op, oldvalues->values[i])
+            Py_REGIONREMOVEREFERENCE(op, old);
             Py_CLEAR(oldvalues->values[i]);
+        }
         free_values(oldvalues);
         dictkeys_decref(interp, oldkeys);
     }
@@ -2459,6 +2484,8 @@ dict_dealloc(PyDictObject *mp)
     Py_TRASHCAN_BEGIN(mp, dict_dealloc)
     if (values != NULL) {
         for (i = 0, n = mp->ma_keys->dk_nentries; i < n; i++) {
+            PyObject *value = values->values[i];
+            Py_REGIONREMOVEREFERENCE(mp, value);
             Py_XDECREF(values->values[i]);
         }
         free_values(values);
@@ -3142,6 +3169,13 @@ PyDict_Copy(PyObject *o)
         dictkeys_incref(mp->ma_keys);
         for (size_t i = 0; i < size; i++) {
             PyObject *value = mp->ma_values->values[i];
+            if (!Py_REGIONADDREFERENCE(split_copy, value))
+            {
+                // TODO: is this safe to dealloc the split_copy?
+                // is it in a valid enough state to be deallocated?
+                Py_DECREF(split_copy);
+                return NULL;
+            }
             split_copy->ma_values->values[i] = Py_XNewRef(value);
         }
         if (_PyObject_GC_IS_TRACKED(mp))
@@ -3446,6 +3480,8 @@ PyDict_SetDefault(PyObject *d, PyObject *key, PyObject *defaultobj)
                 Py_ssize_t index = (int)mp->ma_keys->dk_nentries;
                 assert(index < SHARED_KEYS_MAX_SIZE);
                 assert(mp->ma_values->values[index] == NULL);
+                if (!Py_REGIONADDREFERENCE(mp, value))
+                  return NULL;
                 mp->ma_values->values[index] = Py_NewRef(value);
                 _PyDictValues_AddToInsertionOrder(mp->ma_values, index);
             }
@@ -3467,6 +3503,8 @@ PyDict_SetDefault(PyObject *d, PyObject *key, PyObject *defaultobj)
         assert(mp->ma_keys->dk_usable >= 0);
     }
     else if (value == NULL) {
+        if (!Py_REGIONADDREFERENCE(mp, value))
+          return NULL;
         uint64_t new_version = _PyDict_NotifyEvent(
                 interp, PyDict_EVENT_ADDED, mp, key, defaultobj);
         value = defaultobj;
@@ -5507,7 +5545,7 @@ _PyObject_InitializeDict(PyObject *obj)
         return -1;
     }
     PyObject **dictptr = _PyObject_ComputedDictPointer(obj);
-    Pyrona_ADDREFERENCE(obj, dict);
+    Py_REGIONADDREFERENCE(obj, dict);
     *dictptr = dict;
     return 0;
 }
@@ -5553,11 +5591,20 @@ _PyObject_StoreInstanceAttribute(PyObject *obj, PyDictValues *values,
     assert(values != NULL);
     assert(Py_TYPE(obj)->tp_flags & Py_TPFLAGS_MANAGED_DICT);
 
-    if(!Py_CHECKWRITE(obj)){
+    if (!Py_CHECKWRITE(obj)){
         PyErr_WriteToImmutable(obj);
         return -1;
     }
 
+    //TODO: PYRONA: The addition of the key is complex here.
+    //  The keys PyDictKeysObject, might already have the key. Note that
+    //  the keys PyDictKeysObject is not a PyObject. So it is unclear where
+    //  this edge is created.
+    //  The keys is coming from ht_cached_keys on the type object.
+    //  This is also interesting from a race condition perspective.
+    //  Can this be shared, should it be treated immutably when the type is?
+    // mjp: Leaving for a future PR.
+
     Py_ssize_t ix = DKIX_EMPTY;
     if (PyUnicode_CheckExact(name)) {
         ix = insert_into_dictkeys(keys, name);
@@ -5589,6 +5636,9 @@ _PyObject_StoreInstanceAttribute(PyObject *obj, PyDictValues *values,
             return PyDict_SetItem(dict, name, value);
         }
     }
+    if (!Py_REGIONADDREFERENCE(obj, value)) {
+        return -1;
+    }
     PyObject *old_value = values->values[ix];
     values->values[ix] = Py_XNewRef(value);
     if (old_value == NULL) {
@@ -5604,6 +5654,7 @@ _PyObject_StoreInstanceAttribute(PyObject *obj, PyDictValues *values,
         if (value == NULL) {
             delete_index_from_values(values, ix);
         }
+        Py_REGIONREMOVEREFERENCE(obj, old_value);
         Py_DECREF(old_value);
     }
     return 0;
@@ -5775,7 +5826,7 @@ PyObject_GenericGetDict(PyObject *obj, void *context)
                     _Py_SetImmutable(dict);
                 }
                 else {
-                    Pyrona_ADDREFERENCE(obj, dict);
+                    Py_REGIONADDREFERENCE(obj, dict);
                     dorv_ptr->dict = dict;
                 }
             }
@@ -5789,7 +5840,7 @@ PyObject_GenericGetDict(PyObject *obj, void *context)
                     _Py_SetImmutable(dict);
                 }
                 else {
-                    Pyrona_ADDREFERENCE(obj, dict);
+                    Py_REGIONADDREFERENCE(obj, dict);
                     dorv_ptr->dict = dict;
                 }
             }
@@ -5817,7 +5868,7 @@ PyObject_GenericGetDict(PyObject *obj, void *context)
                 _Py_SetImmutable(dict);
             }
             else {
-                Pyrona_ADDREFERENCE(obj, dict);
+                Py_REGIONADDREFERENCE(obj, dict);
                 *dictptr = dict;
             }
         }
@@ -5841,7 +5892,7 @@ _PyObjectDict_SetItem(PyTypeObject *tp, PyObject **dictptr,
         if (dict == NULL) {
             dictkeys_incref(cached);
             dict = new_dict_with_shared_keys(interp, cached);
-            Pyrona_ADDREFERENCE(owner, dict);
+            Py_REGIONADDREFERENCE(owner, dict);
             if (dict == NULL)
                 return -1;
             *dictptr = dict;
@@ -5850,7 +5901,7 @@ _PyObjectDict_SetItem(PyTypeObject *tp, PyObject **dictptr,
             res = PyDict_DelItem(dict, key);
         }
         else {
-            if (Pyrona_ADDREFERENCES(dict, key, value)) {
+            if (Py_REGIONADDREFERENCES(dict, key, value)) {
                 res = PyDict_SetItem(dict, key, value);
             } else {
                 // Error is set inside ADDREFERENCE
@@ -5863,19 +5914,13 @@ _PyObjectDict_SetItem(PyTypeObject *tp, PyObject **dictptr,
             dict = PyDict_New();
             if (dict == NULL)
                 return -1;
-            Pyrona_ADDREFERENCE(owner, dict);
+            Py_REGIONADDREFERENCE(owner, dict);
             *dictptr = dict;
         }
         if (value == NULL) {
             res = PyDict_DelItem(dict, key);
         } else {
-            // TODO: remove this once we merge Matt P's changeset to dictionary object
-            if (Pyrona_ADDREFERENCES(dict, key, value)) {
-                res = PyDict_SetItem(dict, key, value);
-            } else {
-                // Error is set inside ADDREFERENCE
-                return -1;
-            }
+            res = PyDict_SetItem(dict, key, value);
         }
     }
     ASSERT_CONSISTENT(dict);

Objects/object.c

@@ -1267,7 +1267,7 @@ _PyObject_GetDictPtr(PyObject *obj)
             PyErr_Clear();
             return NULL;
         }
-        Pyrona_ADDREFERENCE(obj, dict);
+        Py_REGIONADDREFERENCE(obj, dict);
         dorv_ptr->dict = dict;
     }
     return &dorv_ptr->dict;
@@ -1467,7 +1467,7 @@ _PyObject_GenericGetAttrWithDict(PyObject *obj, PyObject *name,
                         res = NULL;
                         goto done;
                     }
-                    Pyrona_ADDREFERENCE(obj, dict);
+                    Py_REGIONADDREFERENCE(obj, dict);
                     dorv_ptr->dict = dict;
                 }
             }