Phase 3: Adding WB to object.c #26

TobiasWrigstad · 2024-12-14T23:57:18Z

Line 1176: if(Py_CHECKWRITE(v)){
Line 1193: if(Py_CHECKWRITE(v)){
Line 1639: if(!Py_CHECKWRITE(obj)){

The first two cases are in PyObject_SetAttr.

I believe that PyObject_SetAttr falls out for free with the default tp_setattro implementation which simply calls PyObjectGenericSetAttr which in turn always ends up in dictobject.c -- unless we hit line 1567 which uses tp_descr_set, overloading of __set__.

However, __set__ descriptors' behaviour should fall out for free as they will eventually hit a "normal" store which should be trapped by the write barrier.

The normal tp_setattro implementation is changed by __setattr__ being overridden by user. We should expect things to fall out for free in this case since that code will eventually hit a "normal" store.

Exceptions: unless the implementation of __setattr__ or __set__ is written in C. This is handled using the Py_TPFLAGS_REGION_AWARE from PR #33. If this flag is not set, a dirty flag is set in the object.

The third and last case is a call to PyObject_GenericSetDict which is the setter for o.__dict__ = v. In this case, we need an add ref on the v and a remove ref on the old value of o.__dict__. This has been implemented. However, I have so far been unable to trigger this code. o.__dict__ = v runs subtype_setdict which I have extended in a similar fashion.

TobiasWrigstad · 2024-12-19T14:20:55Z

"How to handle this case" has two parts:

What to do when the set attr function is an "arbitrary C function" outside of the CPython standard libraries — the answer is "set the LRC dirty flag"
How do we distinguish between OK C functions (such as those in the CPython standard libraries) and NOT OK C functions — here the answer is possibly check if the type is Pyrona-aware.

If the answer to 2. is Pyrona-awareness checking, then I believe we should discuss this further before I start making more changes. @mjp41 @xFrednet

xFrednet · 2024-12-19T15:36:13Z

2. How do we distinguish between OK C functions (such as those in the CPython standard libraries) and NOT OK C functions — here the answer is possibly check if the type is Pyrona-aware.

I assumed, that we would check if the type is pyrona aware.

If the answer to 2. is Pyrona-awareness checking, then I believe we should discuss this further before I start making more changes.

I'm free after tomorrow's daily, so we could expand it to discuss this further

TobiasWrigstad · 2025-01-13T08:47:41Z

@xFrednet This PR adds a strawman implementation of the LRC dirty flag and a function void _PyRegion_set_lrc_dirty(PyObject *op) on line 175 in regions.c to set the flag (and one more to read if ofc). I'd like to talk about how to integrate that flag with your trace-based checking.

xFrednet · 2025-01-15T15:49:54Z

Small comment regarding the PR description:

Right now, we don't do anything with this flag and never clear it -- that is the next step in this WIP PR.

#35 starts using the dirty flag to determine if we can close a region or not. This might cause some test failures once both are on the phase 3 branch

xFrednet

Three comments, the rest looks good to me. I believe this actually discovered a new case in the WB that I didn't consider previously. FrankenScript has a MoveReference function which I believe we'll also need.

Objects/object.c

Objects/regions.c

xFrednet · 2025-01-15T16:15:54Z

Objects/typeobject.c

+    if (value == NULL || _Pyrona_AddReference(obj, value)) {
+        if (*dictptr) {
+            _Pyrona_RemoveReference(obj, *dictptr);
+        }
+        Py_XSETREF(*dictptr, Py_XNewRef(value));
+        return 0;


I believe this can run into the problem described here: #35 (comment)

But the Py_REGIONCHANGEREFERENCE will sadly not be the solution here. Since we move a reference. I think we have to add another Py_REGIONMOVEREFERENCE util, which would handle cases where the owner of the same object changes.

I drafted up this implementation, but it doesn't handle cowns yet. I have to think a bit longer about this 🤔

int _Py_RegionMoveReference(PyObject* old_src, PyObject* src, PyObject* tgt) { result = 0; regionmetadata *parent = regionmetadata_get_parent(Py_REGION(tgt)); bool was_clean = regionmetadata_is_dirty(parent); // Prevent the parent from closing by marking it as dirty regionmetadata_mark_as_dirty(parent); // First remove the reference to allow the move of bridges and cross // region references. Any parent regions should remain open since they've // been marked as dirty. _Pyrona_RemoveReference(old_src, tgt); // Attempt to move the reference if (!_Pyrona_AddReference(src, tgt)) { // Re-add the previous reference since the new owner is invalid _Pyrona_AddReference(old_src, tgt); result = -1 } if (was_clean) { regionmetadata_mark_as_not_dirty(parent); } // TODO, this doesn't handle cases where `old_src` is a cown... return result; }

This can be optimized further. We can add a fast track, if the old and new src are from the same region and the target is not bridge object.

EDIT: I now found an easier way based on the changes in #34 we can simply inc the LRC do the move and then DEC the LRC again, all while keeping the region open do to the virtual LRC we added :D

_{These are the problems that make my brain happy to think about :D}

I always thought of move between regions as "move to local" followed by "move from local". I think that might be what you end result might be?

I do wonder if we should build a Py_XSETREF_WITH_REGION operation, like I have added for Py_CLEAR in my PR.

I do wonder if we should build a Py_XSETREF_WITH_REGION operation, like I have added for Py_CLEAR in my PR.

I like the idea, but feel like the error handling is too inconsistent, some require a jump, others just a simple return 0 or return -1.

I always thought of move between regions as "move to local" followed by "move from local". I think that might be what you end result might be?

Yes, implementing a change reference like that should work and keep the region open, as we expect.

TobiasWrigstad · 2025-01-15T19:08:51Z

Small comment regarding the PR description:

Right now, we don't do anything with this flag and never clear it -- that is the next step in this WIP PR.

#35 starts using the dirty flag to determine if we can close a region or not. This might cause some test failures once both are on the phase 3 branch

Right. I think the PR description is >1 month old by now :)

TobiasWrigstad · 2025-01-15T19:10:04Z

Three comments, the rest looks good to me. I believe this actually discovered a new case in the WB that I didn't consider previously. FrankenScript has a MoveReference function which I believe we'll also need.

What does that to? I thought move was just syntactic sugar for remove + add (in some order).

xFrednet · 2025-01-15T20:01:49Z

Three comments, the rest looks good to me. I believe this actually discovered a new case in the WB that I didn't consider previously. FrankenScript has a MoveReference function which I believe we'll also need.

What does that to? I thought move was just syntactic sugar for remove + add (in some order).

In FrankenScript it is just syntactic sugar, we can get away with it since every problematic add will stop execution and show the error. However, here we need to make sure that the objects are in a "decent" state when the exception is thrown.

The problem with removing and then adding a reference is, that the removal of the reference could close the region, which could intern release a cown. @mjp41 last change in #34 gave me an idea. We can just increase the LRC during this move. That will prevent the region from closing and then we can do the "remove reference, add reference dance" without accidentally closing any regions.

TobiasWrigstad · 2025-01-15T20:11:09Z

Three comments, the rest looks good to me. I believe this actually discovered a new case in the WB that I didn't consider previously. FrankenScript has a MoveReference function which I believe we'll also need.

What does that to? I thought move was just syntactic sugar for remove + add (in some order).

In FrankenScript it is just syntactic sugar, we can get away with it since every problematic add will stop execution and show the error. However, here we need to make sure that the objects are in a "decent" state when the exception is thrown.

The problem with removing and then adding a reference is, that the removal of the reference could close the region, which could intern release a cown. @mjp41 last change in #34 gave me an idea. We can just increase the LRC during this move. That will prevent the region from closing and then we can do the "remove reference, add reference dance" without accidentally closing any regions.

I believe we always add before move. Does that prevent the premature closing?

xFrednet · 2025-01-15T20:13:35Z

I believe we always add before move. Does that prevent the premature closing?

Yes, it prevents the premature closing. But could result in errors for bridge objects.

TobiasWrigstad · 2025-01-15T20:30:32Z

I believe we always add before move. Does that prevent the premature closing?

Yes, it prevents the premature closing. But could result in errors for bridge objects.

Meaning the error in #35 (comment) ?

xFrednet · 2025-01-15T20:39:52Z

I believe we always add before move. Does that prevent the premature closing?

Yes, it prevents the premature closing. But could result in errors for bridge objects.

Meaning the error in #35 (comment) ?

Yes! I saw your answer in that PR, but first wanted to think a bit more about it before answering.

TobiasWrigstad · 2025-01-15T20:41:29Z

I believe we always add before move. Does that prevent the premature closing?

Yes, it prevents the premature closing. But could result in errors for bridge objects.

Meaning the error in #35 (comment) ?

Yes! I saw your answer in that PR, but first wanted to think a bit more about it before answering.

Great, thanks for clarifying! So we are ultimately dealing with a single case. Nice!

mjp41 · 2025-01-17T11:17:36Z

Objects/object.c

+        if (Py_CHECKWRITE(v)) {
+            if (!Py_IS_REGION_AWARE(tp)) {
+                _PyObject_mark_region_as_dirty(v);
+            }


Should we modify CHECKWRITE to do this as well?

Ah you mean Py-CHECKWRITE(obj) marks the region as dirty if !Py_IS_REGION_AWARE(obj->ob_type)?

That's excellent! And obvious in hindsight.

If we add that check to CHECKWRITE that will add that check to many more places that what I think we currently would need it. Maybe that's OK because branch prediction.

mjp41 · 2025-01-17T11:29:02Z

Objects/typeobject.c

+    if (value == NULL || _Pyrona_AddReference(obj, value)) {
+        if (*dictptr) {
+            _Pyrona_RemoveReference(obj, *dictptr);
+        }
+        Py_XSETREF(*dictptr, Py_XNewRef(value));
+        return 0;


I always thought of move between regions as "move to local" followed by "move from local". I think that might be what you end result might be?

mjp41 · 2025-01-17T11:29:50Z

Objects/typeobject.c

+    if (value == NULL || _Pyrona_AddReference(obj, value)) {
+        if (*dictptr) {
+            _Pyrona_RemoveReference(obj, *dictptr);
+        }
+        Py_XSETREF(*dictptr, Py_XNewRef(value));
+        return 0;


I do wonder if we should build a Py_XSETREF_WITH_REGION operation, like I have added for Py_CLEAR in my PR.

TobiasWrigstad · 2025-01-17T15:47:52Z

@mjp41 regarding move btw regions as reg1 --> local --> reg2 that seems logical to me. I'm not sure I understand your comment fully though. Are you suggesting what the REMOVE barrier should do?

TobiasWrigstad · 2025-01-17T15:49:23Z

@mjp41 I don't follow the Py_XSETREF_WITH_REGION comment. Sorry, maybe I'm in a completely different head space today... Can you unpack?

mjp41 · 2025-01-17T15:53:24Z

@mjp41 regarding move btw regions as reg1 --> local --> reg2 that seems logical to me. I'm not sure I understand your comment fully though. Are you suggesting what the REMOVE barrier should do?

I was more reacting that I didn't think a "MOVE" between regions was needed, but it could be more optimal.

mjp41 · 2025-01-19T09:54:47Z

@mjp41 I don't follow the Py_XSETREF_WITH_REGION comment. Sorry, maybe I'm in a completely different head space today... Can you unpack?

So the code here:

cpython/Include/cpython/object.h

Lines 371 to 386 in 591bfc7

    
           #define Py_XSETREF(dst, src) \ 
        
               do { \ 
        
                   _Py_TYPEOF(dst)* _tmp_dst_ptr = &(dst); \ 
        
                   _Py_TYPEOF(dst) _tmp_old_dst = (*_tmp_dst_ptr); \ 
        
                   *_tmp_dst_ptr = (src); \ 
        
                   Py_XDECREF(_tmp_old_dst); \ 
        
               } while (0) 
        
           #else 
        
           #define Py_XSETREF(dst, src) \ 
        
               do { \ 
        
                   PyObject **_tmp_dst_ptr = _Py_CAST(PyObject**, &(dst)); \ 
        
                   PyObject *_tmp_old_dst = (*_tmp_dst_ptr); \ 
        
                   PyObject *_tmp_src = _PyObject_CAST(src); \ 
        
                   memcpy(_tmp_dst_ptr, &_tmp_src, sizeof(PyObject*)); \ 
        
                   Py_XDECREF(_tmp_old_dst); \ 
        
               } while (0)

Does the update and the decref. I wonder if we should have a related macro that does the update, region ref remove, and the decref.

TobiasWrigstad · 2025-02-02T10:12:59Z

@mjp41 I've added the proposed SET_XREF_WITH_REGION now (and rebased on new Phase3).

Co-authored-by: Fridtjof Stoldt <xFrednet@gmail.com>

Currently called from one place.

xFrednet

I only skimmed over the changes again, but this looks good to me. It probably makes sense to merge this and just do any fixes in followup PRs

TobiasWrigstad mentioned this pull request Dec 14, 2024

Phase 3: Py_CHECKWRITE -> write barrier todo list #10

Open

64 tasks

TobiasWrigstad force-pushed the wb-added-to-object.c branch from b47926d to 9ccdfaa Compare December 15, 2024 00:38

TobiasWrigstad force-pushed the wb-added-to-object.c branch from 9ccdfaa to 7e956e9 Compare January 13, 2025 08:41

TobiasWrigstad force-pushed the wb-added-to-object.c branch from 7e956e9 to ebb2b28 Compare January 15, 2025 15:03

TobiasWrigstad changed the title ~~WIP Adding WB to object.c~~ Adding WB to object.c Jan 15, 2025

TobiasWrigstad marked this pull request as ready for review January 15, 2025 15:04

TobiasWrigstad requested review from mjp41 and xFrednet January 15, 2025 15:04

xFrednet reviewed Jan 15, 2025

View reviewed changes

mjp41 reviewed Jan 17, 2025

View reviewed changes

mjp41 changed the title ~~Adding WB to object.c~~ Phase 3: Adding WB to object.c Jan 21, 2025

TobiasWrigstad force-pushed the wb-added-to-object.c branch from 5b6bc25 to 6d2e2ce Compare February 2, 2025 09:38

Added write barriers to object.c; uses the TP_REGION_AWARE flag

4f6fd75

TobiasWrigstad and others added 6 commits February 24, 2025 18:09

Update Objects/regions.c

15cdeaa

Co-authored-by: Fridtjof Stoldt <xFrednet@gmail.com>

Marking a region dirty now marks its parent regions dirty as well

7444e96

Removing the marking of parents as dirty

1577c57

Store a bridge object ref r into a region-unaware type => dirty r

f1692b8

Added Py_XSETREF_WITH_REGION

2f92d2c

Currently called from one place.

Called XSET_REF_WITH_REGION also in GenericSetDict

610dcb0

TobiasWrigstad force-pushed the wb-added-to-object.c branch from 6d2e2ce to 610dcb0 Compare February 24, 2025 17:20

xFrednet approved these changes Apr 3, 2025

View reviewed changes

Phase 3: Adding WB to object.c #26

Are you sure you want to change the base?

Phase 3: Adding WB to object.c #26

Uh oh!

Conversation

TobiasWrigstad commented Dec 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TobiasWrigstad commented Dec 19, 2024

Uh oh!

xFrednet commented Dec 19, 2024

Uh oh!

TobiasWrigstad commented Jan 13, 2025

Uh oh!

xFrednet commented Jan 15, 2025

Uh oh!

xFrednet left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

xFrednet Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

TobiasWrigstad commented Jan 15, 2025

Uh oh!

TobiasWrigstad commented Jan 15, 2025

Uh oh!

xFrednet commented Jan 15, 2025

Uh oh!

TobiasWrigstad commented Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xFrednet commented Jan 15, 2025

Uh oh!

TobiasWrigstad commented Jan 15, 2025

Uh oh!

xFrednet commented Jan 15, 2025

Uh oh!

TobiasWrigstad commented Jan 15, 2025

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!

TobiasWrigstad commented Jan 17, 2025

Uh oh!

TobiasWrigstad commented Jan 17, 2025

Uh oh!

mjp41 commented Jan 17, 2025

Uh oh!

mjp41 commented Jan 19, 2025

Uh oh!

TobiasWrigstad commented Feb 2, 2025

Uh oh!

xFrednet left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

TobiasWrigstad commented Dec 14, 2024 •

edited

Loading

xFrednet left a comment •

edited

Loading

xFrednet Jan 15, 2025 •

edited

Loading

TobiasWrigstad commented Jan 15, 2025 •

edited

Loading