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: JIT: Further optimize `_CALL_ISINSTANCE` for class tuples by tomasr8 · Pull Request #134543 · python/cpython

Conversation

@tomasr8

@tomasr8 tomasr8 commented May 22, 2025 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

We can already const eval isinstance(inst, cls) calls when both arguments are known, but only if cls is a single class (e.g. int).
This PR adds support for isinstance(inst, (cls1, cls2, ..., clsN)). This allows us to optimize for example:

  • isinstance(42, (int, str)) (to True)
  • isinstance(42, (bool, str)) (to False)

We can narrow to True even when only some of the classes are known, as long as inst is an instance of one of the known classes.

  • 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.
All reactions
tomasr8 added 2 commits May 22, 2025 22:20
@tomasr8
Add news entry
1d582e8
@tomasr8
Optimize _CALL_ISINSTANCE for class tuples
97aca36
@tomasr8 tomasr8 changed the title gh-134369: JIT: Further optimize _CALL_ISINSTANCE for class tuples gh-131798: JIT: Further optimize _CALL_ISINSTANCE for class tuples May 22, 2025
@bedevere-app bedevere-app Bot mentioned this pull request May 22, 2025
Better uop coverage in the JIT optimizer #131798
Open
@tomasr8
Add more tests
467bcb9
tomasr8
tomasr8 commented May 22, 2025
View reviewed changes
Comment thread Lib/test/test_capi/test_opt.py
Comment on lines +2124 to +2125
self.assertIn("_BUILD_TUPLE", uops)
self.assertIn("_POP_CALL_TWO_LOAD_CONST_INLINE_BORROW", uops)

@tomasr8 tomasr8 May 22, 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.

_BUILD_TUPLE is preventing us from optimizing out _POP_CALL_TWO_LOAD_CONST_INLINE_BORROW.
The bytecode is basically:

LOAD_CONST
LOAD_CONST
_BUILD_TUPLE
_POP_CALL_TWO_LOAD_CONST_INLINE_BORROW

To optimize this, we'd need some special handling for _BUILD_TUPLE in remove_unneeded_uops.

Sorry, something went wrong.

Uh oh!

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

@brandtbucher brandtbucher Jul 29, 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.

Got it, might be worth looking into next if you're up for it! Could be tricky, though.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tomasr8 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Python/optimizer_bytecodes.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@tomasr8
tomasr8 marked this pull request as ready for review May 22, 2025 22:34
@tomasr8
tomasr8 requested a review from Fidget-Spinner as a code owner May 22, 2025 22:34
@bedevere-app bedevere-app Bot added the awaiting review label May 22, 2025
@tomasr8
tomasr8 requested a review from brandtbucher May 22, 2025 22:34
brandtbucher
brandtbucher requested changes Jul 1, 2025
View reviewed changes
Comment thread Python/optimizer_bytecodes.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/optimizer_bytecodes.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot added awaiting changes and removed awaiting review labels Jul 1, 2025
@bedevere-app

bedevere-app Bot commented Jul 1, 2025

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tomasr8 added 5 commits July 5, 2025 23:17
@tomasr8
Bail when encountering an unknown class
d2e339f
@tomasr8
Remove an extra test case
0465c9a
@tomasr8
Simplify code
088ccd8
@tomasr8
Merge branch 'main' into jit-tuple-isinstance
fb28e06
@tomasr8
Fix incorrect type
1e9b95f
@tomasr8

tomasr8 commented Jul 19, 2025

Copy link
Copy Markdown
Member Author

I'll fix the PR hopefully this weekend.

So that was a lie 😆

Anyway, I think I addressed all your comments! :)

I have made the requested changes; please review again
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot added awaiting change review and removed awaiting changes labels Jul 19, 2025
@bedevere-app

bedevere-app Bot commented Jul 19, 2025

Copy link
Copy Markdown

Thanks for making the requested changes!

@brandtbucher: please review the changes made to this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app
bedevere-app Bot requested a review from brandtbucher July 19, 2025 09:38
brandtbucher
brandtbucher approved these changes Jul 29, 2025
View reviewed changes

@brandtbucher brandtbucher 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.

Looks good, just a few suggestions:

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tomasr8 reacted with rocket emoji
All reactions
  • 🚀 1 reaction
Comment thread Lib/test/test_capi/test_opt.py
Comment on lines +2124 to +2125
self.assertIn("_BUILD_TUPLE", uops)
self.assertIn("_POP_CALL_TWO_LOAD_CONST_INLINE_BORROW", uops)

@brandtbucher brandtbucher Jul 29, 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.

Got it, might be worth looking into next if you're up for it! Could be tricky, though.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tomasr8 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Lib/test/test_capi/test_opt.py
for _ in range(n):
# One of the classes is unknown, so we can't narrow
# to True or False, only bool
y = isinstance(42, (eval('str'), int))

@brandtbucher brandtbucher Jul 29, 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.

I'm confused, why can't we narrow to True? We can't remove the call, but the result is known.

Sorry, something went wrong.

Uh oh!

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

@tomasr8 tomasr8 Jul 29, 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.

Yep, that was a brainfart 😄 I somehow conflated narrowing to True/False with replacing the op, but we can obviously just narrow without removing the call as you said!

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
def testfunc(n):
x = 0
for _ in range(n):
y = isinstance(42, ())

@brandtbucher brandtbucher Jul 29, 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.

I had to pause and think for a second to figure out what this would even do. Nice edge case.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tomasr8 reacted with laugh emoji
All reactions
  • 😄 1 reaction
Comment thread Python/optimizer_bytecodes.c Outdated
// It could potentially define its own __instancecheck__
// method so we can only deduce bool.
out = NULL;
break;

