Conversation
Codecov Report
@@ Coverage Diff @@
## master #1267 +/- ##
=======================================
Coverage 86.25% 86.25%
=======================================
Files 1 1
Lines 291 291
=======================================
Hits 251 251
Misses 40 40
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
|
|
||
| public static int tp_init(IntPtr ob, IntPtr args, IntPtr kwds) | ||
| { | ||
| Exceptions.SetArgsAndCause(ob); |
There was a problem hiding this comment.
Please, move the original comment here.
There was a problem hiding this 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
There was a problem hiding this comment.
The comment on SetArgsAndCause won't be enough?
| ob = IntPtr.Zero; | ||
| } | ||
|
|
||
| internal static unsafe void Py_SETREF(IntPtr ob, int offset, IntPtr target) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.😓
There was a problem hiding this comment.
NewReference is not high-level. It is just a struct.
There was a problem hiding this comment.
Then you have to pull the pointer and add an offset, what a tortuous path.
There was a problem hiding this comment.
It is better, than accidentally forgetting to incref the target in the caller.
src/runtime/metatype.cs
Outdated
| //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); |
There was a problem hiding this comment.
Please, remove dead code entirely.
Can you also comment why is this no longer necessary?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The comment above also seems dangerously wrong.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Any dead code should be removed immediately.
src/runtime/runtime.cs
Outdated
| internal static extern void PyObject_GC_UnTrack(IntPtr tp); | ||
|
|
||
| [DllImport(_PythonDll, CallingConvention = CallingConvention.Cdecl)] | ||
| internal static extern void PyObject_ClearWeakRefs(IntPtr obj); |
There was a problem hiding this comment.
We should try to use *Reference types for clarity for all new PInvoke functions.
There was a problem hiding this comment.
That makes sense, but can we just replace them all at once.😅
src/runtime/runtime.cs
Outdated
| internal static extern IntPtr _PyObject_GetDictPtr(IntPtr obj); | ||
|
|
||
| [DllImport(_PythonDll, CallingConvention = CallingConvention.Cdecl)] | ||
| internal static extern IntPtr _PyObject_GC_Calloc(IntPtr basicsize); |
There was a problem hiding this comment.
It is a very bad idea to use private function. Why is it required? Can the same be done with only public functions?
There was a problem hiding this comment.
I'm afraid there's no such public function now, it's required to allocate a custom size Python GC object.
| int metaSize = heapTypeSize + Marshal.SizeOf(typeof(ClrMetaTypeEx)); | ||
|
|
||
| Marshal.WriteIntPtr(type, TypeOffset.tp_base, py_type); | ||
| IntPtr type = Runtime._PyObject_GC_Calloc(new IntPtr(metaSize)); |
There was a problem hiding this comment.
Specifically, can we use PyType_FromSpec here?
There was a problem hiding this comment.
Maybe not, PyType_FromSpec still use PyType_Type to allocate memory, which means it would occur the same problem as before.
There was a problem hiding this comment.
Actually, I think I solved this one. Please see 9682dc6
So you should not need _PyObject_GC_Calloc anymore.
There was a problem hiding this comment.
There was a problem hiding this comment.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
* Add Size field to TypeOffset * Replenish lacked used offset
| // XXX: Use field to trick the public property finding | ||
| public readonly int Size; | ||
|
|
There was a problem hiding this comment.
Why do you think this is better, than the single line you removed from ABI.cs? This is hacky and uses reflection.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This field has the same purpose and value as the one you removed from ABI - sizeof(PyTypeObject)
src/embed_tests/TestClass.cs
Outdated
| public void WeakRefForClrObject() | ||
| { | ||
| var obj = new MyClass(); | ||
| using (var scope = Py.CreateScope()) |
src/runtime/metatype.cs
Outdated
| //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); |
There was a problem hiding this comment.
Any dead code should be removed immediately.
src/runtime/metatype.cs
Outdated
| public IntPtr ClrHandleOffset; | ||
| public IntPtr ClrHandle; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
What name do you prefer?
There was a problem hiding this comment.
This is just a NIT. Maybe TypeClrHandle and InstanceClrHandleOffset?
| IntPtr op = typeof(Exception).IsAssignableFrom(ob.GetType()) ? | ||
| Exceptions.GetExceptHandle((Exception)ob) | ||
| : CLRObject.GetInstHandle(ob); |
There was a problem hiding this comment.
I feel like Exceptions.GetExceptionHandle should be built into CLRObject.GetInstHandle.
There was a problem hiding this comment.
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.
| if (obj.RefCount > 1) | ||
| { | ||
| obj.FreeGCHandle(); | ||
| Marshal.WriteIntPtr(obj.pyHandle, ObjectOffset.magic(obj.tpHandle), IntPtr.Zero); | ||
| } |
There was a problem hiding this comment.
Why can't you do this unconditionally? This condition makes reasoning about the state much harder.
There was a problem hiding this comment.
If obj.RefCount == 1, the next XDecref would dealloc the obj, thus it's no need to call these things.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| int metaSize = heapTypeSize + Marshal.SizeOf(typeof(ClrMetaTypeEx)); | ||
|
|
||
| Marshal.WriteIntPtr(type, TypeOffset.tp_base, py_type); | ||
| IntPtr type = Runtime._PyObject_GC_Calloc(new IntPtr(metaSize)); |
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
I think it is good, except the _PyObject_GC_Calloc, which should be replaced as suggested.
| if (obj.RefCount > 1) | ||
| { | ||
| obj.FreeGCHandle(); | ||
| Marshal.WriteIntPtr(obj.pyHandle, ObjectOffset.magic(obj.tpHandle), IntPtr.Zero); | ||
| } |
There was a problem hiding this comment.
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.
| int metaSize = heapTypeSize + Marshal.SizeOf(typeof(ClrMetaTypeEx)); | ||
|
|
||
| Marshal.WriteIntPtr(type, TypeOffset.tp_base, py_type); | ||
| IntPtr type = Runtime._PyObject_GC_Calloc(new IntPtr(metaSize)); |
There was a problem hiding this comment.
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.
|
I believe this has been superseded by one of the major changes in 3.0. This works on the latest master: >>> import clr
>>> from System import Uri
>>> u = Uri('http://ddg.com')
>>> import weakref
>>> weak = weakref.ref(u)
>>> u
<System.Uri object at 0x000001ED729D8240>
>>> u.ToString()
'http://ddg.com/'
>>> weak().ToString()
'http://ddg.com/' |
this was missed when pythonnet#1267 was superseded
this was missed when #1267 was superseded
What does this implement/fix? Explain your changes.
pyHandleandtpHandleinManagedType, the usage ofobj.tpHandle == obj.tpHandlerepresents theobjas aClassBaseitself is no need anymoreweakrefcan be used for CLR objectSolved the problem that the code like as above would occur a crash because the slot of weakref and the slot of clr handle was overlapping.
4. Minor refactor of
SlotsHolderDoes this close any currently open issues?
#1219 (comment)
Any other comments?
...
Checklist
Check all those that are applicable and complete.
AUTHORSCHANGELOG