Skip to content

/rust

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

incr.comp.: Introduce `ensure` and `ensure` typeck_tables_of #45228

Merged
merged 4 commits into from Oct 15, 2017

Conversation

Reviewers

@nikomatsakis nikomatsakis

@michaelwoerister michaelwoerister

Assignees

@nikomatsakis nikomatsakis

Labels
S-waiting-on-bors
Projects
None yet
Milestone
No milestone
6 participants
@theotherjimmy @rust-highfive @michaelwoerister @bors @nikomatsakis @kennytm
@theotherjimmy
Copy link
Contributor

theotherjimmy commented Oct 12, 2017

Resolves #45210

In this Pull Request we introduce the ensure query/function. ensure has the
semantics and type of the function Q1 below:

fn Q1::ensure(K){
    Q(K);
}

Further, ensure avoids the need to load the result from disk (or execute the
provider, if we are not storing the results of Q to disk).

@nikomatsakis

theotherjimmy added some commits Oct 11, 2017

@theotherjimmy
Implement query ensure 
`$query::ensure()` guarantees that one of two things is true after it returns:
  - The query has all green inputs.
  - The query has been executed.
 and counts as a read from the source query
67b3e7c
@theotherjimmy
Ensure typeck_tables_of from typck_item_bodies 
This should make TypeckTables lazier.
Loading status checks…
bbdc613

@rust-highfive rust-highfive assigned nikomatsakis Oct 12, 2017

@rust-highfive

This comment has been minimized.

Sign in to view
Copy link
Collaborator

rust-highfive commented Oct 12, 2017

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.
@theotherjimmy

theotherjimmy reviewed Oct 12, 2017
View changes

src/librustc/ty/maps/plumbing.rs
if !tcx.dep_graph.is_fully_enabled() {
let _ = tcx.$name(key);
return;
}

This comment has been minimized.

Sign in to view
Copy link
@theotherjimmy

theotherjimmy Oct 12, 2017

Author Contributor

@nikomatsakis Open question: Should the preceding 4 lines be in try_mark_green?

This comment has been minimized.

Sign in to view
Copy link
@theotherjimmy

theotherjimmy Oct 12, 2017

Author Contributor

For reference, try_mark_green will unwrap a None here if tcx.dep_graph.is_fully_enabled() would return False.

This comment has been minimized.

Sign in to view
Copy link
@michaelwoerister

michaelwoerister Oct 12, 2017

Contributor

I'd rather keep the explicit panic in try_mark_green, I think.

This comment has been minimized.

Sign in to view
Copy link
@theotherjimmy

theotherjimmy Oct 12, 2017

Author Contributor

I'll leave the code here as it is then.

This comment has been minimized.

Sign in to view
Copy link
@nikomatsakis

nikomatsakis Oct 13, 2017

Contributor

I also expected those lines to be in try_mark_green, but I trust @michaelwoerister's intuitions here. =) That said, I think we could consider trying to extract some of the common code between this function and try_get_with. I had thought it might make sense to extract a helper function that returns Some(dep_node_index) if either: the node color is green or can be made green, and None otherwise. This function here could invoke tcx.$name(key) if None is returned, and try_get_with can do whatever it does now.

This comment has been minimized.

Sign in to view
Copy link
@theotherjimmy

theotherjimmy Oct 13, 2017

Author Contributor

I like it! I'll implement that refactor as a follow on PR, unless this round of testing does not go well and I have to fix something.

@theotherjimmy theotherjimmy referenced this pull request Oct 12, 2017

Open

[incremental] skip type-checking #45208

3 of 42 tasks complete
@michaelwoerister

michaelwoerister reviewed Oct 12, 2017
View changes

src/librustc/ty/maps/plumbing.rs Outdated
use dep_graph::DepNodeColor;
match tcx.dep_graph.node_color(&dep_node) {
Some(DepNodeColor::Green(dep_node_index)) => {
profq_msg!(tcx, ProfileQueriesMsg::CacheHit);

This comment has been minimized.

Sign in to view
Copy link
@michaelwoerister

michaelwoerister Oct 12, 2017

Contributor

We shouldn't do any profq_msg! calls in this method for now. We'll have to talk to @matthewhammer at some point about how to properly integrate the new tracking system with the query profiler.

This comment has been minimized.

Sign in to view
Copy link
@theotherjimmy

theotherjimmy Oct 12, 2017

Author Contributor

I'll pull them out. Thanks.
@michaelwoerister

michaelwoerister reviewed Oct 12, 2017
View changes

src/librustc/ty/maps/plumbing.rs Outdated
tcx.dep_graph.read_index(dep_node_index);
}
Some(DepNodeColor::Red) => {
let _ = tcx.$name(key);

This comment has been minimized.

Sign in to view
Copy link
@michaelwoerister

michaelwoerister Oct 12, 2017

Contributor

Could you add a comment here saying something like:

The DepNode being DepNodeColor::Red means that this query has been already executed before. We are still calling the proper again in order to issue the appropriate dep_graph.read() call. The performance cost this introduces should be negligible because we'll immediately hit the in-memory cache.

This comment has been minimized.

Sign in to view
Copy link
@theotherjimmy

theotherjimmy Oct 12, 2017

Author Contributor

Yes. Just a moment.
@theotherjimmy

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

theotherjimmy commented Oct 12, 2017

@michaelwoerister I think I corrected everything you pointed out in your review comments. Thanks for the review!
@michaelwoerister

This comment has been minimized.

Sign in to view
Copy link
Contributor

michaelwoerister commented Oct 12, 2017

Thanks @theotherjimmy!

@bors r+
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Oct 12, 2017

📌 Commit d0cb4d0 has been approved by michaelwoerister

@kennytm kennytm added the S-waiting-on-bors label Oct 13, 2017

@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Oct 15, 2017

⌛️ Testing commit d0cb4d0 with merge 2689fd2...

bors added a commit that referenced this pull request Oct 15, 2017

@bors
Auto merge of #45228 - theotherjimmy:ensure-query, r=michaelwoerister 
incr.comp.: Introduce `ensure` and `ensure` typeck_tables_of

Resolves #45210

In this Pull Request we introduce the `ensure` query/function. `ensure` has the
semantics and type of the function `Q1` below:
```rust
fn Q1::ensure(K){
    Q(K);
}
```
Further, `ensure` avoids the need to load the result from disk (or execute the
provider, if we are not storing the results of Q to disk).

@nikomatsakis
Loading status checks…
2689fd2

@bors bors referenced this pull request Oct 15, 2017

Merged

Add "Buidling on *nix" sub-header #45293

@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Oct 15, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing 2689fd2 to master...

@bors bors merged commit d0cb4d0 into rust-lang:master Oct 15, 2017
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@theotherjimmy theotherjimmy referenced this pull request Oct 15, 2017

Merged

Refactor `ensure` and `try_get_with` #45312

@michaelwoerister

This comment has been minimized.

Sign in to view
Copy link
Contributor

michaelwoerister commented Oct 16, 2017

🎉

bors added a commit that referenced this pull request Oct 19, 2017

@bors
Auto merge of #45312 - theotherjimmy:refactor-ensure, r=michaelwoerister 
Refactor `ensure` and `try_get_with` into `read_node_index`

There was a bit of code shared between `try_get_with` and `ensure`, after I
added `ensure`. I refactored that shared code into a query-agnostic method
called `read_node_index`.

The new method `read_node_index` will attempt to find the node
index (`DepNodeIndex`) of a query. When `read_node_index` finds the
`DepNodeIndex`, it marks the current query as a reader of the node it's
requesting the index of.

This is used by `try_get_with` and `ensure` as it elides the unimportant (to
them) details of if the query is invalidated by previous changed computation (Red)
or new and if they had to mark the query green. For both `try_get_with` and
`ensure`, they just need to know if they can lookup the results or have to
reevaluate.

@nikomatsakis this is the [refactor we discussed](#45228 (comment)) in the comment thread of #45228
Loading status checks…
07a3441

bors added a commit that referenced this pull request Oct 20, 2017

@bors
Auto merge of #45312 - theotherjimmy:refactor-ensure, r=michaelwoerister 
Refactor `ensure` and `try_get_with`

There was a bit of code shared between `try_get_with` and `ensure`, after I
added `ensure`. I refactored that shared code into a query-agnostic method
called `read_node_index`.

The new method `read_node_index` will attempt to find the node
index (`DepNodeIndex`) of a query. When `read_node_index` finds the
`DepNodeIndex`, it marks the current query as a reader of the node it's
requesting the index of.

This is used by `try_get_with` and `ensure` as it elides the unimportant (to
them) details of if the query is invalidated by previous changed computation (Red)
or new and if they had to mark the query green. For both `try_get_with` and
`ensure`, they just need to know if they can lookup the results or have to
reevaluate.

@nikomatsakis this is the [refactor we discussed](#45228 (comment)) in the comment thread of #45228
Loading status checks…
354eb16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.