incr.comp.: Introduce `ensure` and `ensure` typeck_tables_of by theotherjimmy · Pull Request #45228 · rust-lang/rust

theotherjimmy · 2017-10-12T03:29:11Z

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

`$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

This should make TypeckTables lazier.

rust-highfive · 2017-10-12T03:29:25Z

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 · 2017-10-12T03:30:22Z

src/librustc/ty/maps/plumbing.rs

+                        if !tcx.dep_graph.is_fully_enabled() {
+                            let _ = tcx.$name(key);
+                            return;
+                        }


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

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

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

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

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.

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.

michaelwoerister · 2017-10-12T07:51:20Z

src/librustc/ty/maps/plumbing.rs

+                use dep_graph::DepNodeColor;
+                match tcx.dep_graph.node_color(&dep_node) {
+                    Some(DepNodeColor::Green(dep_node_index)) => {
+                        profq_msg!(tcx, ProfileQueriesMsg::CacheHit);


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.

I'll pull them out. Thanks.

michaelwoerister · 2017-10-12T08:00:22Z

src/librustc/ty/maps/plumbing.rs

+                        tcx.dep_graph.read_index(dep_node_index);
+                    }
+                    Some(DepNodeColor::Red) => {
+                        let _ = tcx.$name(key);


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.

Yes. Just a moment.

theotherjimmy · 2017-10-12T13:55:57Z

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

michaelwoerister · 2017-10-12T14:54:15Z

Thanks @theotherjimmy!

@bors r+

bors · 2017-10-12T14:54:16Z

📌 Commit d0cb4d0 has been approved by michaelwoerister

bors · 2017-10-15T14:00:45Z

⌛ Testing commit d0cb4d0 with merge 2689fd2...

@nikomatsakis

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

bors · 2017-10-15T16:27:30Z

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

michaelwoerister · 2017-10-16T09:09:09Z

🎉

@nikomatsakis

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

@nikomatsakis

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

theotherjimmy added 2 commits October 11, 2017 22:08

Implement query ensure

67b3e7c

`$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

Ensure typeck_tables_of from typck_item_bodies

bbdc613

This should make TypeckTables lazier.

rust-highfive assigned nikomatsakis Oct 12, 2017

theotherjimmy commented Oct 12, 2017

View reviewed changes

theotherjimmy mentioned this pull request Oct 12, 2017

[incremental] skip type-checking #45208

Open

42 tasks

michaelwoerister reviewed Oct 12, 2017

View reviewed changes

theotherjimmy added 2 commits October 12, 2017 08:48

Remove query profiling from ensure

f03af1b

Explain why ensure of a red node == the query

d0cb4d0

kennytm added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 13, 2017

bors mentioned this pull request Oct 15, 2017

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

Merged

bors merged commit d0cb4d0 into rust-lang:master Oct 15, 2017

theotherjimmy mentioned this pull request Oct 15, 2017

Refactor ensure and try_get_with #45312

Merged

Zalathar mentioned this pull request Mar 7, 2026

Don't define ensure_ok and ensure_done for anon queries #153474

Open

Uh oh!

Conversation

theotherjimmy commented Oct 12, 2017

Uh oh!

rust-highfive commented Oct 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theotherjimmy commented Oct 12, 2017

Uh oh!

michaelwoerister commented Oct 12, 2017

Uh oh!

bors commented Oct 12, 2017

Uh oh!

bors commented Oct 15, 2017

Uh oh!

bors commented Oct 15, 2017

Uh oh!

michaelwoerister commented Oct 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants