Skip to content
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.

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

GH-115776: Embed the values array into the object, for "normal" Python objects. #116115

Merged
merged 35 commits into from Apr 2, 2024

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Feb 29, 2024

A "normal" Python object is one that:

  • Doesn't inherit from a builtin class
  • Does not have __slots__

@markshannon markshannon requested a review from DinoV March 1, 2024 12:05
@markshannon
Copy link
Member Author

@DinoV any ideas what might be causing the failures on free-threading builds?

Python/gc.c Show resolved Hide resolved
Objects/dictobject.c Show resolved Hide resolved
@markshannon markshannon marked this pull request as ready for review March 26, 2024 17:14
Copy link
Contributor

@DinoV DinoV left a comment

Choose a reason for hiding this comment

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

Looks awesome, just a few nits!

Objects/dictobject.c Outdated Show resolved Hide resolved
Include/internal/pycore_object.h Show resolved Hide resolved
Objects/dictobject.c Outdated Show resolved Hide resolved
@markshannon markshannon merged commit c32dc47 into python:main Apr 2, 2024
61 checks passed
if (_PyType_HasFeature(typeobj, Py_TPFLAGS_HEAPTYPE)) {
Py_INCREF(typeobj);
}
assert(_PyType_HasFeature(typeobj, Py_TPFLAGS_HEAPTYPE) || _Py_IsImmortal(typeobj));
Copy link
Contributor

Choose a reason for hiding this comment

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

I now hit this assertion in Cython modules. The (static) extension type that we use for generators has tp_alloc = PyType_GenericAlloc and calls it in tp_new():

#6  0x00007ffff7cc2e96 in __GI___assert_fail (assertion=assertion@entry=0x5555559db4e0 "_PyType_HasFeature(typeobj, Py_TPFLAGS_HEAPTYPE) || _Py_IsImmortal(typeobj)", file=file@entry=0x5555559db468 "./Include/internal/pycore_object.h", line=line@entry=268, function=function@entry=0x555555a04ad8 <__PRETTY_FUNCTION__.140> "_PyObject_Init") at ./assert/assert.c:101
#7  0x0000555555774e33 in _PyObject_Init (typeobj=0x7ffff6c26d40 <__pyx_type_8buildenv___pyx_scope_struct__genexpr>, op=0x7ffff4f1e210) at ./Include/internal/pycore_object.h:268
#8  0x000055555577cec3 in _PyObject_Init (typeobj=<optimized out>, op=<optimized out>) at ./Include/object.h:430
#9  _PyType_AllocNoTrack (type=type@entry=0x7ffff6c26d40 <__pyx_type_8buildenv___pyx_scope_struct__genexpr>, nitems=0) at Objects/typeobject.c:1908
#10 0x000055555577cf09 in PyType_GenericAlloc (type=0x7ffff6c26d40 <__pyx_type_8buildenv___pyx_scope_struct__genexpr>, nitems=<optimized out>) at Objects/typeobject.c:1922
#11 0x00007ffff6c08100 in __pyx_tp_new_8buildenv___pyx_scope_struct__genexpr (t=<optimized out>, a=<optimized out>, k=k@entry=0x0) at buildenv.c:2954

Is this assertion simply wrong, or is there anything I can do to avoid it? The "immortal objects" API doesn't seem to be intended for public use.

Copy link
Member Author

Choose a reason for hiding this comment

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

#19474 sets the reference count to 1 for statically allocated objects unless Py_BUILD_CORE is defined.

As a workaround, I'd suggest making these classes immortal and making them immutable if you can.

The "immortal objects" API doesn't seem to be intended for public use.

That does seem to be the case. I don't know why. Want to open an issue for that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why this new assertion exists? The previous if condition came from a conservative change that intended to avoid refcounting static types but wanted to make sure (heap) type objects are correctly kept alive as long as their objects. Now the code requires all non-heap types to be declared immortal. That seems a rather heavy change in requirements.

Copy link
Member Author

Choose a reason for hiding this comment

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

All non-heap types are immortal. Requiring that they are declared as such seems reasonable, and is probably necessary for memory safety.

I don't think you should need to do so explicitly though, PyObject_HEAD_INIT should do it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Does #117673 fix the problem for you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants