Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-100288: Specialize class attribute loads #101379

Closed

Conversation

brandtbucher
Copy link
Member

@brandtbucher brandtbucher commented Jan 27, 2023

This handles all of the "class attr simple" failures.

Benchmarking results are "1.00x faster". It improves the LOAD_ATTR hit rate by less than 1%, and has a 32% miss rate (I suspect, due to polymorphism).

@brandtbucher brandtbucher added performance Performance or resource usage interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Jan 27, 2023
@brandtbucher brandtbucher self-assigned this Jan 27, 2023
@markshannon
Copy link
Member

If this addresses the "class attr simple" case, then according to the stats it should improve the hit rate by 4-5% (13.3% * 0.36).
So, something isn't right there. Are the stats right?

@brandtbucher
Copy link
Member Author

So, something isn't right there. Are the stats right?

I assume so. Here are the stats for the base commit and this branch.

For LOAD_ATTR, deferreds fell by 1.3%, hits rose by 0.7%, and misses rose by 0.6%.

I'm guessing that there are two reasons why the number of hits didn't increase by the full 5% you're expecting:

  • About 1/3 of the new instructions that do specialize end up missing (so we should really be expecting a ~3% increase in hits).
  • More importantly, it looks like nearly all of the cases that used to fail with "class attr simple" are now failing with "has managed dict" (line 860 in this PR). You can see that, before, we had ~600k each of "class attr simple" and "has managed dict" failures. Now, we have 0 "class attr simple" failures, and 1.1M "has managed dict" failures.

So, it appears that "class attr simple" is not so "simple" after all... 5 out of 6 instances that reach it have a __dict__!

@gvanrossum
Copy link
Member

gvanrossum commented Feb 5, 2023

When (if?) you fix the merge conflict, could you add those new opcodes to the LOAD_ATTR family and properly declare their cache/stack effects, letting the generator take care of those? I recently modernized all the existing family members (gh-101488).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting core review DO-NOT-MERGE interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants