Try simple-minded call expression cache #19505
Conversation
This comment has been minimized.
This comment has been minimized.
|
Nice! Interpreted |
|
Hm, both changes in the primer however look suspicious, I will take a look at them later. |
mypy/checkexpr.py
Outdated
| # Deeply nested generic calls can deteriorate performance dramatically. | ||
| # Although in most cases caching makes little difference, in worst case | ||
| # it avoids exponential complexity. | ||
| # TODO: figure out why caching within lambdas is fragile. |
There was a problem hiding this comment.
I also discovered that in #19408. Accepting a lambda has explicit type context dependency (infer_lambda_type_using_context), so generic calls do really require re-accepting the lambda every time, context from outer generic may have propagated later.
There was a problem hiding this comment.
OK, thanks for the pointer. I will update the comment.
| @@ -931,7 +932,8 @@ def prefer_simple_messages(self) -> bool: | |||
| if self.file in self.ignored_files: | |||
| # Errors ignored, so no point generating fancy messages | |||
| return True | |||
| for _watcher in self._watchers: | |||
| if self._watchers: | |||
There was a problem hiding this comment.
Could you explain this change? Watchers used to be additive and that sounded reasonable to me...
There was a problem hiding this comment.
Previously, if any of the active watchers was ignoring errors, we could use simpler messages, but in presence of caching this is not valid anymore. For example, we can accept an expression when there is enclosing ignoring watcher, but then the caching watcher will record simple message, and if next time we, by chance, accept same expression in same type context, but without the ignoring watcher, an incorrect (i.e. way too terse) error message will be pulled from the cache.
Without this change 6 tests fail because of terse/simplistic error messages are used.
| # Although in most cases caching makes little difference, in worst case | ||
| # it avoids exponential complexity. | ||
| # TODO: figure out why caching within lambdas is fragile. | ||
| elif isinstance(node, (CallExpr, ListExpr, TupleExpr)) and not ( |
There was a problem hiding this comment.
Would it be difficult to allow dicts and sets here? Inline dictionaries are relatively common and even heavier than lists, and sets just for consistency.
Also operator exprs can be really heavy (#14978) and are fundamentally similar to CallExpr, are they worth considering?
There was a problem hiding this comment.
The problem with dicts/sets is that I see around 0.3% regression on self-check when I add them (but maybe this is just noise). My reasoning is that most code has a bunch of shallow dictionaries, and for those caching is just busy-work that will never be used (note caching is not free, since mypyc is slow on creating local type maps and watchers).
Anyway, I am open to considering more expression kinds to cache, but lets put those in separate PR(s).
|
OK, it looks like I found the problem with |
|
Diff from mypy_primer, showing the effect of this PR on open source code: werkzeug (https://github.com/pallets/werkzeug)
+ src/werkzeug/datastructures/structures.py:193: error: Generator has incompatible item type "tuple[Union[K, Any], Union[list[V], list[Any]]]"; expected "tuple[K, V]" [misc]
|
|
OK, the two previous changes in |
|
LG! I'm still a bit scared to approve this now, will take another look at this tomorrow:) |
|
@sterliakov Did you have time for another look? Btw comparison with current master went below noise level, probably because of #19515. But I also played with some "micro-benchmarks" involving deeply nested calls, and this PR gives performance improvements on order of 100x (no exaggeration). If there are no concrete suggestions, I think we should give this a try. |
This gives a modest 1% improvement on self-check (compiled), but it gives almost 40% on
mypy -c "import colour". Some comments:CallExpr,ListExpr, andTupleExpr, this is not very principled, I found this as a best balance between rare cases likecolour, and more common cases like self-check.In general, this is a bit scary (as this a major change), but also perf improvements for slow libraries are very tempting.