Weakref support #1267

amos402 · 2020-10-22T09:39:46Z

codecov-io · 2020-10-22T10:10:16Z

src/runtime/classbase.cs

src/runtime/classmanager.cs

lostmsu · 2020-11-17T23:43:29Z

src/runtime/exceptions.cs

+
+        public static int tp_init(IntPtr ob, IntPtr args, IntPtr kwds)
+        {
+            Exceptions.SetArgsAndCause(ob);


Please, move the original comment here.

What comment?

NIT: You seem to have moved this from CLRObject, and removed

// Fix the BaseException args (and __cause__ in case of Python 3) // slot if wrapping a CLR exception

The comment on SetArgsAndCause won't be enough?

lostmsu · 2020-11-17T23:47:24Z

src/runtime/runtime.cs

            ob = IntPtr.Zero;
        }

+        internal static unsafe void Py_SETREF(IntPtr ob, int offset, IntPtr target)


This seem to be silently stealing a reference to target. I'd prefer it to explicitly take NewReference or ref NewReference (to clear the original variable), or take BorrowedReference and IncRef here.

Ideally, I would also check ob and offset for sanity.

Also, this really should also be outside the Runtime class, but I am not sure where is best to put it.

The original version is the C macro, the implementation used pointer offset, maybe is not suited for such a high-level type to be involved. I want to keep such things simple, If you need that, maybe create a high-level wrapper.😓

NewReference is not high-level. It is just a struct.

Then you have to pull the pointer and add an offset, what a tortuous path.

It is better, than accidentally forgetting to incref the target in the caller.

lostmsu · 2020-11-18T00:04:16Z

src/runtime/metatype.cs

+            //TypeManager.CopySlot(base_type, type, TypeOffset.tp_dealloc);

            // Hmm - the standard subtype_traverse, clear look at ob_size to
            // do things, so to allow gc to work correctly we need to move
            // our hidden handle out of ob_size. Then, in theory we can
            // comment this out and still not crash.
-            TypeManager.CopySlot(base_type, type, TypeOffset.tp_traverse);
-            TypeManager.CopySlot(base_type, type, TypeOffset.tp_clear);
-
+            //TypeManager.CopySlot(base_type, type, TypeOffset.tp_traverse);
+            //TypeManager.CopySlot(base_type, type, TypeOffset.tp_clear);


Please, remove dead code entirely.

Can you also comment why is this no longer necessary?

These slots can inherit from the base type automatically. Emmm, I can add some comments here, but it still needs the dead code comment as an explanation.

The comment above also seems dangerously wrong.

Hmmm, that was an old comment, that time before we unify the shutdown progresses, it can't and won't use tp_traverse and tp_clear. Seems it can be removed at one time.

Any dead code should be removed immediately.

lostmsu · 2020-11-18T00:07:47Z

src/runtime/runtime.cs

@@ -1991,6 +1996,9 @@ internal static IntPtr PyType_GenericAlloc(IntPtr type, long n)
        [DllImport(_PythonDll, CallingConvention = CallingConvention.Cdecl)]
        internal static extern void PyObject_GC_UnTrack(IntPtr tp);

+        [DllImport(_PythonDll, CallingConvention = CallingConvention.Cdecl)]
+        internal static extern void PyObject_ClearWeakRefs(IntPtr obj);


We should try to use *Reference types for clarity for all new PInvoke functions.

That makes sense, but can we just replace them all at once.😅

@amos402 are you prepared to make this happen? )

lostmsu · 2020-11-18T00:11:51Z

src/runtime/runtime.cs

@@ -1982,6 +1984,9 @@ internal static IntPtr PyType_GenericAlloc(IntPtr type, long n)
        [DllImport(_PythonDll, CallingConvention = CallingConvention.Cdecl)]
        internal static extern IntPtr _PyObject_GetDictPtr(IntPtr obj);

+        [DllImport(_PythonDll, CallingConvention = CallingConvention.Cdecl)]
+        internal static extern IntPtr _PyObject_GC_Calloc(IntPtr basicsize);


It is a very bad idea to use private function. Why is it required? Can the same be done with only public functions?

I'm afraid there's no such public function now, it's required to allocate a custom size Python GC object.

lostmsu · 2020-11-18T00:23:56Z

src/runtime/typemanager.cs


-            Marshal.WriteIntPtr(type, TypeOffset.tp_base, py_type);
+            IntPtr type = Runtime._PyObject_GC_Calloc(new IntPtr(metaSize));


Specifically, can we use PyType_FromSpec here?

Maybe not, PyType_FromSpec still use PyType_Type to allocate memory, which means it would occur the same problem as before.

Actually, I think I solved this one. Please see 9682dc6
So you should not need _PyObject_GC_Calloc anymore.

No, it didn't, see https://github.com/python/cpython/blob/fbc7723778be01b8f3bb72d2dcac15ab9fbb9923/Objects/typeobject.c#L1051-L1082

Yes, it did, see https://github.com/python/cpython/blob/8b3414818f5289eac530bf38bcfbd7b2b851805c/Include/cpython/objimpl.h#L21 :-P

