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.

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

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

Merged
merged 1 commit into from
Dec 11, 2019

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Dec 2, 2019

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 2, 2019
@eddyb 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
@eddyb eddyb force-pushed the choose-what-you-reveal-carefully branch from 59d8369 to 1d1298e Compare December 2, 2019 16:27
@eddyb
Copy link
Member Author

eddyb commented Dec 2, 2019

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

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Dec 2, 2019

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

bors added a commit that referenced this pull request Dec 2, 2019
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
Copy link
Contributor

bors commented Dec 2, 2019

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

@rust-timer
Copy link
Collaborator

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

@eddyb
Copy link
Member Author

eddyb commented Dec 2, 2019

@craterbot check

@craterbot
Copy link
Collaborator

👌 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

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

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

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.

@@ -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());
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@eddyb eddyb Dec 9, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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).

@@ -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)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Collaborator

Finished benchmarking try commit 7cb06892a23b4da510686618c8730f0b2e8bd952, comparison URL.

@eddyb
Copy link
Member Author

eddyb commented Dec 3, 2019

The perf regression could be from one of two things:

  • more data to hash/compare in the cache keys
  • 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
Copy link
Collaborator

🚧 Experiment pr-66963 is now running

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

@nikomatsakis
Copy link
Contributor

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

image

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

@craterbot
Copy link
Collaborator

🎉 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

@craterbot 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
@Centril
Copy link
Contributor

Centril commented Dec 6, 2019

Regression is spurious.

@nikomatsakis
Copy link
Contributor

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
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Dec 10, 2019

📌 Commit 1d1298e has been approved by nikomatsakis

@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 Dec 10, 2019
@nikomatsakis
Copy link
Contributor

@bors r-

@bors 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
@nikomatsakis
Copy link
Contributor

Actually, let's land this together with #66821

bors added a commit that referenced this pull request Dec 11, 2019
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 bors merged commit 1d1298e into rust-lang:master Dec 11, 2019
@eddyb eddyb deleted the choose-what-you-reveal-carefully branch December 11, 2019 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants