-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Optimize mypy.fastparse (Python 3 only) #5722
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
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.
These are good optimizations. I left few small comments.
mypy/__main__.py
Outdated
cProfile.run('main(None)', 'profstats') | ||
p = pstats.Stats('profstats') | ||
p.sort_stats('time').print_stats(40) | ||
#main(None) |
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 looks like the temporary profiling code should be removed.
mypy/fastparse.py
Outdated
res = [] # type: List[Expression] | ||
for e in l: | ||
exp = self.visit(e) | ||
isinstance(exp, Expression) | ||
# assert isinstance(exp, Expression) |
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 this be removed? Anyway visit
returns Any
.
mypy/fastparse.py
Outdated
res = [] # type: List[Statement] | ||
for e in l: | ||
stmt = self.visit(e) | ||
isinstance(stmt, Statement) | ||
# assert isinstance(stmt, Statement) |
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 can probably be removed as well.
|
||
def do_func_def(self, n: Union[ast3.FunctionDef, ast3.AsyncFunctionDef], | ||
is_coroutine: bool = False) -> Union[FuncDef, Decorator]: | ||
"""Helper shared between visit_FunctionDef and visit_AsyncFunctionDef.""" | ||
no_type_check = bool(n.decorator_list and | ||
any(is_no_type_check_decorator(d) for d in n.decorator_list)) | ||
|
||
args = self.transform_args(n.args, n.lineno, no_type_check=no_type_check) | ||
lineno = n.lineno |
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.
Why not do the same for n.col_offset
? (Probably a bit below when it is actually used.)
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's only used when generating errors, so the code paths will be rare.
e = CallExpr(self.visit(n.func), | ||
arg_types, | ||
arg_kinds, | ||
cast('List[Optional[str]]', [None] * len(args)) + keyword_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.
Why the quotes are needed around List[Optional[str]]
now?
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.
They are not needed but they speed things up on older Python versions (when not compiled).
@@ -742,7 +742,7 @@ def __init__(self, name: str, type: 'Optional[mypy.types.Type]' = None) -> None: | |||
super().__init__() | |||
self._name = name # Name without module prefix | |||
# TODO: Should be Optional[str] | |||
self._fullname = cast(Bogus[str], None) # Name with module prefix | |||
self._fullname = cast('Bogus[str]', None) # Name with module prefix |
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.
Why quotes are now needed here?
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.
Again, it's a micro-optimization. Profiling hints at spending a lot of time generating temporary type objects.
self.visitor_cache[typeobj] = visitor | ||
return visitor(node) | ||
|
||
def set_line(self, node: N, n: Union[ast3.expr, ast3.stmt]) -> N: |
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.
Just a random idea, maybe it is possible to set line at the end of visit()
above and avoid "manually" calling it everywhere?
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's like this because it should only be done for certain node types, not always.
This implements various small optimizations to make AST conversion from the typed_ast AST faster in compiled mypy. This makes compiled mypy about 4% faster on self-check, and it also makes non-compiled mypy around 1% faster on self-check. Notable included optimizations: * Implement visit methods more efficiently. Don't inherit visitors from typed_ast visitor classes. * Refactor decorated methods away, as they are slow with mypyc. * Refactor away many non-native attribute lookups. * Refactor away some nested functions as they are slow with mypyc. Also removed some dead code. I'll implement the same optimizations for Python 2 AST conversions in a separate PR.
These seem to speed up Python 2 type checking runtimes by up to around 3%, when compiled with mypyc. This is similar to #5722, but for Python 2. Also added some parser tests, since Python 2 coverage was spotty. (It's still not quite at full coverage, though.)
This implements various small optimizations to make AST conversion
from the typed_ast AST faster in compiled mypy. This makes compiled
mypy about 4% faster on self-check, and it also makes non-compiled
mypy around 1% faster on self-check.
Notable included optimizations:
typed_ast
visitor classes.Also removed some dead code.
I'll implement the same optimizations for Python 2 AST conversion once
this PR has been accepted to avoid having to update two PRs based
on review.