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

bpo-46841: Move the cache for LOAD_GLOBAL inline. #31575

Merged
merged 14 commits into from
Feb 28, 2022

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Feb 25, 2022

Also removes duplication of the _PyOpcode_InlineCacheEntries table.

https://bugs.python.org/issue46841

@markshannon markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Feb 25, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @markshannon for commit 26d0d70 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Feb 25, 2022
@markshannon
Copy link
Member Author

Performance is underwhelming.
Not surprising, given that we are duplicating the cache and still having to handle the old caches.

What is notable is that the number of EXTENDED_ARG instructions more than doubles from ~0.3% to ~0.7%.

@brandtbucher
Copy link
Member

Performance is underwhelming.
Not surprising, given that we are duplicating the cache and still having to handle the old caches.

Yeah, I suspect we may not see a ton of payoff until we convert most/all of the caches.

Copy link
Member

@brandtbucher brandtbucher left a comment

Choose a reason for hiding this comment

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

A few things:

  • The magic number needs to be bumped every time we do this.
  • I thought we decided to use _Py_CODEUNIT everywhere you're using uint16_t now (it may also be worth adding _Py_TWOCODEUNITS and _Py_FOURCODEUNITS aliases for uint32_t and uintptr_t, respectively).

Include/internal/pycore_code.h Show resolved Hide resolved
Tools/scripts/generate_opcode_h.py Outdated Show resolved Hide resolved
Python/ceval.c Outdated Show resolved Hide resolved
Python/ceval.c Outdated Show resolved Hide resolved
Python/specialize.c Outdated Show resolved Hide resolved
Python/specialize.c Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@markshannon
Copy link
Member Author

A few things:

* The magic number needs to be bumped every time we do this.

👍

* I thought we decided to use `_Py_CODEUNIT` everywhere you're using `uint16_t` now (it may also be worth adding `_Py_TWOCODEUNITS` and `_Py_FOURCODEUNITS` aliases for `uint32_t` and `uintptr_t`, respectively).

Hmm, I'd prefer not. Versions, for example, are 32 bit value, not 2 code units. If the size of the _Py_CODEUNIT were to change, we would still want versions to be 32 bit numbers.
So, the structs should be defined in terms of code units, but the read/write helpers should be in terms of width.

Maybe we should give up on the pretense that the size of the code unit can be easily changed, and replace _Py_CODEUNIT with the standard type uint16_t?

@markshannon markshannon merged commit 4558af5 into python:main Feb 28, 2022
Comment on lines -390 to +391
# Python 3.11a5 3481 (Use inline CACHE instructions)
# Python 3.11a5 3482 (Use inline caching for UNPACK_SEQUENCE)
# Python 3.11a5 3481 (Use inline cache for BINARY_OP)
# Python 3.11a5 3482 (Use inline caching for UNPACK_SEQUENCE and LOAD_GLOBAL)
Copy link
Member

@brandtbucher brandtbucher Feb 28, 2022

Choose a reason for hiding this comment

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

Hm, looks like the magic number wasn't actually changed... bad merge?

Copy link
Member Author

Choose a reason for hiding this comment

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

I figured we didn't need more than one version number per day.

Copy link
Member Author

Choose a reason for hiding this comment

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

#31618 has another version number bump.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see what you did now. Thanks for clarifying.

@brandtbucher brandtbucher mentioned this pull request Mar 2, 2022
26 tasks
@markshannon markshannon deleted the inline-cache-load-globals branch September 26, 2023 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants