Optimize mypy.fastparse2 #5735
Optimize mypy.fastparse2 #5735
Conversation
mypy/fastparse2.py
Outdated
| if isinstance(s.s, bytes): | ||
| n = s.s | ||
| if isinstance(n.s, bytes): | ||
| s = n.s |
There was a problem hiding this comment.
I would use a longer name here instead of s, maybe string. Also s used in above methods for statements.
| e = SuperExpr(member_expr.name, obj) # type: Expression | ||
| else: | ||
| e = member_expr | ||
| return self.set_line(e, n) |
There was a problem hiding this comment.
I am curious, does this (using isinstance() with native classes in this method) give a visible perf win?
Also maybe it is worth adding a comment, otherwise it is not clear why we always create a MemberExpr instead of implementing this as it was before.
There was a problem hiding this comment.
I didn't measure the impact of each individual change, but this should be slightly faster since super expressions are much less common than member expressions, and the check for a super expression will be faster in the new implementation since we can do native attribute access (obj.callee and such). The impact of this particular change may not be measurable, but the overall reduction in Python attribute accesses gave a measurable aggregate performance increase in the Python 3 AST transformation case at least. Will add a comment.
| if isinstance(arg, Name): | ||
| v = arg.id | ||
| elif isinstance(arg, ast27_Tuple): | ||
| v = '__tuple_arg_{}'.format(index + 1) |
There was a problem hiding this comment.
Should we add a test for this? Or do we already have one?
There was a problem hiding this comment.
We already had tests for Python 2 specific features such as this.
| FuncDef:7( | ||
| g | ||
| Block:7( | ||
| PassStmt:8())))) |
There was a problem hiding this comment.
Thanks for adding all these tests!
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.)