FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

perf: try to cache inner contexts of overloads by sterliakov · Pull Request #19408 · python/mypy · GitHub

Repository navigation

perf: try to cache inner contexts of overloads - #19408

Closed
sterliakov wants to merge 10 commits into
python:masterfrom
sterliakov:experiments/st-overload-cache
Closed

sterliakov wants to merge 10 commits into
python:masterfrom
sterliakov:experiments/st-overload-cache

Conversation

sterliakov commented Jul 8, 2025 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Improves #14978 and marginally improves nested overloads checking in general. This is not ready for review, but I would appreciate any in-progress feedback on the idea itself.

If I understand correctly, this change should not change any behavior at all: infer_arg_types_in_empty_context is almost pure.

This change halves colour check time.

Another very similar problem arises when checking overloaded binary operations (like in #14978), they do not translate directly into a series of overloads check but repeat the whole process for every node instead. Improving overload checking helps with that, but the result is still slow beyond reasonable. I'll try to push this further later in a separate PR.

This comment has been minimized.

sterliakov changed the title WIP: try to cache inner contexts of overloads and ops WIP: try to cache inner contexts of overloads Jul 8, 2025

This comment has been minimized.

sterliakov closed this Jul 8, 2025
sterliakov reopened this Jul 9, 2025
sterliakov force-pushed the experiments/st-overload-cache branch from c7da595 to a1f8c77 Compare July 9, 2025 15:00

This comment has been minimized.

Copy link
Copy Markdown
Collaborator Author

Now this is what I wanted to see: no behavior changes and colour brought down to 27 minutes from 50 - almost halved!

This is not ready for code review, removing draft status to get some feedback on the idea itself. I do not particularly like using id(expr) as a cache key, but it should be OK? TempNode is excluded, and everything else should not be subject to ID rot because objects actually remain in the parsed tree for the whole lifetime of checker, their IDs cannot get reused, and we should not construct any nodes other than TempNode while checking function calls.

sterliakov marked this pull request as ready for review July 9, 2025 17:18

This comment has been minimized.

sterliakov changed the title WIP: try to cache inner contexts of overloads perf: try to cache inner contexts of overloads Jul 10, 2025

JukkaL left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think we need something along these lines, but it may be more complicated than this to maintain 100% compatibility with non-cached behavior. I suspect that we may need to consider the binder and possibly cache generated errors as well. There may be some other relevant state that is kind of implicit in the type checker. I can look into this in more detail and reason through all possible issues, but it will take some effort.

Copy link
Copy Markdown
Collaborator Author

That's why I only added caching to infer_args_in_empty_context. It's a nice place to do that: diagnostics from this call are never silenced and do not affect current overload resolution. We reject incompatible signatures later, accepting all arguments again anyway. Furthermore, it is only called when checking overload calls and Any calls, so I can safely remove overload stack check from it and replace it with a boolean flag. So I don't think that the generated errors can become relevant later. This might change overload resolution in presence of a globally invalid argument (foo(1 + 'a') where foo is overloaded), but any behavior is reasonable in such case IMO: we still emit an error that the argument is invalid itself. To prevent this, it is enough to record whether any errors were encountered, and skip the cache in such case.

Binder is indeed an implicit dependency, but it's still OK within a parent overload: re-accepting an AssignmentExpr should not change anything? Anyway, we can poison the cache in visit_assignment_expr to be extra sure, and that's probably a reasonable thing to do.

I'll add the changes mentioned above in a few hrs, but frankly my main "wtf" here is using IDs as cache keys.

Copy link
Copy Markdown
Contributor

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

Copy link
Copy Markdown
Collaborator Author

Still quite nice in a single sample:

/tmp/mypy_primer/mypy_new/venv/bin/mypy on colour took 1988.16s
/tmp/mypy_primer/mypy_old/venv/bin/mypy on colour took 2953.89s

ilevkivskyi commented Jul 23, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

FWIW I am trying a (much) higher-level caching, like caching check_call() or maybe even accept() itself. Hopefully I will have time to finish a PR this weekend.

Copy link
Copy Markdown
Collaborator Author

Superseded by #19505 which implements a superset of this functionality (but only for some expressions), further attempts should add other expressions there instead of rolling out another cache on top.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants


Back | FazBrowse Home | New Git URL