Skip to content

GH-95245: Move weakreflist into the pre-header. #95657

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
markshannon wants to merge 2 commits into from
Closed

GH-95245: Move weakreflist into the pre-header. #95657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
16746d9
Move weakreflist into the pre-header.
markshannon Jul 26, 2022
bcf23ce
Add news
markshannon Aug 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Include/internal/pycore_object.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ extern int _Py_CheckSlotResult(

// Test if a type supports weak references
static inline int _PyType_SUPPORTS_WEAKREFS(PyTypeObject *type) {
return (type->tp_weaklistoffset > 0);
return (type->tp_weaklistoffset != 0);
}

extern PyObject* _PyType_AllocNoTrack(PyTypeObject *type, Py_ssize_t nitems);
Expand Down
3 changes: 3 additions & 0 deletions Misc/NEWS.d/next/Core and Builtins/2022-08-04-16-04-18.gh-issue-95245.N4gOUV.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Reduces the size of a "simple" Python object from 8 to 6 words by moving the
weakreflist pointer into the pre-header directly before the object's
dict/values pointer.
34 changes: 23 additions & 11 deletions Objects/typeobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2483,8 +2483,9 @@ subtype_getweakref(PyObject *obj, void *context)
return NULL;
}
_PyObject_ASSERT((PyObject *)type,
type->tp_weaklistoffset > 0);
type->tp_weaklistoffset != 0);
_PyObject_ASSERT((PyObject *)type,
(type->tp_weaklistoffset == -4 * ((int)sizeof(PyObject *))) ||
((type->tp_weaklistoffset + sizeof(PyObject *))
<= (size_t)(type->tp_basicsize)));
weaklistptr = (PyObject **)((char *)obj + type->tp_weaklistoffset);
Expand Down Expand Up @@ -3078,19 +3079,28 @@ type_new_descriptors(const type_new_ctx *ctx, PyTypeObject *type)
}
}

if (ctx->add_dict && ctx->base->tp_itemsize) {
type->tp_dictoffset = -(long)sizeof(PyObject *);
slotoffset += sizeof(PyObject *);
if (ctx->add_dict) {
if (ctx->base->tp_itemsize) {
type->tp_dictoffset = -(long)sizeof(PyObject *);
slotoffset += sizeof(PyObject *);
}
else {
assert((type->tp_flags & Py_TPFLAGS_MANAGED_DICT) == 0);
type->tp_flags |= Py_TPFLAGS_MANAGED_DICT;
}
Comment thread
ericsnowcurrently marked this conversation as resolved.
}

if (ctx->add_weak) {
assert(!ctx->base->tp_itemsize);
type->tp_weaklistoffset = slotoffset;
slotoffset += sizeof(PyObject *);
if (type->tp_flags & Py_TPFLAGS_MANAGED_DICT) {

@ericsnowcurrently ericsnowcurrently Aug 4, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ericsnowcurrently ericsnowcurrently Aug 4, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@markshannon markshannon Aug 5, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@markshannon markshannon Aug 5, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ericsnowcurrently ericsnowcurrently Aug 5, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@ericsnowcurrently ericsnowcurrently Aug 5, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 = -4 * sizeof(PyObject *);

@ericsnowcurrently ericsnowcurrently Aug 4, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@markshannon markshannon Aug 5, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ericsnowcurrently ericsnowcurrently Aug 5, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What? No SVG?

}
else {
type->tp_weaklistoffset = slotoffset;
slotoffset += sizeof(PyObject *);
}
Comment thread
ericsnowcurrently marked this conversation as resolved.
}
if (ctx->add_dict && ctx->base->tp_itemsize == 0) {
assert((type->tp_flags & Py_TPFLAGS_MANAGED_DICT) == 0);
type->tp_flags |= Py_TPFLAGS_MANAGED_DICT;
if (type->tp_flags & Py_TPFLAGS_MANAGED_DICT) {
Comment on lines -3092 to +3103

@ericsnowcurrently ericsnowcurrently Aug 4, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

type->tp_dictoffset = -slotoffset - sizeof(PyObject *)*3;
}

Expand Down Expand Up @@ -5070,11 +5080,13 @@ compatible_for_assignment(PyTypeObject* oldto, PyTypeObject* newto, const char*
!same_slots_added(newbase, oldbase))) {
goto differs;
}
/* The above does not check for managed __dicts__ */
/* The above does not check for managed __dict__ or __weakref__ */
if ((oldto->tp_flags & Py_TPFLAGS_MANAGED_DICT) ==
((newto->tp_flags & Py_TPFLAGS_MANAGED_DICT)))
{
return 1;
if (oldto->tp_weaklistoffset == newto->tp_weaklistoffset) {
return 1;
}
}
differs:
PyErr_Format(PyExc_TypeError,
Expand Down