Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-131798: JIT: Narrow the return type ofisinstance for some known arguments#133172
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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)) { |
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 simulatesPyObject_TypeCheck
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? :)
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!
Python/optimizer_bytecodes.c Outdated
| @@ -886,6 +886,44 @@ dummy_func(void) { | |||
| } | |||
| } | |||
| op(_CALL_ISINSTANCE, (callable, self_or_null, args[oparg] -- res)) { | |||
| if (sym_is_null(self_or_null) || sym_is_not_null(self_or_null)) { | |||
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 withself_or_null but it's not clear to me whether it is needed here as well?
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'sNULL or not". Maybe it's worth adding a helper function (in another PR) to make it clearer, since the meaning is super subtle.
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 secondisinstance arg is a class with a metaclass defining__instancecheck__?
classEvenNumberMeta(type):def__instancecheck__(self,number):returnnotnumber%2classEvenNumber(metaclass=EvenNumberMeta):pass# Optimizer only narrows to bool, runtime value is True:even=isinstance(42,EvenNumber)
Lib/test/test_capi/test_opt.py Outdated
| 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 |
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:
| classFoo: | |
| bar=42 | |
| x=0 | |
| for_inrange(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) | |
| ify: | |
| x+=1 | |
| returnx | |
| x=0 | |
| for_inrange(n): | |
| # The optimizer doesn't know the return type here: | |
| bar=eval("42") | |
| # This will only narrow to bool: | |
| y=isinstance(bar,int) | |
| ify: | |
| x+=1 | |
| returnx |
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 useeval! I updated the test :)
Uh oh!
There was an error while loading.Please reload this page.
Python/optimizer_bytecodes.c Outdated
| @@ -886,6 +886,44 @@ dummy_func(void) { | |||
| } | |||
| } | |||
| op(_CALL_ISINSTANCE, (callable, self_or_null, args[oparg] -- res)) { | |||
| if (sym_is_null(self_or_null) || sym_is_not_null(self_or_null)) { | |||
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'sNULL or not". Maybe it's worth adding a helper function (in another PR) to make it clearer, since the meaning is super subtle.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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)) { |
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? :)
Python/optimizer_bytecodes.c Outdated
| } | ||
| 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); | ||
| } | ||
| } |
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 unconditionalres = sym_new_type(ctx, &PyBool_Type); at the top, and usingsym_set_const(ctx, ...) to narrow it when possible.
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!
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 |
(Marking as draft until#133339 is merged which will let us simplify the oparg logic) |
I think I addressed all your points. I also added the test you suggested. I'm planning to add support for tuples (e.g. |
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.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
This LGTM. A side note: on PyPy, they can actually narrow |
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. |
CI failures are unrelated. |
8d490b3 intopython:mainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
In this PR:
isintance(obj, cls)toTrueif obj is a known type and cls is a known class and obj is a subclass of cls (and vice versa forFalse)isinstancetobool.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 :)