FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-131798: Add `MATCH_SEQUENCE/MAPPING` to the JIT optimizer by Sacul0457 · Pull Request #148124 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

gh-131798: Add MATCH_SEQUENCE/MAPPING to the JIT optimizer - #148124

Open
Sacul0457 wants to merge 8 commits into
python:mainfrom
Sacul0457:Optimize-MATCH_SEQUENCE-MAPPING
ConversationCommits (8)ChecksFiles changed
Open

gh-131798: Add MATCH_SEQUENCE/MAPPING to the JIT optimizer#148124
Sacul0457 wants to merge 8 commits into
python:mainfrom
Sacul0457:Optimize-MATCH_SEQUENCE-MAPPING

Conversation

Sacul0457 commented Apr 5, 2026 •
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Contributor

I hope there aren't any edge cases that I did not consider...

bedevere-app Bot mentioned this pull request Apr 5, 2026
Open
cocolato reviewed Apr 7, 2026
View reviewed changes
Comment thread Python/optimizer_bytecodes.c Outdated
Comment thread Python/optimizer_bytecodes.c Outdated
Comment thread Python/optimizer_bytecodes.c
op(_MATCH_MAPPING, (subject -- subject, res)) {
if (sym_has_type(subject)) {
PyTypeObject *type = sym_get_type(subject);
int match = type->tp_flags & Py_TPFLAGS_MAPPING;

cocolato Apr 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I also wonder if this logic will be broken by abc.register.

Sacul0457 Apr 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Hmm I'm not too sure either. I'll try to see if I can find out more

cocolato Apr 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

abc.register will change the tp_flags but don't update the type version, so JIT won't invalid the old trace

cocolato Apr 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@Fidget-Spinner Do we need call assign_version_tag when use abc.register?

kumaraditya303 Apr 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Why? ABC never changes the actual type flags.

Fidget-Spinner Apr 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

abc.register will change the tp_flags but don't update the type version, so JIT won't invalid the old trace

That sounds like a bug in abc.register. We should fix abc.register as that might affect the specializing interpreter as well.

kumaraditya303 Apr 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Oh, I thought it already did invalidation, so yes this is a bug.

Fidget-Spinner Apr 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@cocolato do you mind opening an issue for abc.register separately please?

cocolato Apr 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Sure, i will try to create the issue and solve this :)

Fidget-Spinner Apr 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

You don't have to solve it (unless you want to of course), but just opening an issue is fine :). Thank you

cocolato mentioned this pull request Apr 12, 2026
Closed

github-actions Bot commented May 18, 2026

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

github-actions Bot added the stale Stale PR or inactive for long period of time. label May 18, 2026
markshannon requested changes May 21, 2026
View reviewed changes

markshannon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

If we know the type, we can optimize the code as well as refining the type information

Comment thread Python/optimizer_bytecodes.c Outdated
PyTypeObject *type = sym_get_type(subject);
if (type != NULL) {
int match = type->tp_flags & Py_TPFLAGS_MAPPING;
res = sym_new_const(ctx, match ? Py_True : Py_False);

markshannon May 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Since we know what _MATCH_MAPPING is going to produce, we can do this:

REPLACE_OP(this_instr, _LOAD_COMMON_CONST, match? CONSTANT_TRUE : CONSTANT_FALSE, 0);

Comment thread Python/optimizer_bytecodes.c Outdated
PyTypeObject *type = sym_get_type(subject);
if (type != NULL) {
int match = type->tp_flags & Py_TPFLAGS_SEQUENCE;
res = sym_new_const(ctx, match ? Py_True : Py_False);

markshannon May 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Likewise:

REPLACE_OP(this_instr, _LOAD_COMMON_CONST, match? CONSTANT_TRUE : CONSTANT_FALSE, 0);

bedevere-app Bot commented May 21, 2026

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Sacul0457 commented May 21, 2026

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again

I believe mark meant _LOAD_COMMON_CONSTANT? And I have updated the tests accordingly.
However, there seems to be an error during runtime for MATCH_MAPPING when I run that test, not sure why. Feel free to edit the PR accordingly, thanks!

bedevere-app Bot commented May 21, 2026

Copy link
Copy Markdown

Thanks for making the requested changes!

@markshannon, @kumaraditya303: please review the changes made to this pull request.

kumaraditya303 reviewed May 23, 2026
View reviewed changes
Comment thread Python/optimizer_bytecodes.c
PyTypeObject *type = sym_get_type(subject);
if (type != NULL) {
int match = type->tp_flags & Py_TPFLAGS_MAPPING;
REPLACE_OP(this_instr, _LOAD_COMMON_CONSTANT, match? CONSTANT_TRUE : CONSTANT_FALSE, 0);

kumaraditya303 May 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

you need to assign to res here as well

Sacul0457 May 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

added, sorry for the wait

github-actions Bot removed the stale Stale PR or inactive for long period of time. label May 26, 2026
kumaraditya303 reviewed May 27, 2026
View reviewed changes
Comment thread Python/optimizer_bytecodes.c
PyTypeObject *type = sym_get_type(subject);
if (type != NULL) {
int match = type->tp_flags & Py_TPFLAGS_SEQUENCE;
REPLACE_OP(this_instr, _LOAD_COMMON_CONSTANT, match? CONSTANT_TRUE : CONSTANT_FALSE, 0);

kumaraditya303 May 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

In other places, LOAD_CONST_INLINE_BORROW is used which would be faster

github-actions Bot commented Aug 30, 2026

Copy link
Copy Markdown

This PR is stale because it has been open for 90 days with no activity.

github-actions Bot added the stale Stale PR or inactive for long period of time. label Aug 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

cocolato cocolato left review comments
kumaraditya303 kumaraditya303 approved these changes
tomasr8 Awaiting requested review from tomasr8 tomasr8 is a code owner
Fidget-Spinner Awaiting requested review from Fidget-Spinner Fidget-Spinner is a code owner
savannahostrowski Awaiting requested review from savannahostrowski
markshannon Awaiting requested review from markshannon markshannon is a code owner

Assignees

No one assigned

Labels

awaiting change review skip news stale Stale PR or inactive for long period of time.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL