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

bpo-45947: Place dict and values pointer at fixed (negative) offset just before GC header. #29879

Merged
merged 21 commits into from
Dec 7, 2021

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Dec 1, 2021

@markshannon markshannon requested a review from a team as a code owner December 1, 2021 16:59
@markshannon markshannon force-pushed the regular-dict-placement branch from a76861c to 79e61bf Compare December 1, 2021 17:04
@markshannon markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 2, 2021
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @markshannon for commit 79e61bf 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 2, 2021
@markshannon markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 2, 2021
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @markshannon for commit ce0f65b 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 2, 2021
@@ -90,9 +90,6 @@ PyAPI_FUNC(int) PyObject_IS_GC(PyObject *obj);
# define _PyGC_FINALIZED(o) PyObject_GC_IsFinalized(o)
#endif

PyAPI_FUNC(PyObject *) _PyObject_GC_Malloc(size_t size);
Copy link
Member

Choose a reason for hiding this comment

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

_PyObject_GC_Malloc is part of stable ABI. AFAIK the function cannot be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not part of the stable ABI. It starts with an underscore.
https://www.python.org/dev/peps/pep-0384/#excluded-functions

Copy link
Member

Choose a reason for hiding this comment

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

Misc/stable_abi.txt define it as stable ABI.

function _PyObject_GC_Malloc
    added 3.2
    abi_only

On the other hand, no public macro in Python/C API use it. So I doubt it is actually stable abi.

As far as this repo, only one package in top4000 packages uses it.
https://github.com/hpyproject/top4000-pypi-packages/search?q=_PyObject_GC_Malloc

Copy link
Member

Choose a reason for hiding this comment

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

I sent an email to python-dev asking for clarification of the status of this functions and others that start with _ but are listed in Misc/stable_abi.txt. (It is not listed in Doc/data/stable_abi.dat.)

@gvanrossum gvanrossum self-requested a review December 6, 2021 18:10
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Here's a first pass at a review, mostly nits. I think I'm following everything, though I'm not necessarily fully aware of the whole big picture.

@@ -90,9 +90,6 @@ PyAPI_FUNC(int) PyObject_IS_GC(PyObject *obj);
# define _PyGC_FINALIZED(o) PyObject_GC_IsFinalized(o)
#endif

PyAPI_FUNC(PyObject *) _PyObject_GC_Malloc(size_t size);
Copy link
Member

Choose a reason for hiding this comment

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

I sent an email to python-dev asking for clarification of the status of this functions and others that start with _ but are listed in Misc/stable_abi.txt. (It is not listed in Doc/data/stable_abi.dat.)

_PyObject_GC_Link(PyObject *op)
{
PyGC_Head *g = AS_GC(op);
assert(((uintptr_t)g & (sizeof(uintptr_t)-1)) == 0); // g must be correctly aligned
Copy link
Member

Choose a reason for hiding this comment

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

Wow, you can use & on pointers. I didn't know that. :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Only after you've cast it to an int. You can do anything with a cast 🙂
NULL is usually just 0 cast to a pointer, after all.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, somehow I thought uintptr_t was a pointer. :-/

Comment on lines 86 to 87
Py_TPFLAGS_MANAGED_DICT = (1 << 4)
Py_TPFLAGS_HEAPTYPE = (1 << 9)
Copy link
Member

Choose a reason for hiding this comment

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

Please realign these like the rest?

Comment on lines +650 to +651
else {
if (!PyDict_CheckExact(dict)) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing that if we have a class of objects and only some of those have a materialized __dict__ that will wreak havoc with our specialization, right? Because it will flop-flop as it encounters instances with and without __dict__. In this case we have to hope that the vast majority don't have a __dict__.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes.
The specialized case is so much faster though, that it might not be any worse than not specializing at all, even in that case.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

LGTM.

@markshannon markshannon merged commit 8319114 into python:main Dec 7, 2021
@markshannon markshannon deleted the regular-dict-placement branch December 7, 2021 16:02
malor added a commit to malor/cpython-lldb that referenced this pull request Nov 17, 2024
python/cpython#29879 changed the layout of
user classes, and the code is no longer able to find the location
of __dict__. Do a quick fix for now, as we don't support pretty-printing
user defined classes.
malor added a commit to malor/cpython-lldb that referenced this pull request Nov 18, 2024
python/cpython#29879 changed the layout of
user classes, and the code is no longer able to find the location
of __dict__. Do a quick fix for now, as we don't support pretty-printing
user defined classes.
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.

6 participants