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 Dec 1, 2021
@markshannon markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 2, 2021
@bedevere-bot
Copy link

bedevere-bot commented Dec 2, 2021

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

If you want to schedule another build, you need to add the "🔨 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

bedevere-bot commented Dec 2, 2021

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

If you want to schedule another build, you need to add the "🔨 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

@tiran tiran Dec 2, 2021

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

@markshannon markshannon Dec 2, 2021

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

@methane methane Dec 3, 2021

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

@gvanrossum gvanrossum Dec 6, 2021

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

Include/object.h Show resolved Hide resolved
@gvanrossum gvanrossum self-requested a review Dec 6, 2021
Copy link
Member

@gvanrossum gvanrossum left a comment

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

@gvanrossum gvanrossum Dec 6, 2021

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

Modules/_testcapimodule.c Outdated Show resolved Hide resolved
_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

@gvanrossum gvanrossum Dec 6, 2021

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

@markshannon markshannon Dec 7, 2021

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

@gvanrossum gvanrossum Dec 7, 2021

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. :-/

Modules/gcmodule.c Outdated Show resolved Hide resolved
Objects/object.c Outdated Show resolved Hide resolved
Objects/object.c Outdated Show resolved Hide resolved
Python/ceval.c Outdated Show resolved Hide resolved
Py_TPFLAGS_MANAGED_DICT = (1 << 4)
Py_TPFLAGS_HEAPTYPE = (1 << 9)
Copy link
Member

@gvanrossum gvanrossum Dec 6, 2021

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?

else {
if (!PyDict_CheckExact(dict)) {
Copy link
Member

@gvanrossum gvanrossum Dec 6, 2021

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

@markshannon markshannon Dec 7, 2021

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.

Python/ceval.c Outdated Show resolved Hide resolved
Copy link
Member

@gvanrossum gvanrossum left a comment

LGTM.

@markshannon markshannon merged commit 8319114 into python:main Dec 7, 2021
11 of 12 checks passed
@markshannon markshannon deleted the regular-dict-placement branch Dec 7, 2021
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