Two optimizations for load_graph() #4294
Merged
Conversation
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
added 3 commits
November 29, 2017 20:28
(Alternatively we could put the cache in BuildManager.)
JukkaL
reviewed
Nov 30, 2017
mypy/options.py
Outdated
| @@ -177,17 +180,22 @@ def __repr__(self) -> str: | |||
| return 'Options({})'.format(pprint.pformat(self.__dict__)) | |||
|
|
|||
| def clone_for_module(self, module: str) -> 'Options': | |||
| res = self.clone_cache.get(module) | |||
There was a problem hiding this comment.
Add docstring that mentions that after this method has been called the options object should be treated as immutable, because otherwise the cache would become stale.
| @@ -1056,11 +1058,10 @@ def validate_meta(meta: Optional[CacheMeta], id: str, path: Optional[str], | |||
|
|
|||
| # TODO: Share stat() outcome with find_module() | |||
| path = os.path.abspath(path) | |||
| # TODO: Don't use isfile() but check st.st_mode | |||
| if not os.path.isfile(path): | |||
| st = manager.get_stat(path) # TODO: Errors | |||
There was a problem hiding this comment.
What about adding a is_file method to BuildManager instead and using it here?
There was a problem hiding this comment.
That's a bigger effort, we'd have to give BuildManager a cache results and use that everywhere. There's also an inefficiency in find_module(), which seems to make way to many stat() and listdir() calls, which I haven't tracked down.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Profiling load_graph() found that there were some unnecessary stat() calls going on, and, more importantly, that Options.clone_for_module() is very inefficient if you have many sections in your mypy.ini file. I solved the latter by introducing a cache. An alternative design would move the cache into BuildManager -- that would avoid a reference cycle, but it turns out that having the cache in the root Options object means that it survives between dmypy check runs, and that's a nice win there. (This explains at least partly why load_graph() was slow even with everything loaded in memory.)
@JukkaL