Skip to content

GH-100288: Skip extra work when failing to specialize LOAD_ATTR#101354

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.

Sign up for GitHub

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

Jump to bottom
Merged
brandtbucher merged 2 commits into from
Jan 31, 2023
Merged

GH-100288: Skip extra work when failing to specialize LOAD_ATTR#101354

brandtbucher merged 2 commits into from
Jan 31, 2023

Conversation

@brandtbucher
Copy link
Member

@brandtbucherbrandtbucher commented Jan 26, 2023•
edited by bedevere-bot
Loading

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__).

@brandtbucherbrandtbucher added performance Performance or resource usage skip news interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Jan 26, 2023
@brandtbucherbrandtbucher self-assigned this Jan 26, 2023
@bedevere-botbedevere-bot mentioned this pull request Jan 26, 2023
Closed
2 tasks
markshannon
markshannon reviewed Jan 27, 2023
View reviewed changes
Copy link
Member

@markshannonmarkshannon 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.

Python/specialize.c Outdated
}
}
if (dictkind==MANAGED_VALUES||dictkind==OFFSET_DICT){
if (dictkind==MANAGED_VALUES){
Copy link
Member

@markshannonmarkshannonJan 27, 2023

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
MemberAuthor

brandtbucher commented Jan 27, 2023

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

markshannon commented Jan 29, 2023

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

markshannon commented Jan 31, 2023

Looks good.

@markshannonmarkshannon self-requested a review January 31, 2023 12:34
@brandtbucher
Copy link
MemberAuthor

brandtbucher commented Jan 31, 2023

Confirmed locally that this doesn't change the stats.

@brandtbucherbrandtbucher merged commit 76efcb4 into python:mainJan 31, 2023
carljm added a commit to carljm/cpython that referenced this pull request Jan 31, 2023
@carljm
Merge branch 'main' into inlinecomp2
db208d5
* 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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@markshannonmarkshannonmarkshannon approved these changes

Assignees

@brandtbucherbrandtbucher

Labels

interpreter-core(Objects, Python, Grammar, and Parser dirs)performancePerformance or resource usageskip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@brandtbucher@markshannon@bedevere-bot