rustc: include ParamEnv in global trait select/eval cache keys. #66963

eddyb · 2019-12-02T15:50:49Z

Without #66821, this doesn't cache more than before, but it does include Reveal in the trait select/eval cache keys (via ParamEnv), which is what caused the test failures in #66821.

IIUC, this is a bug fix, because before this PR, cached select/eval results:

if Reveal::UserFacing, could limit what Reveal::All sees (and cause ICEs?)
if Reveal::All, could expose more to Reveal::UserFacing than intended

The second case is the worse one IMO, I just hope we don't find people relying on it, on crater.

r? @nikomatsakis cc @rust-lang/compiler

eddyb · 2019-12-02T16:27:59Z

@bors try @rust-timer queue (I don't want to cause a perf regression)

rust-timer · 2019-12-02T16:28:00Z

Awaiting bors try build completion

bors · 2019-12-02T16:28:10Z

⌛ Trying commit 1d1298e with merge 7cb06892a23b4da510686618c8730f0b2e8bd952...

@nikomatsakis

rustc: allow non-empty ParamEnv's in global trait select/eval caches. *Based on #66963* This appears to alleviate the symptoms of #65510 locally (without fixing WF directly), and is potentially easier to validate as sound (since it's a more ad-hoc version of queries we already have). I'm opening this PR primarily to test the effects on perf. r? @nikomatsakis cc @rust-lang/wg-traits

bors · 2019-12-02T19:04:03Z

☀️ Try build successful - checks-azure
Build commit: 7cb06892a23b4da510686618c8730f0b2e8bd952 (7cb06892a23b4da510686618c8730f0b2e8bd952)

rust-timer · 2019-12-02T19:04:05Z

Queued 7cb06892a23b4da510686618c8730f0b2e8bd952 with parent 2da942f, future comparison URL.

eddyb · 2019-12-02T19:06:35Z

@craterbot check

craterbot · 2019-12-02T19:06:37Z

👌 Experiment pr-66963 created and queued.
🤖 Automatically detected try build 7cb06892a23b4da510686618c8730f0b2e8bd952
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

nikomatsakis

I agree this is likely a bug fix and seems pretty reasonable. I'm curious to see the performance impact. I'm also curious to understand the behavior of the test case.

nikomatsakis · 2019-12-02T23:11:05Z

src/librustc/traits/project.rs

@@ -1079,12 +1079,10 @@ fn assemble_candidates_from_impls<'cx, 'tcx>(
                if !is_default {
                    true
                } else if obligation.param_env.reveal == Reveal::All {
-                    debug_assert!(!poly_trait_ref.needs_infer());


To be clear, did you see this debug assertion fail?

Yes, in a run-pass specialization test, as I assume this can happen inside a temporary inference context inside a normalization query.

This little bit of code is making me nervous. I'm wondering if we might get into some scenario where we have an unresolved inference variable and so we opt not to reveal the item. I think this would mean we normalize to (e.g.) <T as Foo<?X>>::Bar. But then later the inference variable ?X gets resolved in such a way that we could have revealed the value. This could lead to an inconsistency that causes some ICEs or something.

I think I would feel better if we flagged cases that involve inference variables as 'ambiguous'. I have to remember how that is done in this code, I can check if it has any impact.

Still, I could probably be persuaded to land it as is.

I'm pretty sure not revealing is "ambiguous", to the extent that projection would be.
Doesn't ambiguity result in not making progress, in general?

This could lead to an inconsistency that causes some ICEs or something.

Wouldn't that same situation apply to non-specialized impls when e.g. there's a Foo<A> impl and a Foo<B> one and so ?X being resolved later to one of them would allow revealing Bar?

I don't mind ICEs caused indirectly by this, considering this was even more ICE-y before, but also keep in mind this requires specialization, which is still unstable.

Huh, looks like the assert was added by... me, in #35091, and it assumed that only monomorphic codegen would use the Reveal::All codepath (I was curious and dug through the history).

nikomatsakis · 2019-12-02T23:28:22Z

src/librustc/traits/select.rs

@@ -1143,15 +1148,15 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
        let tcx = self.tcx();
        if self.can_use_global_caches(param_env) {
            let cache = tcx.evaluation_cache.hashmap.borrow();
-            if let Some(cached) = cache.get(&trait_ref) {
+            if let Some(cached) = cache.get(&param_env.and(trait_ref)) {


ok, so, in this case the only purpose is to capture the reveal mode, correct?

Yes, I can change to be literally that if e.g. it's a perf problem.

I wouldn't expect that to have a perf impact, but might be worth including in a comment somewhere.

nikomatsakis · 2019-12-02T23:31:44Z

src/test/ui/type-alias-impl-trait/bound_reduction2.stderr

@@ -1,5 +1,14 @@
+error[E0277]: the trait bound `T: TraitWithAssoc` is not satisfied


Huh, this error is definitely not great. Do you understand what's going on here? (I don't, at least not yet)

I think this is because T ends up in the context of the impl Trait which doesn't have the parameter nor the relevant bound in its ParamEnv.
It's not great but this is a pre-existing bug in the existential type aliases feature that was being hidden because the Reveal mode was getting ignored after the first time.

rust-timer · 2019-12-03T00:24:21Z

Finished benchmarking try commit 7cb06892a23b4da510686618c8730f0b2e8bd952, comparison URL.

eddyb · 2019-12-03T09:55:16Z

The perf regression could be from one of two things:

more data to hash/compare in the cache keys
- including the caller_bounds field of ParamEnv, even if it's always empty
- we could just use (Reveal, T) instead of ParamEnvAnd<T>
  - however, if we do end up landing rustc: allow non-empty ParamEnv's in global trait select/eval caches. #66821, it will go back to ParamEnvAnd<T>
more cache misses, because the Reveal mode doesn't match
- there's a potential correlation here between how much perf regressed and how many cache entries could've been tainted (but "most of the time" it was probably fine)
- cc @wesleywiser could we track global trait select/eval in -Z self-profile?

craterbot · 2019-12-04T06:55:17Z

🚧 Experiment pr-66963 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

nikomatsakis · 2019-12-05T13:37:22Z

@eddyb well the perf impact looks fairly small to me:

Not sure if it's worth worrying about, especially if we expect some improvements down the line.

craterbot · 2019-12-06T22:45:52Z

🎉 Experiment pr-66963 is completed!
📊 1 regressed and 0 fixed (81904 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Centril · 2019-12-06T23:31:34Z

Regression is spurious.

nikomatsakis · 2019-12-10T22:59:06Z

OK, so, I ran an experiment where I made trait projection "ambiguous" when there were unresolved inference variables. I ran into an ICE in the issue-38091.rs test. I didn't dig too deeply into it, but the ambiguous result was coming from an attempt to resolve <i32 as Iterate<'?x>>::Ty. This is an interesting case because the inference variable here ('?x) is a lifetime and hence shouldn't really be relevant.

It's also interesting why it gets an ICE. The ICE occurred because we were never able to definitely resolve the obligation, I think because we would never generate any constraint on '?x (presumably lifetimes were erased at this point? Maybe I should dig a bit deeper...).

What happens on this branch, I think, is that we ignore the impl, and hence we normalize to <T as Iterate<'a>>::Ty (i.e., the "placeholder", in Chalk terms). This doesn't seem wrong -- in this particular case, since al we have to prove is Valid, and we are able to do so, it all works out. I was not able to construct an example where we got an ICE because this didn't work out. One other thing worth noting is that all of this is specific to uses of specialization.

OK, I'm inclined to land this then, because:

there may be some danger of getting an obscure ICE, but I don't think we'll do anything wrong
the crater run suggests this doesn't happen in practice, it may not even be possible
all of this affects only specialization anyhow, and I hope that we can transition to a more robust strategy here in any case eventually (i.e., chalk)

nikomatsakis · 2019-12-10T22:59:15Z

@bors r+

bors · 2019-12-10T22:59:16Z

📌 Commit 1d1298e has been approved by nikomatsakis

nikomatsakis · 2019-12-10T23:01:24Z

@bors r-

nikomatsakis · 2019-12-10T23:01:45Z

Actually, let's land this together with #66821

@nikomatsakis

rustc: allow non-empty ParamEnv's in global trait select/eval caches. *Based on #66963* This appears to alleviate the symptoms of #65510 locally (without fixing WF directly), and is potentially easier to validate as sound (since it's a more ad-hoc version of queries we already have). I'm opening this PR primarily to test the effects on perf. r? @nikomatsakis cc @rust-lang/wg-traits

rust-highfive assigned nikomatsakis Dec 2, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 2, 2019

eddyb added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 2, 2019

rustc: include ParamEnv in global trait select/eval cache keys.

1d1298e

eddyb force-pushed the choose-what-you-reveal-carefully branch from 59d8369 to 1d1298e Compare December 2, 2019 16:27

eddyb mentioned this pull request Dec 2, 2019

rustc: allow non-empty ParamEnv's in global trait select/eval caches. #66821

Merged

craterbot added the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Dec 2, 2019

nikomatsakis reviewed Dec 2, 2019

View reviewed changes

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Dec 6, 2019

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 Dec 10, 2019

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

bors merged commit 1d1298e into rust-lang:master Dec 11, 2019

eddyb deleted the choose-what-you-reveal-carefully branch December 11, 2019 17:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustc: include ParamEnv in global trait select/eval cache keys. #66963

rustc: include ParamEnv in global trait select/eval cache keys. #66963

eddyb commented Dec 2, 2019 •

edited

eddyb commented Dec 2, 2019

rust-timer commented Dec 2, 2019

bors commented Dec 2, 2019

bors commented Dec 2, 2019

rust-timer commented Dec 2, 2019

eddyb commented Dec 2, 2019

craterbot commented Dec 2, 2019

nikomatsakis left a comment

nikomatsakis Dec 2, 2019

eddyb Dec 2, 2019

nikomatsakis Dec 9, 2019

eddyb Dec 9, 2019 •

edited

eddyb Dec 9, 2019

nikomatsakis Dec 2, 2019

eddyb Dec 2, 2019

nikomatsakis Dec 5, 2019

nikomatsakis Dec 2, 2019

eddyb Dec 2, 2019

rust-timer commented Dec 3, 2019

eddyb commented Dec 3, 2019 •

edited

craterbot commented Dec 4, 2019

nikomatsakis commented Dec 5, 2019

craterbot commented Dec 6, 2019

Centril commented Dec 6, 2019

nikomatsakis commented Dec 10, 2019

nikomatsakis commented Dec 10, 2019

bors commented Dec 10, 2019

nikomatsakis commented Dec 10, 2019

nikomatsakis commented Dec 10, 2019

		@@ -1,5 +1,14 @@
		error[E0277]: the trait bound `T: TraitWithAssoc` is not satisfied

rustc: include ParamEnv in global trait select/eval cache keys. #66963

rustc: include ParamEnv in global trait select/eval cache keys. #66963

Conversation

eddyb commented Dec 2, 2019 • edited

eddyb commented Dec 2, 2019

rust-timer commented Dec 2, 2019

bors commented Dec 2, 2019

bors commented Dec 2, 2019

rust-timer commented Dec 2, 2019

eddyb commented Dec 2, 2019

craterbot commented Dec 2, 2019

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb Dec 9, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rust-timer commented Dec 3, 2019

eddyb commented Dec 3, 2019 • edited

craterbot commented Dec 4, 2019

nikomatsakis commented Dec 5, 2019

craterbot commented Dec 6, 2019

Centril commented Dec 6, 2019

nikomatsakis commented Dec 10, 2019

nikomatsakis commented Dec 10, 2019

bors commented Dec 10, 2019

nikomatsakis commented Dec 10, 2019

nikomatsakis commented Dec 10, 2019

eddyb commented Dec 2, 2019 •

edited

eddyb Dec 9, 2019 •

edited

eddyb commented Dec 3, 2019 •

edited