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: Skip extra work when failing to specialize LOAD_ATTR #101354

Merged
merged 2 commits into from
Jan 31, 2023

Conversation

brandtbucher
Copy link
Member

@brandtbucher brandtbucher commented Jan 26, 2023

I think this was missed in #100753.

This removes a dict lookup, a dict version, and a cache write from one case where we fail to specialize method loads (due to the existence of a non-managed instance __dict__).

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

@markshannon markshannon left a comment

Choose a reason for hiding this comment

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

Thanks. Could you also move the body of the if statement into the switch case.

@@ -1093,7 +1093,7 @@ PyObject *descr, DescriptorClassification kind)
}
}
}
if (dictkind == MANAGED_VALUES || dictkind == OFFSET_DICT) {
if (dictkind == MANAGED_VALUES) {
Copy link
Member

Choose a reason for hiding this comment

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

Since there is only the one case now, it would make the code easier to follow if this were moved into the case MANAGED_VALUES: below.

@brandtbucher
Copy link
Member Author

Can I just get rid of the switch entirely? Each branch in the above if/else logic maps to exactly one case, so we might as well just lift the bodies of the cases up there.

@markshannon
Copy link
Member

Can I just get rid of the switch entirely? Each branch in the above if/else logic maps to exactly one case, so we might as well just lift the bodies of the cases up there.

Sounds good to me. We can then get rid of ObjectDictKind as well.

@markshannon
Copy link
Member

Looks good.

@markshannon markshannon self-requested a review January 31, 2023 12:34
@brandtbucher
Copy link
Member Author

Confirmed locally that this doesn't change the stats.

@brandtbucher brandtbucher merged commit 76efcb4 into python:main Jan 31, 2023
carljm added a commit to carljm/cpython that referenced this pull request Jan 31, 2023
* main:
  pythonGH-100288: Skip extra work when failing to specialize LOAD_ATTR (pythonGH-101354)
  pythongh-101409: Improve generated clinic code for self type checks (python#101411)
  pythongh-98831: rewrite BEFORE_ASYNC_WITH and END_ASYNC_FOR in the instruction definition DSL (python#101458)
  pythongh-101469: Optimise get_io_state() by using _PyModule_GetState() (pythonGH-101470)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants