GeezerVison

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

GeezerVison is showing github.com — gh-131798: JIT: Narrow the return type of `isinstance` for some known arguments by tomasr8 · Pull Request #133172 · python/cpython

Conversation

tomasr8 commented Apr 29, 2025 •

edited by bedevere-app Bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

In this PR:

narrows isintance(obj, cls) to True if obj is a known type and cls is a known class and obj is a subclass of cls (and vice versa for False)
in all other cases narrows isinstance to bool.

Brandt also suggested adding an optimization for tuples which I'd like to add in a followup in order to keep the sizes of the individual PRs smaller. Though if you prefer to have it in one PR I can do it as well :)

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 3 commits

April 29, 2025 23:25


          Optimize CALL_ISINSTANCE

59b37f7


          Add news entry

a81c02e


          Optimize for subclasses as well

b1eb6a0

tomasr8 requested review from Fidget-Spinner and markshannon as code owners

April 29, 2025 21:35

bedevere-app Bot added the awaiting review label

Apr 29, 2025

bedevere-app Bot mentioned this pull request

Apr 29, 2025

Better uop coverage in the JIT optimizer #131798

Open

tomasr8 commented

Apr 29, 2025

View reviewed changes

Comment thread

Python/optimizer_bytecodes.c Outdated

		// isinstance(obj, cls) where both obj and cls have known types
		// We can deduce either True or False
		PyTypeObject *inst_type = sym_get_type(inst_sym);
		if (sym_matches_type(inst_sym, cls) \|\| PyType_IsSubtype(inst_type, cls)) {

tomasr8 Apr 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.

This simulates PyObject_TypeCheck

Sorry, something went wrong.

Uh oh!

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

All reactions

brandtbucher May 2, 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.

Can you add a comment to that effect? :)

Sorry, something went wrong.

Uh oh!

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

All reactions

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

Added!

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Python/optimizer_bytecodes.c Outdated

		}

		op(_CALL_ISINSTANCE, (callable, self_or_null, args[oparg] -- res)) {
		if (sym_is_null(self_or_null) \|\| sym_is_not_null(self_or_null)) {

tomasr8 Apr 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.

I've seen this guard used elsewhere with self_or_null but it's not clear to me whether it is needed here as well?

Sorry, something went wrong.

Uh oh!

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

All reactions

brandtbucher May 2, 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.

Yep. It's just a (weird) way of saying "we know whether it's NULL or not". Maybe it's worth adding a helper function (in another PR) to make it clearer, since the meaning is super subtle.

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

brandtbucher requested changes

May 2, 2025

View reviewed changes

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.

Cool! Can you also add a test where the second isinstance arg is a class with a metaclass defining __instancecheck__?

class EvenNumberMeta(type):
    def __instancecheck__(self, number):
        return not number % 2

class EvenNumber(metaclass=EvenNumberMeta):
    pass

# Optimizer only narrows to bool, runtime value is True:
even = isinstance(42, EvenNumber)

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 Outdated

Comment on lines +1996 to +2008

		class Foo:
		bar = 42

		x = 0
		for _ in range(n):
		# we only know bar (LOAD_ATTR) is not null (set via sym_new_not_null)
		bar = Foo.bar
		# This will only narrow to bool and not to True due to 'bar' having
		# unknown (non-null) type
		y = isinstance(bar, int)
		if y:
		x += 1
		return x

brandtbucher May 2, 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.

This is pretty fragile: the class attr lookup is cached in the bytecode, and I actually have a branch I'll open a PR with soon that teaches the optimizer to read these caches.

So instead, let's use everyone's favorite optimization-breaker:

Suggested change

	class Foo:
	bar = 42

	x = 0
	for _ in range(n):
	# we only know bar (LOAD_ATTR) is not null (set via sym_new_not_null)
	bar = Foo.bar
	# This will only narrow to bool and not to True due to 'bar' having
	# unknown (non-null) type
	y = isinstance(bar, int)
	if y:
	x += 1
	return x
	x = 0
	for _ in range(n):
	# The optimizer doesn't know the return type here:
	bar = eval("42")
	# This will only narrow to bool:
	y = isinstance(bar, int)
	if y:
	x += 1
	return x

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 May 9, 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.

Nice trick to use eval! I updated the test :)

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Python/bytecodes.c Resolved

Uh oh!

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

Comment thread

Python/optimizer_bytecodes.c Outdated

		}

		op(_CALL_ISINSTANCE, (callable, self_or_null, args[oparg] -- res)) {
		if (sym_is_null(self_or_null) \|\| sym_is_not_null(self_or_null)) {

brandtbucher May 2, 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.

Yep. It's just a (weird) way of saying "we know whether it's NULL or not". Maybe it's worth adding a helper function (in another PR) to make it clearer, since the meaning is super subtle.

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.

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 Outdated

		// isinstance(obj, cls) where both obj and cls have known types
		// We can deduce either True or False
		PyTypeObject *inst_type = sym_get_type(inst_sym);
		if (sym_matches_type(inst_sym, cls) \|\| PyType_IsSubtype(inst_type, cls)) {

brandtbucher May 2, 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.

Can you add a comment to that effect? :)

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Python/optimizer_bytecodes.c Outdated

Comment on lines +911 to +925

		}
		else {
		// isinstance(obj, cls) where obj has unknown type
		res = sym_new_type(ctx, &PyBool_Type);
		}
		}
		else {
		// isinstance(obj, cls) where cls has unknown type
		res = sym_new_type(ctx, &PyBool_Type);
		}
		}
		else {
		res = sym_new_type(ctx, &PyBool_Type);
		}
		}

brandtbucher May 2, 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.

You can avoid all of this repetition by doing an unconditional res = sym_new_type(ctx, &PyBool_Type); at the top, and using sym_set_const(ctx, ...) to narrow it when possible.

Sorry, something went wrong.

Uh oh!

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

All reactions

tomasr8 May 9, 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 that is way better, updated!

Sorry, something went wrong.

Uh oh!

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

All reactions

bedevere-app Bot added awaiting changes and removed awaiting review labels

May 2, 2025

bedevere-app Bot commented May 2, 2025

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.

All reactions

Sorry, something went wrong.

Uh oh!

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

tomasr8 mentioned this pull request

May 3, 2025

gh-131798: JIT: Split CALL_ISINSTANCE into several uops #133339

Merged

tomasr8 marked this pull request as draft

May 3, 2025 11:57

bedevere-app Bot removed the awaiting changes label

May 3, 2025

tomasr8 commented May 3, 2025

Copy link

Copy Markdown

Member Author

(Marking as draft until #133339 is merged which will let us simplify the oparg logic)

All reactions

Sorry, something went wrong.

Uh oh!

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

tomasr8 added 10 commits

May 8, 2025 23:36


          Merge branch 'main' into jit-isinstance

c42172d


          Regen cases

770f7ed


          Add a comment

3e86810


          Merge remote-tracking branch 'upstream/main' into jit-isinstance

0820a3c


          Simplify

60d21fa


          Add a metaclass test

38370a3


          Improve test

bb228eb


          Update comment

0ede9f3


          Mark some stackrefs as unused

3d0df4c


          Simplify even more

ce0cd38

tomasr8 commented May 9, 2025

Copy link

Copy Markdown

Member Author

I think I addressed all your points. I also added the test you suggested. I'm planning to add support for tuples (e.g. isinstance(foo, (int, str)) in a followup so for now it just supports single types.

All reactions

Sorry, something went wrong.

Uh oh!

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

tomasr8 marked this pull request as ready for review

May 9, 2025 20:46

bedevere-app Bot added the awaiting review label

May 9, 2025

tomasr8 requested a review from brandtbucher

May 9, 2025 20:47

brandtbucher self-assigned this

May 19, 2025

brandtbucher added the sprint label

May 19, 2025

github-project-automation Bot added this to Sprint

May 19, 2025

github-project-automation Bot moved this to Todo in Sprint

May 19, 2025

brandtbucher approved these changes

May 19, 2025

View reviewed changes

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.

Thanks! Just one suggestion, then we can land it.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Python/optimizer_bytecodes.c Resolved

Uh oh!

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

github-project-automation Bot moved this from Todo to In Progress in Sprint

May 19, 2025

bedevere-app Bot added awaiting merge and removed awaiting review labels

May 19, 2025

tomasr8 and others added 3 commits

May 19, 2025 15:37


          Simplify code

20fd185

Co-authored-by: Brandt Bucher <[email protected]>


          Merge branch 'main' into jit-isinstance

62854fe


          make regen-cases

f985963

Fidget-Spinner commented May 19, 2025

Copy link

Copy Markdown

Member

This LGTM. A side note: on PyPy, they can actually narrow isinstance(x, cls) to subtype of cls or even better. I wonder if we can do that? Not sure if it's safe to do so though due to subclassing rules and all that.

All reactions

Sorry, something went wrong.

Uh oh!

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

brandtbucher commented May 19, 2025

Copy link

Copy Markdown

Member

Yeah, could be cool. It's not really useful to us right now, since all of our guards are against exact types/versions, not many subclass checks.

All reactions

Sorry, something went wrong.

Uh oh!

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


          Merge branch 'main' into jit-isinstance

27b1b43

brandtbucher commented May 19, 2025

Copy link

Copy Markdown

Member

CI failures are unrelated.

All reactions

Sorry, something went wrong.

Uh oh!

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

brandtbucher merged commit 8d490b3 into python:main

May 19, 2025

bedevere-app Bot removed the awaiting merge label

May 19, 2025

github-project-automation Bot moved this from In Progress to Done in Sprint

May 19, 2025

tomasr8 deleted the jit-isinstance branch

May 19, 2025 17:34

Pranjal095 pushed a commit to Pranjal095/cpython that referenced this pull request

Jul 12, 2025


          pythonGH-131798: Narrow the return type of isinstance for some known …

0f96049

…arguments in the JIT (pythonGH-133172)

taegyunkim pushed a commit to taegyunkim/cpython that referenced this pull request

Aug 4, 2025


          pythonGH-131798: Narrow the return type of isinstance for some known …

d9707f4

…arguments in the JIT (pythonGH-133172)

Reviewers

brandtbucher

brandtbucher approved these changes

Fidget-Spinner

Awaiting requested review from Fidget-Spinner

Fidget-Spinner is a code owner

markshannon

Awaiting requested review from markshannon

markshannon is a code owner

Uh oh!

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

Archived in project

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

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.

Conversation

tomasr8 commented Apr 29, 2025 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brandtbucher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-app Bot commented May 2, 2025

Uh oh!

tomasr8 commented May 3, 2025

Uh oh!

tomasr8 commented May 9, 2025

Uh oh!

brandtbucher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Fidget-Spinner commented May 19, 2025

Uh oh!

brandtbucher commented May 19, 2025

Uh oh!

brandtbucher commented May 19, 2025

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tomasr8 commented Apr 29, 2025 •

edited by bedevere-app Bot

Loading

Uh oh!

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