Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

GH-93533: Shrink theLOAD_ATTR caches#103014

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed

Conversation

brandtbucher
Copy link
Member

@brandtbucherbrandtbucher commentedMar 24, 2023
edited by bedevere-bot
Loading

This adds a fixed array of cached methods to thePyTypeObject struct. This approach saves memory if there are ~23x moreLOAD_ATTR sites than there are types.

A size of 16 was chosen because:

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

@brandtbucherbrandtbucher added the performancePerformance or resource usage labelMar 24, 2023
@brandtbucherbrandtbucher self-assigned thisMar 24, 2023
@brandtbucherbrandtbucher changed the titleGH-93533: Shrink theLOAD_ATTR caches.GH-93533: Shrink theLOAD_ATTR cachesMar 24, 2023
@TeamSpen210
Copy link

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 likendarray orflask.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.

@brandtbucher
Copy link
MemberAuthor

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.

@brandtbucher
Copy link
MemberAuthor

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

@hugovk
Copy link
Member

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

@markshannon
Copy link
Member

markshannon commentedMar 25, 2023
edited
Loading

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

@brandtbucher
Copy link
MemberAuthor

Closing because of the various issues outlined above.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@markshannonmarkshannonAwaiting requested review from markshannonmarkshannon is a code owner

Assignees

@brandtbucherbrandtbucher

Labels
awaiting core reviewperformancePerformance or resource usage
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@brandtbucher@TeamSpen210@hugovk@markshannon@bedevere-bot

[8]ページ先頭

©2009-2025 Movatter.jp