GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-45340: Don't create object dictionaries unless actually needed by markshannon · Pull Request #28802 · python/cpython

Conversation

@markshannon

@markshannon markshannon commented Oct 7, 2021 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

A "normal" Python objects is conceptually just a pair of pointers, one to the class, and one to the dictionary.

With shared keys, the dictionary is redundant as it is no more than a pair of pointers, one to the keys and one to the values.

By adding a pointer to the values to the object and fetching the keys via the class, we can avoid creating a dictionary for many objects.

See faster-cpython/ideas#72 for more details.

About 1% faster, which is nice, but the main benefit is the reduced memory consumption.

https://bugs.python.org/issue45340

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
markshannon added 17 commits September 22, 2021 14:54
@markshannon
Add insertion order bit-vector to dict values to allow dicts to share…
8214006 
… keys more freely.
@markshannon
Add NEWS
45c9814
@markshannon
Never change types' cached keys. It could invalidate inline attribute…
c3de09e 
… objects.
@markshannon
Lazily create object dictionaries. Work in progress.
58dd2a0
@markshannon
Restore specialization for inline attributes.
769183a
@markshannon
Turn on stats
1e6cd1f
@markshannon
Fix specialization of LOAD/STORE_ATTR.
a8fbb34
@markshannon
Turn off stats.
37d8cc6
@markshannon
Don't update shared keys version for deletion of value.
0e022f9
@markshannon
Merge main into inline-attributes-with-opt
ddd2e2c
@markshannon
Fix refrence leak.
e979575
@markshannon
Change 'InlineAttribute' to 'InstanceAttribute' as they are not inlin…
42361ad 
…e (yet)
@markshannon
Merge branch 'main' into inline-attributes-with-opt
e9b09d5
@markshannon
Update gdb support to handle instance values.
70dc34f
@markshannon
Rename SPLIT_KEYS opcodes to INSTANCE_VALUE.
022f2c5
@markshannon
Tidy test_gc
4ec5f11
@markshannon
Don't lose errors.
1818a5a
@markshannon
markshannon requested a review from methane as a code owner October 7, 2021 14:31
@bedevere-bot bedevere-bot added the awaiting core review label Oct 7, 2021
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Oct 7, 2021
@markshannon

markshannon commented Oct 7, 2021

Copy link
Copy Markdown
Member Author

Needs NEWS and some additional documentation.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
markshannon added 3 commits October 8, 2021 00:25
@markshannon
Make sure order is updated when deleting from values array.
bfc7be0
@markshannon
Update test_dict.
62a3a59
@markshannon
Add NEWS
7978220
Fidget-Spinner
Fidget-Spinner reviewed Oct 8, 2021
View reviewed changes

@Fidget-Spinner Fidget-Spinner 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.