@brandtbucher brandtbucher Jul 29, 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.

Could this be continue? We could still narrow to True later to handle cases like the one I called out above, right?

Suggested change
break;
continue;

Sorry, something went wrong.

Uh oh!

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

@tomasr8 tomasr8 Jul 29, 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.

Yup, see #134543 (comment). I just needed to add some extra bookkeeping to know when we can replace the call and when not.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/optimizer_bytecodes.c
}
PyTypeObject *cls_o = (PyTypeObject *)sym_get_const(ctx, item);
if (cls_o &&
sym_matches_type(item, &PyType_Type) &&

@brandtbucher brandtbucher Jul 29, 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.

Maybe add a comment explaining that this is to protect against metaclasses definine __instancecheck__.

Sorry, something went wrong.

Uh oh!

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

@tomasr8 tomasr8 Jul 29, 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.

How would you formulate it? I don't think of it as specifically a guard for __instancecheck__ but basically PyObject_TypeCheck adapted to the JIT optimizer.

Sorry, something went wrong.

Uh oh!

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

@brandtbucher brandtbucher Jul 29, 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.

We're not only checking that the object is a subclass of type, we're also checking that it is an exact instance of type itself. We care about this second condition because it guarantees that __instancecheck__ doesn't exist (otherwise we would need to look it up to check if it exists or not).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tomasr8 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@tomasr8 tomasr8 Aug 2, 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.

Thanks for the clarification, I added a comment :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting change review labels Jul 29, 2025
@tomasr8
Narrow to true but keep the isinstance call
a09b860
@tomasr8
tomasr8 requested a review from brandtbucher July 29, 2025 18:59
brandtbucher
brandtbucher approved these changes Jul 29, 2025
View reviewed changes
@brandtbucher

brandtbucher commented Jul 29, 2025

Copy link
Copy Markdown
Member

Off-topic, but something I've been thinking about that you may want to check out next: removing bounds checks when indexing into strings and tuples with a constant index.
tomasr8 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.
@tomasr8
Add a comment about __instancecheck__
17701d3
@tomasr8

tomasr8 commented Aug 2, 2025

Copy link
Copy Markdown
Member Author

Thanks for the review! Super helpful as always :) I'll leave this open for a few days so @Fidget-Spinner can also have a look :)

Off-topic, but something I've been thinking about that you may want to check out next: removing bounds checks when indexing into strings and tuples with a constant index.

Sure, I can look into that! I guess this will also require knowing the string/tuple length?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@tomasr8 tomasr8 mentioned this pull request Aug 10, 2025
gh-131798: Remove bounds check when indexing into tuples with a constant index #137607
Merged
Fidget-Spinner
Fidget-Spinner reviewed Aug 20, 2025
View reviewed changes
Comment thread Python/optimizer_bytecodes.c
if (cls_o &&
// Ensure that item is an exact instance of `type` ensuring that
// there is no __instancecheck__ defined.
sym_matches_type(item, &PyType_Type) &&

@Fidget-Spinner Fidget-Spinner Aug 20, 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.

Wait, wouldn't this only work on something that is isinstance(x, type), like the actual type itself, not any type like int? Sorry, I'm confused!

Sorry, something went wrong.

Uh oh!

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

@tomasr8 tomasr8 Aug 23, 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.

Hmm I don't believe so. If item is e.g. int then sym_matches_type(item, &PyType_Type) should return true

Sorry, something went wrong.

Uh oh!

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

@tomasr8 tomasr8 Aug 23, 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.

For example something like this seems to work:

JitOptRef ref_type = _Py_uop_sym_new_const(ctx, (PyObject *)&PyLong_Type);
TEST_PREDICATE(_Py_uop_sym_matches_type(ref_type, &PyType_Type), "int is not a type");

Sorry, something went wrong.

Uh oh!

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

@Fidget-Spinner Fidget-Spinner Aug 24, 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.

That''s kinda strange. The code for _Py_uop_sym_matches_type is:

_Py_uop_sym_get_type(sym) == typ

so it's a pointer comparison, not a subclass check.

Sorry, something went wrong.

Uh oh!

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

@tomasr8 tomasr8 Aug 24, 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.

The subclass check is done after with PyType_IsSubtype if that's what you mean? The current version of _CALL_ISINSTANCE (which does not handle tuples) works the same way though. @brandtbucher, what do you think?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
tomasr8 added 2 commits December 27, 2025 22:33
@tomasr8
Merge remote-tracking branch 'upstream/main' into jit-tuple-isinstance
cf71d8b
@tomasr8
Regenerate files
25ae15f
@tomasr8
tomasr8 requested a review from markshannon as a code owner December 27, 2025 21:34
@Fidget-Spinner

Fidget-Spinner commented Dec 28, 2025

Copy link
Copy Markdown
Member

I'm really sorry Tomas, but the more I look at the code, the more I feel the complexity is not worth it in this case. __instancecheck__ is tricky (and you've handled that well in the code), but I feel for the additional lines we get in the optimizer_bytecodes.c, we don't get much back in terms of optimization :(.
tomasr8 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.
@tomasr8 tomasr8 closed this Dec 28, 2025
@tomasr8 tomasr8 reopened this Jan 8, 2026
Fidget-Spinner
Fidget-Spinner reviewed Jan 9, 2026
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.

We need a test for a type with __instancecheck__, otherwise it LGTM, and you can merge after the test is added

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tomasr8 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@github-actions

github-actions Bot commented Apr 25, 2026

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Apr 25, 2026
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 left review comments
@brandtbucher brandtbucher brandtbucher 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

No one assigned

Labels

awaiting merge 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.

Uh oh!

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

3 participants

@tomasr8 @brandtbucher @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.