GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — GH-131798: Remove JIT guards for `dict`, `frozenset`, `list`, `set`, and `tuple` by brandtbucher · Pull Request #132289 · python/cpython

Conversation

@brandtbucher

@brandtbucher brandtbucher commented Apr 8, 2025 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

  • Issue: Better uop coverage in the JIT optimizer #131798

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
stonebig reacted with heart emoji
All reactions
  • ❤️ 1 reaction
brandtbucher added 7 commits April 1, 2025 18:04
@brandtbucher
Add guards for dicts, lists, sets, and tuples
10a16c5
@brandtbucher
Remove old sym_set_type call
99d6288
@brandtbucher
blurb add
a471ee3
@brandtbucher
Add regression tests
0efca44
@brandtbucher
Fix backward tuple unpacking
0f643c4
@brandtbucher
Catch up with main
790faa9
@brandtbucher
Catch up with main
972f74c
@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
brandtbucher requested review from Fidget-Spinner and markshannon as code owners April 8, 2025 18:55
@bedevere-app bedevere-app Bot added the awaiting core review label Apr 8, 2025
@bedevere-app bedevere-app Bot mentioned this pull request Apr 8, 2025
Better uop coverage in the JIT optimizer #131798
Open
@brandtbucher

brandtbucher commented Apr 8, 2025

Copy link
Copy Markdown
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).
tomasr8 reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Fidget-Spinner
Fidget-Spinner reviewed Apr 8, 2025
View reviewed changes

@Fidget-Spinner Fidget-Spinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Just one concern.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_capi/test_opt.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/optimizer_bytecodes.c
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);

@Fidget-Spinner Fidget-Spinner Apr 8, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@brandtbucher brandtbucher Apr 8, 2025

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@Zheaoli

Zheaoli commented Apr 8, 2025

Copy link
Copy Markdown
Contributor

@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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Fidget-Spinner
Fidget-Spinner approved these changes Apr 8, 2025
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Apr 8, 2025
@brandtbucher

brandtbucher commented Apr 9, 2025

Copy link
Copy Markdown
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!
Zheaoli reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@brandtbucher
brandtbucher merged commit 20926c7 into python:main Apr 9, 2025
@bedevere-app bedevere-app Bot removed the awaiting merge label Apr 9, 2025
@Zheaoli Zheaoli mentioned this pull request Apr 12, 2025
gh-131798: Use sym_new_type instead of sym_new_not_null for _BUILD_LIST, _BUILD_SLICE, and _BUILD_MAP #132434
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Fidget-Spinner Fidget-Spinner Fidget-Spinner approved these changes
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@brandtbucher brandtbucher

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@brandtbucher @Zheaoli @Fidget-Spinner
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.