Some comments about LOAD_METHOD.

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 +943 to 945
if (*owner_dictptr) {
SPECIALIZATION_FAIL(LOAD_METHOD, SPEC_FAIL_IS_ATTR);
goto fail;

@Fidget-Spinner Fidget-Spinner Oct 8, 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.

Does this reduce the specialization rates by much?

Sorry, something went wrong.

Uh oh!

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

@markshannon markshannon Oct 9, 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.

I didn't check. Overall performance is up a bit, so that's good enough for this PR 🙂

We will need to recheck the specialization rates and kinds of failures for LOAD_ATTR, STORE_ATTR and LOAD_METHOD. From my superficial scan, it appears that the kinds of failures are more clustered, which bodes well.

But changing all that in this PR would just confuse matters.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/ceval.c
assert(self_cls->tp_dictoffset > 0);
assert(self_cls->tp_inline_values_offset > 0);
PyDictObject *dict = *(PyDictObject **)(((char *)self) + self_cls->tp_dictoffset);
DEOPT_IF(dict != NULL, LOAD_METHOD);

@Fidget-Spinner Fidget-Spinner Oct 8, 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.

Wait is this due to the new scheme that objects shouldn't have dict unless really required?

Sorry, something went wrong.

Uh oh!

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

@markshannon markshannon Oct 9, 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, it is. We assume that having no __dict__ is the most common case, so it isn't worth bothering to check for the shared key dict case.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/specialize.c
PyObject *descr = NULL;
DesciptorClassification kind = 0;
kind = analyze_descriptor(owner_cls, name, &descr, 0);
// Store the version right away, in case it's modified halfway through.

@Fidget-Spinner Fidget-Spinner Oct 8, 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.

Out of curiosity: is there a reason why we don't need to do this immediately anymore? Are the new dictionary operations atomic/don't call any _PyType_Lookup?

Sorry, something went wrong.

Uh oh!

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

@markshannon markshannon Oct 9, 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.

We used to call _Py_dict_lookup, which could do anything, now we call _PyDictKeys_StringLookup which cannot.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@markshannon markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 9, 2021
@bedevere-bot

bedevere-bot commented Oct 9, 2021

Copy link
Copy Markdown

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

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 bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 9, 2021
methane
methane reviewed Oct 12, 2021
View reviewed changes
Comment thread Include/cpython/object.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
@markshannon
Merge branch 'main' into inline-attributes-with-opt
b25b1d9
markshannon added 4 commits October 12, 2021 09:45
@markshannon
Merge branch 'main' into inline-attributes-with-opt
7b2efa4
@markshannon
Make sure that instance dicts are GC tracked when necessary.
459e29e
@markshannon
Support older versions of Python in gdb.
83b90c5
@markshannon
Rename function and move to internal header.
8c894f3
@markshannon markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 12, 2021
@bedevere-bot

bedevere-bot commented Oct 12, 2021

Copy link
Copy Markdown

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

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 bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 12, 2021
@markshannon

markshannon commented Oct 13, 2021

Copy link
Copy Markdown
Member Author

The buildbot failures are:

  • buildbot/AMD64 Windows10 PR: Not enough memory on that machine. Lots of MemoryErrors.
  • buildbot/s390x RHEL7 LTO PR: Segfault due to stack overflow. Pre-existing issue (although one that needs fixing if we are going to support s390)
  • buildbot/x86 Gentoo Installed with X PR: Tk issue
  • buildbot/x86 Gentoo Non-Debug with X PR: Tk issue
All reactions

Sorry, something went wrong.

Uh oh!

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

markshannon commented Oct 13, 2021

Copy link
Copy Markdown
Member Author

Benchmark of final version: 1% faster
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@markshannon
markshannon merged commit a8b9350 into python:main Oct 13, 2021
@bedevere-bot bedevere-bot removed the awaiting core review label Oct 13, 2021
Fidget-Spinner
Fidget-Spinner reviewed Oct 22, 2021
View reviewed changes
Comment thread Python/specialize.c
if (owner_cls->tp_inline_values_offset) {
PyObject **owner_dictptr = _PyObject_DictPointer(owner);
assert(owner_dictptr);
if (*owner_dictptr) {

@Fidget-Spinner Fidget-Spinner Oct 22, 2021 •
edited
Loading

Uh oh!

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

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.

@markshannon I'm somewhat confused why anything with a dict pointer is now an attribute, even without looking into said dict. Is there a reason elsewhere (or is this so that we don't need another opcode for the other dict form)?

Sorry, something went wrong.

Uh oh!

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

@markshannon markshannon Oct 22, 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.

Anything will a dict pointer will be a miss for LOAD_INSTANCE_VALUE, so I'm assuming it isn't worth optimizing.
Maybe we should attempt to optimize this using LOAD_ATTR_WITH_HINT at this point?

Sorry, something went wrong.

Uh oh!

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

@Fidget-Spinner Fidget-Spinner Oct 22, 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.

This code seems to be for LOAD_METHOD_CACHED. I'll try to play around with it and see if the dictionary matters (versus LOAD_ATTR_INSTANCE_VALUE/LOAD_ATTR_WITH_HINT and such.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@markshannon markshannon mentioned this pull request Dec 2, 2021
Compact layout of Plain Python Objects faster-cpython/ideas#72
Closed
@geryogam geryogam mentioned this pull request May 4, 2022
bpo-43857: Improve the AttributeError message when deleting a missing attribute #25424
Merged
@markshannon markshannon mentioned this pull request Nov 29, 2022
Lazily create dictionaries for plain Python objects #89503
Closed
@gertjanvanzwieten gertjanvanzwieten mentioned this pull request Dec 2, 2022
serialized evalf evalf/nutils#734
Merged
@markshannon
markshannon deleted the inline-attributes-with-opt branch September 26, 2023 12:47
@markshannon markshannon mentioned this pull request Feb 21, 2024
Use static analysis to assist inlining values and compact object layout. #115775
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@methane methane methane left review comments
@Fidget-Spinner Fidget-Spinner Fidget-Spinner left review comments

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.

5 participants

@markshannon @bedevere-bot @methane @Fidget-Spinner @the-knights-who-say-ni
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 01402503 — 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.