Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Querify WF-checking so it can be cached #48939

Merged
merged 5 commits into from
Mar 22, 2018
Merged

Querify WF-checking so it can be cached #48939

merged 5 commits into from
Mar 22, 2018

Conversation

wesleywiser
Copy link
Member

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 11, 2018
wesleywiser
wesleywiser commented Mar 11, 2018
View reviewed changes
Copy link
Member Author

@wesleywiser wesleywiser left a comment

Choose a reason for hiding this comment

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

CheckTypeWellFormedVisitor.code was only ever set to ObligationCauseCode::MiscObligation so I removed it to simplify splitting the struct in the next commit.

wesleywiser
wesleywiser commented Mar 11, 2018
View reviewed changes
src/librustc_typeck/check/wfcheck.rs Outdated
@@ -26,7 +26,7 @@ use errors::{DiagnosticBuilder, DiagnosticId};
use rustc::hir::intravisit::{self, Visitor, NestedVisitorMap};
use rustc::hir;

pub struct CheckTypeWellFormedVisitor<'a, 'tcx:'a> {
pub struct CheckTypeWellFormed<'a, 'tcx:'a> {
Copy link
Member Author

Choose a reason for hiding this comment

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

It seemed best to me to split the code that visits the HIR from the code which actually does the wf calculations. At this point, this struct probably isn't that useful and its methods could just be free functions. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think it would make sense to transform them into free-standing functions. That makes it easier to see that there's no state in self that one needs to worry about.

@wesleywiser
Copy link
Member Author

Should we have three separate queries or just one that dispatches on the type of hir::Item? Also, should the queries be on-disk cacheable?

@michaelwoerister
Copy link
Member

Let's leave it a three separate queries. That's a bit cleaner, I think.
We don't need to cache the queries. Caching would store the result, but we have no result. Using ensure() allows to express that we don't need the result and we are just re-executing for the diagnostics.

If we did not use ensure() then, in the case where there are no changes, the system would detect that it does not have to re-compute (because everything is green) but in the next step it would re-compute anyway because it still needs to return the result.

@michaelwoerister
Copy link
Member

@bors try

@bors
Copy link
Contributor

bors commented Mar 12, 2018

⌛ Trying commit 2b3d2a5 with merge 843c304...

bors added a commit that referenced this pull request Mar 12, 2018
@bors
Auto merge of #48939 - wesleywiser:incr_query_wf_checking, r=<try>
843c304 
Querify WF-checking so it can be cached

r? @michaelwoerister
@michaelwoerister
Copy link
Member

Thanks, @wesleywiser! The PR looks good as is. Turning the methods into free-standing functions would make it even nicer, I think.

It started a try-build, so we can see the performance gains from this. There should be noticeable wins for clean rebuilds.

@bors
Copy link
Contributor

bors commented Mar 12, 2018

☀️ Test successful - status-travis
State: approved= try=True

@michaelwoerister
Copy link
Member

@Mark-Simulacrum, could you do a perf run for this as well, please?

@wesleywiser
Copy link
Member Author

Thanks @michaelwoerister! I just pushed another commit which converts the methods into free functions.

@michaelwoerister
Copy link
Member

Thanks, @wesleywiser!

Seems like one of the lines is a bit too long:

[00:05:32] tidy error: /checkout/src/librustc_typeck/check/wfcheck.rs:228: line longer than 100 chars
[00:05:33] some tidy checks failed

@wesleywiser wesleywiser force-pushed the incr_query_wf_checking branch from 6a7e593 to e5bbf2d Compare March 13, 2018 12:44
@wesleywiser
Copy link
Member Author

Whoops. I saw that before I pushed but then I forgot to amend my commit 🤦‍♂️

@michaelwoerister
Copy link
Member

Thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Mar 13, 2018

📌 Commit e5bbf2d has been approved by michaelwoerister

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 13, 2018
@wesleywiser
Copy link
Member Author

@michaelwoerister Did you still want a perf run?

@Mark-Simulacrum
Copy link
Member

843c304 is queued for perf in case we want it.

@michaelwoerister
Copy link
Member

OK, that queued run should have the same performance as the approved version. I'd still be interested in it, yes.

@michaelwoerister
Copy link
Member

Perf link: http://perf.rust-lang.org/compare.html?start=222b0eb583a64299f43d6640617e22f19973ddab&end=843c3044c09ed670ae4dbefc3f4db20c3d2ee642&stat=instructions%3Au

@michaelwoerister
Copy link
Member

The perf.rlo link works now. The numbers look excellent :)

@nikomatsakis
Copy link
Contributor

This rocks! Curious that style-servo got slower, but it's only in the semi-artificial case. Presumably something is no longer being reused?

@wesleywiser
Copy link
Member Author

@nikomatsakis I believe that's issue #48184 (as @michaelwoerister mentioned here).

@bors
Copy link
Contributor

bors commented Mar 16, 2018

☔ The latest upstream changes (presumably #49051) made this pull request unmergeable. Please resolve the merge conflicts.

@wesleywiser wesleywiser force-pushed the incr_query_wf_checking branch from e5bbf2d to b418b7b Compare March 16, 2018 03:17
@wesleywiser
Copy link
Member Author

Rebased

@michaelwoerister
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Mar 16, 2018

📌 Commit b418b7b has been approved by michaelwoerister

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 16, 2018
@bors
Copy link
Contributor

bors commented Mar 18, 2018

⌛ Testing commit b418b7b with merge 160c836a58911574755c40a3db91958622dd920c...

@bors
Copy link
Contributor

bors commented Mar 18, 2018

💔 Test failed - status-travis

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 18, 2018
@wesleywiser
Copy link
Member Author

Looks like a timeout on the asmjs builder.

@michaelwoerister
Copy link
Member

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 19, 2018
@michaelwoerister
Copy link
Member

@bors rollup -- hopefully this will get it merged more quickly

kennytm added a commit to kennytm/rust that referenced this pull request Mar 21, 2018
@kennytm
Rollup merge of rust-lang#48939 - wesleywiser:incr_query_wf_checking,…
237ba6f 
… r=michaelwoerister

Querify WF-checking so it can be cached

r? @michaelwoerister
@kennytm kennytm mentioned this pull request Mar 21, 2018
Closed
@kennytm kennytm mentioned this pull request Mar 22, 2018
Merged
kennytm added a commit to kennytm/rust that referenced this pull request Mar 22, 2018
@kennytm
Rollup merge of rust-lang#48939 - wesleywiser:incr_query_wf_checking,…
b7ee149 
… r=michaelwoerister

Querify WF-checking so it can be cached

r? @michaelwoerister
bors added a commit that referenced this pull request Mar 22, 2018
@bors
Auto merge of #49264 - kennytm:rollup, r=kennytm
5092c6b 
Rollup of 23 pull requests

- Successful merges: #48374, #48596, #48759, #48939, #49029, #49069, #49093, #49109, #49117, #49140, #49158, #49188, #49189, #49209, #49211, #49216, #49225, #49231, #49234, #49242, #49244, #49105, #49038
- Failed merges:
@alexcrichton alexcrichton merged commit b418b7b into rust-lang:master Mar 22, 2018
@michaelwoerister
Copy link
Member

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelwoerister michaelwoerister michaelwoerister left review comments

Assignees

@michaelwoerister michaelwoerister

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@wesleywiser @michaelwoerister @bors @Mark-Simulacrum @nikomatsakis @alexcrichton @rust-highfive