It's not about the type size which the MetaType can generate. It's about the size of MetaType itself. You can't create the MetaType by using PyType_GenericAlloc(PyTypeType, 0), unless you

old_size = PyTypeType.tp_basicsize; PyTypeType.tp_basicsize += x; PyType_GenericAlloc(PyTypeType, 0); PyTypeType.tp_basicsize = old_size;

As long as it claimed itself as a managed type, it should have a slot indicate the place of its instances where the related GCHandle would be stored. As for now, the MetaType doesn't attach any C# object, so it remains 0. Look back to the master, if you put the PyCLRMetaType to GetManagedObject, actually it would read the memory it should not read(the part of the reserved tp_itemsize). The code doesn't explicitly call something like that, but something may trigger it does, like domain reload may visit it through PyVisit. Even it won't crash right now, but it's hard to say that's a kind of sanity code.
Of course, you can just remove the Manged flag from PyCLRMetaType to avoid that. But I suggest following the old rule: the type of a CLR type is a managed type.
Another approach is to allocate with PyObject_Malloc, but that means it has to be initialized manually, e.g. call _Py_NewReference manually on debug version, add it to GCTrack.
Nevertheless, the point is, the type must clarify itself what's itself and what it can do, that commit avoided some problems, but didn't solve completely.

As far as I can see, CLR Metatype does not contain GCHandle, so it does not need GetManagedObject to work on its instances. So I think my implementation linked above is correct. Perhaps PyCLRMetaType should not be a managed object then?

It indeed has a bit of struggle with that, the PyCLRMetaType it's the ancestor of all managed types, if it claimed it's not a managed type, it against the commonsense. Currently, it doesn't need to be bound with a clr instance even it can. Well, just an identity problem.

I think it makes sense, that CLR metatype is not a managed type. In my mind managed type == type, whose implementation is managed == has a managed object, that implements functionality == has GCHandle pointing to that object. CLR metatype does not have a GCHandle.

src/runtime/typemanager.cs

lostmsu · 2020-12-21T18:38:58Z

src/runtime/native/GeneratedTypeOffsets.cs

+        // XXX: Use field to trick the public property finding
+        public readonly int Size;
+


Why do you think this is better, than the single line you removed from ABI.cs? This is hacky and uses reflection.

It's not related to that line, the reason for removing that line just because was because that's not the fixed slot anymore.
Well, it was just need something to store the size, tp_basicsize might capable do that too, this part can be improved.

This field has the same purpose and value as the one you removed from ABI - sizeof(PyTypeObject)

src/runtime/native/TypeOffset.cs

lostmsu · 2021-02-19T18:08:45Z

src/embed_tests/TestClass.cs

+        public void WeakRefForClrObject()
+        {
+            var obj = new MyClass();
+            using (var scope = Py.CreateScope())


NIT: use C# 9 using syntax

src/runtime/classbase.cs

lostmsu · 2021-02-19T18:18:49Z

src/runtime/metatype.cs

+            //TypeManager.CopySlot(base_type, type, TypeOffset.tp_dealloc);

            // Hmm - the standard subtype_traverse, clear look at ob_size to
            // do things, so to allow gc to work correctly we need to move
            // our hidden handle out of ob_size. Then, in theory we can
            // comment this out and still not crash.
-            TypeManager.CopySlot(base_type, type, TypeOffset.tp_traverse);
-            TypeManager.CopySlot(base_type, type, TypeOffset.tp_clear);
-
+            //TypeManager.CopySlot(base_type, type, TypeOffset.tp_traverse);
+            //TypeManager.CopySlot(base_type, type, TypeOffset.tp_clear);


Any dead code should be removed immediately.

lostmsu · 2021-02-19T18:22:32Z

src/runtime/metatype.cs

+        public IntPtr ClrHandleOffset;
+        public IntPtr ClrHandle;


These two refer to different handles, yet are named similarly, which is confusing.

Besides, in C# 9 we can start using nint instead of IntPtr for non-pointers.

What name do you prefer?

This is just a NIT. Maybe TypeClrHandle and InstanceClrHandleOffset?

src/runtime/native/TypeOffset.cs

lostmsu · 2021-02-19T18:30:30Z

src/runtime/pyobject.cs

+            IntPtr op = typeof(Exception).IsAssignableFrom(ob.GetType()) ?
+                Exceptions.GetExceptHandle((Exception)ob)
+                : CLRObject.GetInstHandle(ob);


I feel like Exceptions.GetExceptionHandle should be built into CLRObject.GetInstHandle.

The exception object is a special case, we should keep it outside from the generic case GetInstHandle, for example, if it has multiple special cases instead of one, you won't want to contaminate the CLRObject methods, right? Just like I removed the special case code from the CLRObject's constructor.

lostmsu · 2021-02-19T18:32:55Z

src/runtime/runtime.cs

+                if (obj.RefCount > 1)
+                {
+                    obj.FreeGCHandle();
+                    Marshal.WriteIntPtr(obj.pyHandle, ObjectOffset.magic(obj.tpHandle), IntPtr.Zero);
                }


Why can't you do this unconditionally? This condition makes reasoning about the state much harder.

If obj.RefCount == 1, the next XDecref would dealloc the obj, thus it's no need to call these things.

Is this similar to tp_clear in any way? I would suggest to extract these two updates (FreeGCHandle and zeroing out) into a method.

The point being tp_clear has certain semantics, so it is easier to understand code containing a call to it, rather than has same actions inline.

Also, if RefCount will correctly return 0, I'd move this if below.

Not exactly, tp_clear is used to clear the Python object reference in order to break reference cycles. These codes are used to disconnect the relation between Python and clr, if the RefCount reached 0, their relation would naturally be gone.
Their relation needs to be kept if RefCount > 1 because in tp_dealloc, it needs to obtain the bound object.
tp_dealloc didn't zero out the handle because this chunk of memory would be freed soon after tp_dealloc, extract these into a method is not really necessary.
Since after dealloc the memory may be released, the if scope cannot move to below.

lostmsu · 2021-02-19T18:45:06Z

src/runtime/typemanager.cs


-            Marshal.WriteIntPtr(type, TypeOffset.tp_base, py_type);
+            IntPtr type = Runtime._PyObject_GC_Calloc(new IntPtr(metaSize));


As far as I can see, CLR Metatype does not contain GCHandle, so it does not need GetManagedObject to work on its instances. So I think my implementation linked above is correct. Perhaps PyCLRMetaType should not be a managed object then?

lostmsu

lostmsu · 2021-02-21T19:33:48Z

src/runtime/runtime.cs

+                if (obj.RefCount > 1)
+                {
+                    obj.FreeGCHandle();
+                    Marshal.WriteIntPtr(obj.pyHandle, ObjectOffset.magic(obj.tpHandle), IntPtr.Zero);
                }


Is this similar to tp_clear in any way? I would suggest to extract these two updates (FreeGCHandle and zeroing out) into a method.

The point being tp_clear has certain semantics, so it is easier to understand code containing a call to it, rather than has same actions inline.

Also, if RefCount will correctly return 0, I'd move this if below.

src/runtime/classbase.cs

lostmsu · 2021-02-21T19:42:25Z

src/runtime/typemanager.cs


-            Marshal.WriteIntPtr(type, TypeOffset.tp_base, py_type);
+            IntPtr type = Runtime._PyObject_GC_Calloc(new IntPtr(metaSize));


I think it makes sense, that CLR metatype is not a managed type. In my mind managed type == type, whose implementation is managed == has a managed object, that implements functionality == has GCHandle pointing to that object. CLR metatype does not have a GCHandle.

lostmsu · 2021-12-25T18:29:17Z

amos402 added 3 commits September 24, 2020 15:13

Support weakref for CLR types

d6c9716

Merge branch 'master' into weakref-support

5d57e5a

Apply unified mechanism for GetManagedObjectType

f08813c

amos402 mentioned this pull request Oct 22, 2020

Exception in finalizer of custom encoder #1256

Open

lostmsu requested changes Nov 18, 2020

View reviewed changes

filmor and others added 4 commits December 10, 2020 18:50

Merge branch 'master' into weakref-support

b014ce1

Merge branch 'master' into weakref-support

e7f97a9

* Remove magic offset

b68f0a4

* Add Size field to TypeOffset * Replenish lacked used offset

Merge branch 'master' into weakref-support

10000

b4b1cff

lostmsu reviewed Dec 21, 2020

View reviewed changes

amos402 added 4 commits February 10, 2021 21:03

Merge branch 'master' into weakref-support

ff35e6d

Merge branch 'master' into weakref-support

ea3d47a

Fix Shutdown crash

958a8eb

Use PyTypeType.tp_basicsize as size of PyHeapTypeObject directly inst…

53ad8d9

…ead of calculate it

amos402 mentioned this pull request Feb 19, 2021

Finalizer Fixes #1383

Closed

4 tasks

lostmsu requested changes Feb 19, 2021

View reviewed changes

amos402 added 2 commits February 21, 2021 16:25

Merge branch 'master' into weakref-support

647bc8a

* Use C# 9.0 for Python.EmbeddingTest.csproj

f474039

* Remove dead code

lostmsu requested changes Feb 21, 2021

View reviewed changes

lostmsu mentioned this pull request Apr 12, 2021

Replace magic offsets with proper offset calculation #1441

Merged

lostmsu closed this Dec 25, 2021

lostmsu added a commit to losttech/pythonnet that referenced this pull request Apr 8, 2022

clear weakref list when reflected object is destroyed

2293b33

this was missed when pythonnet#1267 was superseded

lostmsu mentioned this pull request Apr 8, 2022

Clear weakref list when reflected object is destroyed #1758

Merged

1 task

filmor pushed a commit that referenced this pull request Apr 9, 2022

clear weakref list when reflected object is destroyed (#1758)

50da522

this was missed when #1267 was superseded

Weakref support #1267

Weakref support #1267

Uh oh!

Conversation

Uh oh!

What does this implement/fix? Explain your changes.

Does this close any currently open issues?

Any other comments?

Checklist

Uh oh!

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

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!

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!

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!

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!

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!