-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Cache type_object_type() #19514
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cache type_object_type() #19514
Conversation
This comment has been minimized.
This comment has been minimized.
Non-trivial results in |
This comment has been minimized.
This comment has been minimized.
Although remaining errors are kind of correct, they are sill weird. It looks like they appear because previously class C
@no_type_check
def __new__(cls): ... resulted in |
OK, so it turns out there was an existing bug (inconsistency) with While looking at this I found second place where we don't handle not-ready types properly (decorated overloads), again I simply don't cache in this case, since a proper fix may be non-trivial (I also update the relevant TODO item). I update the PR description to mention this. |
This comment has been minimized.
This comment has been minimized.
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.
LG! The idea makes sense, and the bench on GHA shows 2.6-2.9% win for selfcheck. Nice! I'm a bit worried that it uncovered at least three semi-unrelated issues - can there be others, not revealed by primer runs? It may be too much fun to debug caching-related bugs later...
mypy/typeops.py
Outdated
else: | ||
builtins_type = named_type("builtins.type") | ||
|
||
fallback = info.metaclass_type or builtins_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.
Can we keep it lazy (only call named_type
if info.metaclass_type
is None)?
@sterliakov Of course there can be more. But those are all real bugs, we would need to fix them sooner or later anyway. In an ideal bug-free world, we should be able to always cache. |
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
This gives almost 4% performance boost (Python 3.12, compiled). Note there is an old bug in
type_object_type()
, we treat not ready types asAny
without deferring, I disable caching in this case.Unfortunately, using this in fine-grained mode is tricky, essentially I have three options:
__init__
/__new__
I decided to choose the last option. I think it has the best balance complexity/benefits.