Skip to content

GH-131798: Remove JIT guards for dict, frozenset, list, set, and tuple #132289

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

Merged
merged 7 commits into from
Apr 9, 2025

Conversation

brandtbucher
Copy link
Member

@brandtbucher brandtbucher commented Apr 8, 2025

According to the stats, this removes:

  • 144 million tuple guards (25%)
  • 280 million set/frozenset guards (19%)
  • 484 million dict guards (42%)
  • 1.3 billion list guards (34%)

It also fixes a bug that I encountered in the optimizer's logic for _UNPACK_SEQUENCE_TUPLE, which puts items on the stack in reverse order.

@brandtbucher brandtbucher added performance Performance or resource usage interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-JIT labels Apr 8, 2025
@brandtbucher brandtbucher self-assigned this Apr 8, 2025
@brandtbucher
Copy link
Member Author

@Zheaoli and @tomasr8, you might be interested in checking this out. Specifically, see the changes in Python/bytecodes.c which use the same syntax as Python/optimizer_bytecodes.c, but describes the actual implementations of these instructions.

You can see that this PR breaks up the old, larger instr definitions into smaller op definitions combined into a macro. This doesn't change the semantics of the instruction itself, but allows the JIT to remove parts of the instruction as it sees fit (in this case, separating out the type checks allows the JIT to remove them).

Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

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

Just one concern.

@@ -396,7 +396,6 @@ dummy_func(void) {
op(_TO_BOOL_LIST, (value -- res)) {
int already_bool = optimize_to_bool(this_instr, ctx, value, &res);
if (!already_bool) {
sym_set_type(value, &PyList_Type);
Copy link
Member

Choose a reason for hiding this comment

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

Does CONTAIN_OP_SET and CONTAIN_OP_DICT need to remove their sym setting type as well? Or is that not even happening at the moment?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not happening currently,. _CONTAINS_OP_SET can't even set anything, since it handles both set and frozenset (this might be worth re-evaluating, or splitting up).

@Zheaoli
Copy link
Contributor

Zheaoli commented Apr 8, 2025

@Zheaoli and @tomasr8, you might be interested in checking this out. Specifically, see the changes in Python/bytecodes.c which use the same syntax as Python/optimizer_bytecodes.c, but describes the actual implementations of these instructions.

You can see that this PR breaks up the old, larger instr definitions into smaller op definitions combined into a macro. This doesn't change the semantics of the instruction itself, but allows the JIT to remove parts of the instruction as it sees fit (in this case, separating out the type checks allows the JIT to remove them).

Thanks! I think I might need some time to understand this PR! Thanks for your patience!

@brandtbucher
Copy link
Member Author

Going to go ahead and merge this to reduce the risk of merge conflicts, but let me know if you have any questions @Zheaoli!

@brandtbucher brandtbucher merged commit 20926c7 into python:main Apr 9, 2025
76 checks passed
seehwan pushed a commit to seehwan/cpython that referenced this pull request Apr 16, 2025
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 topic-JIT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants