◐ Shell
reader mode source ↗
Skip to content

bpo-44337: Improve LOAD_ATTR specialization#26759

Merged
markshannon merged 5 commits into
python:mainfrom
faster-cpython:improve-load-attr-specialization
Jun 21, 2021
Merged

bpo-44337: Improve LOAD_ATTR specialization#26759
markshannon merged 5 commits into
python:mainfrom
faster-cpython:improve-load-attr-specialization

Conversation

@markshannon

@markshannon markshannon commented Jun 16, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

We were being overly conservative by not specializing for instance attributes when the class also has an attribute.
It is OK to perform that specialization if the class attribute is not an overriding descriptor (and has an immutable class, so it will remain so).
This requires more detailed analysis of the class attribute, so this is factored out in a new function, which we can also use when we specialize STORE_ATTR.

This PR also specializes obj.__class__ using LOAD_ATTR_SLOT.

https://bugs.python.org/issue44337

@markshannon
Improve LOAD_ATTR specialization …
d8e1017 
* Specialize obj.__class__ with LOAD_ATTR_SLOT
* Specialize instance attribute lookup with attribute on class, provided attribute on class is not an overriding descriptor.
@markshannon
Add stat for how many times the unquickened instruction has executed.
65524d1
@markshannon
Turn stats off by default
d5e7d20
@markshannon

markshannon commented Jun 17, 2021

Copy link
Copy Markdown
Member Author

Performance results: https://gist.github.com/markshannon/f938b10691f081b3a6cdad54521890fb

ericsnowcurrently
ericsnowcurrently reviewed Jun 17, 2021
View reviewed changes

@ericsnowcurrently ericsnowcurrently left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

mostly LGTM

I did not notice any logical problems or bugs. I've left a number of clarifying questions and a few minor suggestions. I do not consider any of them to be blockers, especially since this PR is an improvement regardless of my comments. However, in a couple of the cases it would be worth having a bit of discussion before merging.

Specifically:

@ericsnowcurrently

ericsnowcurrently commented Jun 17, 2021

Copy link
Copy Markdown
Member

Performance results: https://gist.github.com/markshannon/f938b10691f081b3a6cdad54521890fb

Any thoughts on why some of those benchmarks are showing performance regressions?

@markshannon

markshannon commented Jun 18, 2021

Copy link
Copy Markdown
Member Author

Performance results: https://gist.github.com/markshannon/f938b10691f081b3a6cdad54521890fb

Any thoughts on why some of those benchmarks are showing performance regressions?

Not really, nor why unpack_sequence is 18% faster

ericsnowcurrently
ericsnowcurrently approved these changes Jun 18, 2021
View reviewed changes

@ericsnowcurrently ericsnowcurrently left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

LGTM

Thanks for the feedback on my questions/comments. The changes make sense and analyze_descriptor() is a definite improvement.

@markshannon markshannon reopened this Jun 21, 2021
@markshannon markshannon merged commit fb68791 into python:main Jun 21, 2021
@markshannon markshannon deleted the improve-load-attr-specialization branch January 6, 2022 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ericsnowcurrently ericsnowcurrently ericsnowcurrently approved these changes

+1 more reviewer

@bluss bluss bluss left review comments

Assignees

No one assigned

Labels

DO-NOT-MERGE skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@markshannon @ericsnowcurrently @bluss @the-knights-who-say-ni @bedevere-bot