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-93533: Shrink the LOAD_METHOD cache by one codeunit. #93537

Merged
merged 1 commit into from Jun 7, 2022

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Jun 6, 2022

#93533

Remove the dict_offset cache entry.
See faster-cpython/ideas#396
The LOAD_METHOD_WITH_DICT is largely ineffective for objects with managed dictionaries, so we don't specialize for that case.

@markshannon markshannon requested a review from Fidget-Spinner Jun 6, 2022
@markshannon markshannon changed the title Shrink the LOAD_METHOD cache by one codeunit. GH-93533: Shrink the LOAD_METHOD cache by one codeunit. Jun 6, 2022
Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

LGTM. Anyways this can be specialized with the future LOAD_ATTR anyways, it just won't have the bound method optimization.

@markshannon
Copy link
Member Author

@markshannon markshannon commented Jun 6, 2022

What won't have the bound method optimization, and why?

@Fidget-Spinner
Copy link
Member

@Fidget-Spinner Fidget-Spinner commented Jun 6, 2022

What won't have the bound method optimization, and why?

Descriptors with MANAGED_DICT (if we let LOAD_ATTR specialize it). But as you said it wasnt that effective anyways.

Edit:
I also forgot that it can just fall back on the generic LOAD_METHOD instruction which will do the bound method optimization.

@AA-Turner AA-Turner added the performance label Jun 6, 2022
@markshannon markshannon merged commit f012df7 into python:main Jun 7, 2022
13 checks passed
@@ -4581,12 +4581,9 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int
DEOPT_IF(self_cls->tp_version_tag != read_u32(cache->type_version),
LOAD_METHOD);
/* Treat index as a signed 16 bit value */
int dictoffset = *(int16_t *)&cache->dict_offset;
int dictoffset = self_cls->tp_dictoffset;
Copy link
Member

@Fidget-Spinner Fidget-Spinner Jun 7, 2022

Choose a reason for hiding this comment

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

I just realised this warns on MSVC. It seems that this dictoffset and the one below needs to be Py_ssize_t

Copy link
Member Author

@markshannon markshannon Jun 7, 2022

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants