[incremental] Add support for eval always queries

[wesleywiser]

Part of #45238

[rust-highfive]

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

[wesleywiser]

r? @michaelwoerister

[michaelwoerister]

Looking good, except for the result fingerprinting being missing. Since we essentially disable dependency tracking for this tasks its even more important for them to be fingerprinted.

If you add the fingerprint, however, I guess there'll be a lot of code duplication between with_untracked_task and with_task. Maybe one can be implemented in the terms of the other.

I'm also a bit skeptical about the term untracked since it rather suggests the functionality that we already have with ignore.

[michaelwoerister]

You should be able to make the push and the pop methods here private.

[michaelwoerister]

Since this line is getting a little long, it would be more readable to have each arm on its own line.

[michaelwoerister]

Hm, it looks like implementing with_untracked_task in terms with_task gets rather contrived. I had hoped that we would be able to do something like:

pub fn with_xxx_task<C, A, R, HCX>(&self,
                                   key: DepNode,
                                   cx: C,
                                   arg: A,
                                   task: fn(C, A) -> R)
                                   -> (R, DepNodeIndex) 
{
    self.with_task(key, cx, arg, |cx, arg| {
        // Explicitly add the read to the Krate node
        self.read(DepNode::Krate);

        // Execute the task without recording any other reads
        self.with_ignore(|| {
            task(cx, arg)
        })     
    })
}

But with_task takes a function, not a closure, so we cannot capture the task parameter. If you can come up with a way to make this work nonetheless, that'd be nice. Then you wouldn't even need to modify CurrentDepGraph and OpenTask. Otherwise I suggest to make a generalized, private version of with_task -- which allows to specify somehow which push and pop methods it will invoke -- and call that with different parameters from with_task and with_untracked_task.

[wesleywiser]

Thanks for the review! What about something like "crate-wide query"? I'm also fine just changing it to "eval-always".

[michaelwoerister]

Yeah, let's stick to "eval-always". It's not a pretty name but it conveys the underlying concept well enough.

[wesleywiser]

@michaelwoerister I fixed up the commit per your feedback.

[michaelwoerister]

@wesleywiser Yes, that looks very good now! If you want to continue working on this, I suggest just appending to this PR.

[wesleywiser]

@michaelwoerister Thanks! Pushed three new commits.

[michaelwoerister]

Looks very good!

I'm seeing this error on travis:

thread 'rustc' panicked at 'assertion failed: !data.colors.borrow().contains_key(&key)', /checkout/src/librustc/dep_graph/graph.rs:220:12

This suggests that we are re-running queries although they've already been marked as green. This is because we are not doing the whole try_mark_green_and_read() spiel for eval-always.

Thinking about it some more, there should really be no reason to treat an eval-always query different from a regular one, except for the changes you already implemented in graph.rs. So instead of modifying try_get(), could you just do if dep_node.is_eval_always() {...} in force()?:

rust/src/librustc/ty/maps/plumbing.rs

Lines 433 to 442 in b247805

	profq_msg!(tcx, ProfileQueriesMsg::ProviderBegin);
	let res = tcx.cycle_check(span, Query::$name(key), || {
	tcx.sess.diagnostic().track_diagnostics(|| {
	tcx.dep_graph.with_task(dep_node,
	tcx,
	key,
	Self::compute_result)
	})
	})?;
	profq_msg!(tcx, ProfileQueriesMsg::ProviderEnd);

That should also not introduce any code duplication.

Let me know if you have any questions about this change of strategy.

[wesleywiser]

Hmmm... I can't seem to repro that. ./x.py test succeeds on my machine.

Regardless, I can certainly revert the changes to try_get_with() but I'm not entirely clear what you're proposing in regards to force(). Can you elaborate on that?

[michaelwoerister]

Hmmm... I can't seem to repro that. ./x.py test succeeds on my machine.

You are probably building with debug assertions disabled. I generally recommend setting debug-assertions = true in your config.toml. (and debuginfo-lines = true for better backtraces).

I imagine the updated force() method to look like this:

profq_msg!(tcx, ProfileQueriesMsg::ProviderBegin); 
let res = tcx.cycle_check(span, Query::$name(key), || { 
    tcx.sess.diagnostic().track_diagnostics(|| { 
        if dep_node.is_eval_always() {
            tcx.dep_graph.with_eval_always_task(dep_node, 
                                                tcx, 
                                                key, 
                                                Self::compute_result) 
        } else {
            tcx.dep_graph.with_task(dep_node, 
                                    tcx, 
                                    key, 
                                    Self::compute_result) 
        }
    }) 
})?; 
profq_msg!(tcx, ProfileQueriesMsg::ProviderEnd);

This way eval-always tasks are treated the same as regular with the only exception of what reads are recorded from them.

[wesleywiser]

Oh got it. That makes sense.

I generally recommend setting debug-assertions = true in your config.toml. (and debuginfo-lines = true for better backtraces).

Thanks! I'll do that.

[wesleywiser]

@michaelwoerister Done

[michaelwoerister]

Awesome, thank you so much @wesleywiser!

@bors r+

[bors]

📌 Commit 8281e88 has been approved by michaelwoerister

[bors]

⌛ Testing commit 8281e88 with merge 51456a6...

[bors]

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

[wesleywiser]

Thanks for all the help @michaelwoerister!!

[michaelwoerister]

You're welcome!