Skip to content

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

Merged
brandtbucher merged 2 commits intopython:mainfrom
brandtbucher:remove-old-branch
Jan 31, 2023
Merged

GH-100288: Skip extra work when failing to specialize LOAD_ATTR#101354
brandtbucher merged 2 commits intopython:mainfrom
brandtbucher:remove-old-branch

Conversation

@brandtbucher
Copy link
Member

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

@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
@bedevere-bot bedevere-bot mentioned this pull request Jan 26, 2023
Closed
2 tasks
markshannon
markshannon reviewed Jan 27, 2023
View reviewed changes
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.

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

@markshannon markshannon Jan 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
Member Author

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.

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

brandtbucher commented Jan 31, 2023

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
@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 free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@markshannon markshannon markshannon approved these changes

Assignees

@brandtbucher brandtbucher

Labels

interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@brandtbucher @markshannon @bedevere-bot