NotificationsYou must be signed in to change notification settings
Fork32.1k
Star67.3k

GH-89987: Shrink the`BINARY_SUBSCR` caches#103022

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

Jump to bottom

Merged

brandtbucher merged 6 commits intopython:mainfrombrandtbucher:shrink-binary-subscr-more

Mar 29, 2023

Merged

GH-89987: Shrink the`BINARY_SUBSCR` caches#103022

brandtbucher merged 6 commits intopython:mainfrombrandtbucher:shrink-binary-subscr-more

Mar 29, 2023

Conversation

Copy link

Member

brandtbucher commentedMar 25, 2023•
edited by bedevere-bot
Loading

BINARY_SUBSCR has 3 cache entries that are only used byBINARY_SUBSCR_GETITEM (a type version and a function version). However, we don't really need to store all of this information per call-site... we can just keep the function version on the type (next to the cached method) and clear the cached method whenever the type is modified.

Besides the savings in code size, this has a couple of other minor benefits:

AnyBINARY_SUBSCR_GETITEM site can work withany heap type defining__getitem__, provided it's already been cached at least once somewhere.
We can specialize for more than 65k function versions.

Performance isneutral.

Issue:Remove C stack use by specializing BINARY_SUBSCR, STORE_SUBSCR, LOAD_ATTR, and STORE_ATTR #89987

brandtbucher added4 commits

March 23, 2023 21:38

Remove tp_version_tag from BINARY_SUBSCR caches

b174015

Get rid of the remaining cache entry

8b9f269

Minor cleanup

8c85b34

blurb add

58dc610

brandtbucher added the performancePerformance or resource usage label

Mar 25, 2023

brandtbucher requested a review frommarkshannon as acode owner

March 25, 2023 00:13

brandtbucher self-assigned this

Mar 25, 2023

bedevere-bot mentioned this pull request

Mar 25, 2023

Remove C stack use by specializing BINARY_SUBSCR, STORE_SUBSCR, LOAD_ATTR, and STORE_ATTR#89987

Closed

bedevere-bot added the awaiting core review label

Mar 25, 2023

brandtbucher added2 commits

March 27, 2023 09:03

Merge branch 'main' into shrink-binary-subscr-more

5d8e57c

Clarify the _specialization_cache contract

0de0eb7

Copy link

Member

markshannon commentedMar 28, 2023•
edited
Loading

I'm a bit concerned aboutABA errors.
For example:

Site X is specialized for some version of class C and C.getitem.
Then C.getitem is modified
Site Y is specialized, setting the cached version to the new function.
Now site X runs again.

Is this OK?

I think it is, but it requires a different contract between the specializer, the specialized code and the class's cache, than we had before.

This contract will need to be clearly documented.
Does this seem correct:

If the_specialization_cache.getitem field is non-NULL, then the value it holds is whatPyType_Lookup(cls, "__getitem__") would return.
If_specialization_cache.getitem is NULL, thegetitem_version field is meaningless.
If the_specialization_cache.getitem field is non-NULLand_specialization_cache.getitem_version == _specialization_cache.getitem->func_version then_specialization_cache.getitem is a Python function that can be called with two positional arguments and no keyword arguments, and has neither*args nor**kwargs.