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 — GH-93533: Shrink the `LOAD_ATTR` caches by brandtbucher · Pull Request #103014 · python/cpython

Conversation

@brandtbucher

@brandtbucher brandtbucher commented Mar 24, 2023 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

This adds a fixed array of cached methods to the PyTypeObject struct. This approach saves memory if there are ~23x more LOAD_ATTR sites than there are types.

A size of 16 was chosen because:

  • A size of 8 is 1% slower and has a 81.0% LOAD_ATTR hit rate on pyperformance, with 32.5% of specialization failures due to a too-small cache.
  • A size of 16 is 0% faster and has a 82.2% LOAD_ATTR hit rate on pyperformance, with 8.7% of specialization failures due to a too-small cache.
  • A size of 32 is 0% faster and has a 82.6% LOAD_ATTR hit rate on pyperformance, with 3.3% of specialization failures due to a too-small cache.

A flexible buffer was also considered, but that was 1% slower, likely due to the additional indirection and management of the buffer.

  • Issue: The inline cache for LOAD_ATTR is too large. #93533

Sorry, something went wrong.

Uh oh!

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

All reactions
brandtbucher added 6 commits March 22, 2023 18:29
@brandtbucher
Stick a small, resizable cache on all types
c87e8bd
@brandtbucher
Try a bigger starting cache size (8 instead of 1)
0e21f47
@brandtbucher
Try a fixed cache size
bde6bec
@brandtbucher
Bump the size to 16
212046c
@brandtbucher
Catch up with main
1ee7d47
@brandtbucher
Blurb add
cdc3189
@brandtbucher brandtbucher added the performance Performance or resource usage label Mar 24, 2023
@brandtbucher brandtbucher self-assigned this Mar 24, 2023
@brandtbucher
brandtbucher requested a review from markshannon as a code owner March 24, 2023 20:18
@bedevere-bot bedevere-bot mentioned this pull request Mar 24, 2023
The inline cache for LOAD_ATTR is too large. #93533
Open
@bedevere-bot bedevere-bot added the awaiting core review label Mar 24, 2023
@brandtbucher brandtbucher changed the title GH-93533: Shrink the LOAD_ATTR caches. GH-93533: Shrink the LOAD_ATTR caches Mar 24, 2023
@TeamSpen210

TeamSpen210 commented Mar 24, 2023

Copy link
Copy Markdown

So if I'm understanding this correctly, for each class only 16 methods/attributes can be specialised, with everything after just failing? If so 16 seems fairly small, especially for larger libraries/applications. Something like ndarray or flask.Flask for instance has 50+ methods. I could imagine a larger application using different sets of methods in different areas, filling up the cache. That wouldn't be the sort of thing showing up on benchmarks.

All reactions

Sorry, something went wrong.

Uh oh!

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

@brandtbucher

brandtbucher commented Mar 24, 2023

Copy link
Copy Markdown
Member Author

So if I'm understanding this correctly, for each class only 16 methods/attributes can be specialised, with everything after just failing?

Correct (not including instance attributes). I definitely prefer a growable cache to this approach, except that the numbers we have are slower.

I'm also open to using a larger number of entries, like 32 or 64, if others feel similarly.

All reactions

Sorry, something went wrong.

Uh oh!

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

@brandtbucher

brandtbucher commented Mar 24, 2023

Copy link
Copy Markdown
Member Author

I'm also not sure if this causes problems for interpreter isolation. Maybe putting this sort of state on static built-in types is a bad idea, whether it's resizable or not.

CC @ericsnowcurrently

All reactions

Sorry, something went wrong.

Uh oh!

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

@hugovk
Merge branch 'main' into type-cache-fixed
6934d55
@hugovk

hugovk commented Mar 25, 2023

Copy link
Copy Markdown
Member

(Docs now passing now #103019 is merged, and updating this with main. Thanks for flagging!)

All reactions

Sorry, something went wrong.

Uh oh!

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

@markshannon

markshannon commented Mar 25, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

From a correctness point of view, adding the cache to static classes should be fine. All the attributes of superclasses of static classes must also be static.
If they aren't that's a bug, and this isn't the place to fix it.
No harm in adding some asserts, though.

However, we would like static classes to be const so that they can be properly shared, which would mean that the cache would need to be pre-populated with all attributes of the class and all its superclasses.
The caches would be a bit big, but not that bad: len(object.__dict__ | int.__dict__) == 74; much smaller if we exclude special methods.

All reactions

Sorry, something went wrong.

Uh oh!

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

@brandtbucher

brandtbucher commented Mar 27, 2023

Copy link
Copy Markdown
Member Author

Closing because of the various issues outlined above.

All reactions

Sorry, something went wrong.

Uh oh!

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

@brandtbucher brandtbucher closed this Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

Uh oh!

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

Assignees

@brandtbucher brandtbucher

Labels

awaiting core review performance Performance or resource usage

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

@brandtbucher @TeamSpen210 @hugovk @markshannon @bedevere-bot
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.