Add WriteBarrier to dictobject #34
Conversation
This implements the addreference and remove reference logic for the dictionary.
xFrednet
left a comment
xFrednet
left a comment
There was a problem hiding this comment.
Two hight level comments
|
@mjp41 Is this PR still draft or is that a stale label? |
Yes. There is a test failing I fixed locally, but need to tidy the change. |
xFrednet
left a comment
xFrednet
left a comment
There was a problem hiding this comment.
Overall LGTM, mostly superficial comments and questions
|
Is it okay to merge this? |
TobiasWrigstad
left a comment
TobiasWrigstad
left a comment
There was a problem hiding this comment.
Great work @mjp41 !
| 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); |
There was a problem hiding this comment.
Thanks for the renaming! 🙏
| uint64_t new_version = _PyDict_NotifyEvent( | ||
| interp, PyDict_EVENT_CLONED, mp, b, NULL); | ||
| PyDictKeysObject *keys = clone_combined_dict_keys(other); | ||
| PyDictKeysObject *keys = clone_combined_dict_keys(other, a); // Need to say what owns the keys? |
There was a problem hiding this comment.
The ? at the end of this comment suggests we need a FIXME or TODO label added!
There was a problem hiding this comment.
This one the comment is stale, and needs removing. a owns the keys.
| // 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. |
There was a problem hiding this comment.
| // The keys is coming from ht_cached_keys on the type object. | |
| // The key is coming from ht_cached_keys on the type object. |
There was a problem hiding this comment.
I actually meant the plural, as it is the "keys object/structure". But I can see why it is confusing and the addition of object/structure would make it clearer.
| return -1; | ||
| } | ||
|
|
||
| //TODO: PYRONA: The addition of the key is complex here. |
There was a problem hiding this comment.
Good with the labelling. We should go through all our phase3 code and make sure our TODOs and FIXMEs are all labelled thus.
|
Yes, please go ahead! |
This implements the addreference and remove reference logic for the dictionary.