-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Add constants for Concatenate and Unpack type names #18553
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
Conversation
| if isinstance(expr, CallExpr) and self.get_fullname(expr.callee) in ( | ||
| "typing.TypeVar", | ||
| "typing_extensions.TypeVar", | ||
| "typing.ParamSpec", | ||
| "typing_extensions.ParamSpec", | ||
| "typing.TypeVarTuple", | ||
| "typing_extensions.TypeVarTuple", | ||
| ): | ||
| if isinstance(expr, CallExpr) and self.get_fullname(expr.callee) in TYPE_VAR_LIKE_NAMES: |
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.
Lines 87 to 94 in 82f4e88
| TYPE_VAR_LIKE_NAMES: Final = ( | |
| "typing.TypeVar", | |
| "typing_extensions.TypeVar", | |
| "typing.ParamSpec", | |
| "typing_extensions.ParamSpec", | |
| "typing.TypeVarTuple", | |
| "typing_extensions.TypeVarTuple", | |
| ) |
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
| CONCATENATE_TYPE_NAMES: Final = ("typing.Concatenate", "typing_extensions.Concatenate") | ||
|
|
||
| # Supported Unpack type names. | ||
| UNPACK_TYPE_NAMES: Final = ("typing.Unpack", "typing_extensions.Unpack") |
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.
It would probably be slightly faster with current mypyc to annotated these as Final[tuple[str, ...]], as otherwise I'd expect there to be more boxing/unboxing.
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.
There are quite a few of these already with just Final. Would it be alright if I explore possible performance gains in a followup?
Just for pure Python, using set or frozenset would probably be even faster than tuple. Would need to check if that's true for mypyc as well.
Lines 112 to 134 in f49a1cb
| # Supported names of Protocol base class. | |
| PROTOCOL_NAMES: Final = ("typing.Protocol", "typing_extensions.Protocol") | |
| # Supported TypeAlias names. | |
| TYPE_ALIAS_NAMES: Final = ("typing.TypeAlias", "typing_extensions.TypeAlias") | |
| # Supported Final type names. | |
| FINAL_TYPE_NAMES: Final = ("typing.Final", "typing_extensions.Final") | |
| # Supported @final decorator names. | |
| FINAL_DECORATOR_NAMES: Final = ("typing.final", "typing_extensions.final") | |
| # Supported @type_check_only names. | |
| TYPE_CHECK_ONLY_NAMES: Final = ("typing.type_check_only", "typing_extensions.type_check_only") | |
| # Supported Literal type names. | |
| LITERAL_TYPE_NAMES: Final = ("typing.Literal", "typing_extensions.Literal") | |
| # Supported Annotated type names. | |
| ANNOTATED_TYPE_NAMES: Final = ("typing.Annotated", "typing_extensions.Annotated") | |
| # Supported @deprecated type names | |
| DEPRECATED_TYPE_NAMES: Final = ("warnings.deprecated", "typing_extensions.deprecated") |
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.
From my initial testing it looks like your hunch was correct. While is faster to construct the Final tuples, they need to be recreated for each call. In contrast Final[tuple[str, ...]] takes slightly longer to setup but will be reused.
The bigger performance gain would be to optimize for contains. They are nearly exclusively used with ... in .... A good example here is try_analyze_special_unbound_type which would benefit from using set instead.
Mypyc doesn't seem to optimize frozenset yet, does it? AFAICT it still uses PySequence_Contains for it, not PySet_Contains.
Lines 610 to 619 in d4e7a81
| def try_analyze_special_unbound_type(self, t: UnboundType, fullname: str) -> Type | None: | |
| """Bind special type that is recognized through magic name such as 'typing.Any'. | |
| Return the bound type if successful, and return None if the type is a normal type. | |
| """ | |
| if fullname == "builtins.None": | |
| return NoneType() | |
| elif fullname == "typing.Any" or fullname == "builtins.Any": | |
| return AnyType(TypeOfAny.explicit, line=t.line, column=t.column) | |
| elif fullname in FINAL_TYPE_NAMES: |
--
Will change it in a followup so we can do a proper performance comparison. Although the improvement will probably be fairly small, I suspect.
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.
Mypyc doesn't optimize frozenset much. And in small collections linear search may be faster than a set lookup. The best long-term solution is for mypyc to optimize in operations targeting final fixed-length tuples, so unless there is a measurable performance improvement from using variable-length tuples, it's not worth it.
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.
Did some performance testing today. AFAICT the difference is below the noice. Probably indeed not worth it at this point.
The main advantage this change provided is that devs don't accidentally miss on of the type names if they reuse it. IIRC this happened for typing_extensions.Unpack a while back.
No description provided.