-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Improve daemon #4169
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
Improve daemon #4169
Conversation
I came up with a test case that triggers the use of stale trees (it should fail on master). It's probably possible to simplify it further.
Here's the failure I get on master:
|
Thanks! This is now ready for review. I saw your other issue (#4185), I'll look into it but it's probably orthogonal to this PR. |
Thanks for coming up with that test! It doesn't even touch the binder -- it seems the crucial part is taking the join of two types that are both |
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.
A few minor notes below. I noticed that the performance is inconsistent when using the incremental checked script. Here is an output extract from two runs:
Now testing commit 85c8fecd: "Generate additional fine-grained dependencies (#4139)"
Output matches expected result!
Incremental: 10.798 sec
Original: 14.387 sec
Now testing commit 9c39b15e: "[dmypy] Factored GC and total build time into a context manager"
Output matches expected result!
Incremental: 11.025 sec
Original: 14.374 sec
Now testing commit 18efb5c3: "Fix comment in response to #4130 review"
Output matches expected result!
Incremental: 16.552 sec
Original: 14.422 sec
Now testing commit 05763a91: "Fix start_daemon() -- don't allow reusing existing daemon"
Output matches expected result!
Incremental: 11.152 sec
Original: 14.008 sec
Now testing commit 2bc4b42c: "Get rid of has_errors; meta is None is the same"
Output matches expected result!
Incremental: 11.310 sec
Original: 13.964 sec
Now testing commit e0d8f090: "Robustify start_daemon(); add -x/--exit-on-error flag"
Output matches expected result!
Incremental: 11.110 sec
Original: 14.072 sec
Now testing commit 08915ae0: "Cleanup"
Output matches expected result!
Incremental: 11.195 sec
Original: 14.105 sec
Now testing commit 43f8176d: "Add --log-file flag to dmypy [re]start commands"
Output matches expected result!
Incremental: 10.903 sec
Original: 14.018 sec
Now testing commit 85b0f027: "Crude fix to avoid references to stale trees"
Output matches expected result!
Incremental: 11.372 sec
Original: 14.128 sec
Now testing commit 9dc3dd49: "Clean up loading trees from memory (add comments)"
Output matches expected result!
Incremental: 19.091 sec
Original: 14.275 sec
Now testing commit a2dfbbd4: "Jukka's test for daemon tree reuse (yay!)"
Output matches expected result!
Incremental: 11.287 sec
Original: 14.203 sec
Now testing commit 921261e1: "Merge branch 'dmypy2' of https://github.com/gvanrossum/mypy into gvanrossum-dmypy2"
Output matches expected result!
Incremental: 11.003 sec
Original: 14.304 sec
Another one:
Now testing commit 85c8fecd: "Generate additional fine-grained dependencies (#4139)"
Output matches expected result!
Incremental: 0.369 sec
Original: 14.387 sec
Now testing commit e0d8f090: "Robustify start_daemon(); add -x/--exit-on-error flag"
Output matches expected result!
Incremental: 11.203 sec
Original: 14.072 sec
Now testing commit 08915ae0: "Cleanup"
Output matches expected result!
Incremental: 0.301 sec
Original: 14.105 sec
Now testing commit 43f8176d: "Add --log-file flag to dmypy [re]start commands"
Output matches expected result!
Incremental: 1.243 sec
Original: 14.018 sec
Now testing commit 85b0f027: "Crude fix to avoid references to stale trees"
Output matches expected result!
Incremental: 2.722 sec
Original: 14.128 sec
Now testing commit 9dc3dd49: "Clean up loading trees from memory (add comments)"
Output matches expected result!
Incremental: 2.728 sec
Original: 14.275 sec
Now testing commit a2dfbbd4: "Jukka's test for daemon tree reuse (yay!)"
Output matches expected result!
Incremental: 2.712 sec
Original: 14.203 sec
Now testing commit 921261e1: "Merge branch 'dmypy2' of https://github.com/gvanrossum/mypy into gvanrossum-dmypy2"
Output matches expected result!
Incremental: 10.521 sec
Original: 14.304 sec
Note how the first run had generally much slower runtimes. I'll try to find a consistent repro for this.
mypy/build.py
Outdated
@@ -1408,6 +1408,7 @@ class State: | |||
meta = None # type: Optional[CacheMeta] | |||
data = None # type: Optional[str] | |||
tree = None # type: Optional[MypyFile] | |||
tree_is_new = False # True if the tree came from the in-memory cache |
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.
The name of this attribute is kind of confusing. I'd expect this to be False
if the tree came from the in-memory cache. What about renaming this to is_from_saved_cache
?
@@ -1621,7 +1619,6 @@ def mark_interface_stale(self, *, on_errors: bool = False) -> None: | |||
"""Marks this module as having a stale public interface, and discards the cache data.""" | |||
self.meta = None | |||
self.externally_same = False | |||
self.has_errors = on_errors |
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 is this no longer needed?
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.
I introduced it in the first daemon PR but it turns out I don't need a separate flag, I can just check self.meta is None
.
@@ -570,6 +548,48 @@ def receive(sock: socket.socket) -> Any: | |||
return data | |||
|
|||
|
|||
class GcLogger: |
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 doesn't seem to depend on anything else so this could be easily moved to a new module.
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.
I'll move it as soon as there's a need for it outside this module.
The latest push should fix AppVeyor (the problem was introduced when I mistakenly removed last_manager). I've also addressed other review feedback. I'll look into the perf issues separately. |
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.
Let's merge this soon and fix any remaining issues in separate PRs. Please look at renaming tree_is_new
before merging.
Uh oh!
There was an error while loading. Please reload this page.