GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — bpo-45947: Place dict and values pointer at fixed (negative) offset just before GC header. by markshannon · Pull Request #29879 · python/cpython

Conversation

markshannon commented Dec 1, 2021 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

Produces a solid 1% speedup
See faster-cpython/ideas#80 for full discussion.

https://bugs.python.org/issue45947

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

markshannon added 12 commits

August 5, 2021 12:06


          Remove unsafe _PyObject_GC_Calloc function.

ee59772


          Place __dict__ immediately before GC header for variable sized object…

72b71cc

…s and plain Python objects. Work in progress.


          Restore documented behavior of tp_dictoffset.

cd22dac


          Merge branch 'main' into regular-dict-placement

4c83f77


          Fix up lazy dict creation logic to use managed dict pointers.

8a8593c


          Manage values pointer, placing them directly before managed dict poin…

34b5cea

…ters.


          Refactor a bit.

e8c74ab


          Fix specialization of managed values.

1bf13b0


          Convert hint-based load/store attr specialization target managed dict…

a025dfb

… classes.


          Specialize LOAD_METHOD for managed dict objects.

5a012a8


          Merge branch 'main' into regular-dict-placement

e7734b8


          Use newer API internally.

14d41ab

markshannon requested review from methane and pablogsal as code owners

December 1, 2021 14:04

the-knights-who-say-ni added the CLA signed label

Dec 1, 2021

bedevere-bot added the awaiting core review label

Dec 1, 2021

markshannon added 2 commits

December 1, 2021 14:06


          Add NEWS.

123171a


          Use inline functions instead of magic constants.

48d6a58

markshannon requested a review from a team as a code owner

December 1, 2021 16:59


          Remove unsafe _PyObject_GC_Malloc() function.

79e61bf

markshannon force-pushed the regular-dict-placement branch from a76861c to 79e61bf Compare

December 1, 2021 17:04

markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label

Dec 2, 2021

bedevere-bot commented Dec 2, 2021

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label

Dec 2, 2021


          Remove invalid assert.

ce0f65b

markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label

Dec 2, 2021

bedevere-bot commented Dec 2, 2021

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label

Dec 2, 2021

tiran reviewed

Dec 2, 2021

View reviewed changes

Comment thread

Include/cpython/objimpl.h

		# define _PyGC_FINALIZED(o) PyObject_GC_IsFinalized(o)
		#endif

		PyAPI_FUNC(PyObject *) _PyObject_GC_Malloc(size_t size);

tiran Dec 2, 2021

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

markshannon Dec 2, 2021

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tiran reacted with thumbs up emoji

All reactions

👍 1 reaction

methane Dec 3, 2021

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

gvanrossum Dec 6, 2021

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Include/object.h Resolved

Uh oh!

There was an error while loading. Please reload this page.


          Add comment explaning use of Py_TPFLAGS_MANAGED_DICT.

98ddaed

gvanrossum self-requested a review

December 6, 2021 18:10

gvanrossum approved these changes

Dec 6, 2021

View reviewed changes

gvanrossum left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Include/cpython/objimpl.h

		# define _PyGC_FINALIZED(o) PyObject_GC_IsFinalized(o)
		#endif

		PyAPI_FUNC(PyObject *) _PyObject_GC_Malloc(size_t size);

gvanrossum Dec 6, 2021

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/_testcapimodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Modules/gcmodule.c

		_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

gvanrossum Dec 6, 2021

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

markshannon Dec 7, 2021

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

gvanrossum Dec 7, 2021

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/gcmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Objects/object.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Objects/object.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Python/ceval.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Tools/gdb/libpython.py Outdated

Comment on lines 86 to 87

		Py_TPFLAGS_MANAGED_DICT = (1 << 4)
		Py_TPFLAGS_HEAPTYPE = (1 << 9)

gvanrossum Dec 6, 2021

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Python/specialize.c

Comment on lines +650 to +651

		else {
		if (!PyDict_CheckExact(dict)) {

gvanrossum Dec 6, 2021

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

markshannon Dec 7, 2021

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gvanrossum reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

Python/ceval.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot added awaiting merge and removed awaiting core review labels

Dec 6, 2021

markshannon added 4 commits

December 7, 2021 11:55


          Use inline function, not magic constant.

0f376b5


          Tidy up struct layout a bit.

d724812


          Tidy up gdb/libpython.py.

9435bae


          Fix whitespace.

302f46f

gvanrossum approved these changes

Dec 7, 2021

View reviewed changes

gvanrossum left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

markshannon merged commit 8319114 into python:main

Dec 7, 2021

markshannon deleted the regular-dict-placement branch

December 7, 2021 16:02

bedevere-bot removed the awaiting merge label

Dec 7, 2021

corona10 mentioned this pull request

Feb 9, 2023

gh-101430: Update tracemalloc to handle presize properly. #101745

Merged

jackyk02 mentioned this pull request

Oct 21, 2023

Fix Segfault for Python 3.11 lf-lang/reactor-c#293

Merged

jackyk02 mentioned this pull request

Nov 9, 2023

[Python 3.11] Fix Segfault lf-lang/reactor-c#304

Closed

malor added a commit to malor/cpython-lldb that referenced this pull request

Nov 17, 2024


          cpython3.11: fix pretty-printing of UserDict, UserString, UserList

47deb61

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 mentioned this pull request

Nov 17, 2024

cpython3.11: fix pretty-printing of UserDict, UserString, UserList malor/cpython-lldb#74

Merged

malor added a commit to malor/cpython-lldb that referenced this pull request

Nov 18, 2024


          cpython3.11: fix pretty-printing of UserDict, UserString, UserList

ca9fe33

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.

Reviewers

gvanrossum

gvanrossum approved these changes

methane

Awaiting requested review from methane

methane is a code owner

pablogsal

Awaiting requested review from pablogsal

pablogsal is a code owner

+1 more reviewer

tiran

tiran left review comments

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

markshannon commented Dec 1, 2021 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-bot commented Dec 2, 2021

Uh oh!

bedevere-bot commented Dec 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

markshannon commented Dec 1, 2021 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.