Repository navigation
GH-95245: Move weakreflist into the pre-header. - #95657
markshannon wants to merge 2 commits into
Conversation
|
@ericsnowcurrently You've been playing with weakreflists recently. |
ericsnowcurrently
left a comment
ericsnowcurrently
left a comment
There was a problem hiding this comment.
Mostly LGTM
I'd just like some clarity on what Py_TPFLAGS_MANAGED_DICT means now and if we need a Py_TPFLAGS_MANAGED_WEAKLIST too.
| assert(!ctx->base->tp_itemsize); | ||
| type->tp_weaklistoffset = slotoffset; | ||
| slotoffset += sizeof(PyObject *); | ||
| if (type->tp_flags & Py_TPFLAGS_MANAGED_DICT) { |
There was a problem hiding this comment.
Could Py_TPFLAGS_MANAGED_DICT be set even if wasn't set on line 3089? In other words, could the flag have been set before type_new_descriptors() was called?
If so, that implies that Py_TPFLAGS_MANAGED_DICT also implies a managed weaklist, which I'm not sure is correct.
Otherwise the assert on line 3088 should probably move up to before line 3082.
There was a problem hiding this comment.
Further, if it's always either both-are-managed or neither then it would be helpful if the flag were more clear (e.g. Py_TPFLAGS_MANAGED_DICT_AND_WEAKLIST).
If either can be managed separately then there should probably be a separate Py_TPFLAGS_MANAGED_WEAKLIST flag.
That might be necessary anyway since Py_TPFLAGS_MANAGED_DICT is already part of 3.11.
There was a problem hiding this comment.
We want both to be managed, or neither, but our hand might be forced in the case of multiple inheritance.
One base might have a managed dict, and the other an explicit weakreflist.
I expect this case to be rare.
Currently Py_TPFLAGS_MANAGED_DICT is internal, it is set by the VM, but shouldn't be set by third-party code.
When we make it public, as we should do for 3.12, then it would make sense to add Py_TPFLAGS_MANAGED_WEAKLIST.
I'd like some sort of decision on #95589 first, though.
There was a problem hiding this comment.
Thinking about this a bit more, we can ensure both are explicit or both managed.
If one is explicit, the we make both explicit.
Moving from an explicitly declare offset to a managed one is unsafe, so we can't do that.
But we can do it the other way around.
There was a problem hiding this comment.
Here it only matters in the context of type_new_descriptors(). If we can guarantee the both-or-neither aspect in this "limited" context then that is sufficient for this PR (assuming we leave a comment to that effect).
There was a problem hiding this comment.
Regardless, my original question stands: could Py_TPFLAGS_MANAGED_DICT already be set when type_new_descriptors() is called? If so, the branch on 3095 is now problematic, no?
| type->tp_weaklistoffset = slotoffset; | ||
| slotoffset += sizeof(PyObject *); | ||
| if (type->tp_flags & Py_TPFLAGS_MANAGED_DICT) { | ||
| type->tp_weaklistoffset = -4 * sizeof(PyObject *); |
There was a problem hiding this comment.
Is the pre-header layout documented, either in a comment, the docs, or an adjacent .txt file? Either way, I don't see the weaklist's place in the pre-header added to such documentation in this PR. That information would be super helpful, since I can only guess what -4 * sizeof(PyObject *) means. A short comment here pointing to that documentation would be nice too.
There was a problem hiding this comment.
Documenting the object layout is on the to-do list.
Early draft here: https://github.com/faster-cpython/cpython/blob/document-object-layout/Objects/object_layout.md
| assert((type->tp_flags & Py_TPFLAGS_MANAGED_DICT) == 0); | ||
| type->tp_flags |= Py_TPFLAGS_MANAGED_DICT; | ||
| if (type->tp_flags & Py_TPFLAGS_MANAGED_DICT) { |
There was a problem hiding this comment.
Likewise about Py_TPFLAGS_MANAGED_DICT implying something it didn't before (but only if the flag could possibly be set before type_new_descriptors() gets called).
|
When you're done making the requested changes, leave the comment: |
3 participants
Part 2 of #95245
Stores the weakreflist pointer in the now vacant space before the dict/values pointer.