Skip to content

Only disable cache if predicate has opaques within it #132625

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 7, 2024
Merged

Only disable cache if predicate has opaques within it #132625

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 18 additions & 6 deletions compiler/rustc_trait_selection/src/traits/select/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1310,7 +1310,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
trait_pred: ty::PolyTraitPredicate<'tcx>,
) -> Option<EvaluationResult> {
let tcx = self.tcx();
if self.can_use_global_caches(param_env) {
if self.can_use_global_caches(param_env, trait_pred) {
if let Some(res) = tcx.evaluation_cache.get(&(param_env, trait_pred), tcx) {
return Some(res);
}
Expand All @@ -1331,7 +1331,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
return;
}

if self.can_use_global_caches(param_env) && !trait_pred.has_infer() {
if self.can_use_global_caches(param_env, trait_pred) && !trait_pred.has_infer() {
debug!(?trait_pred, ?result, "insert_evaluation_cache global");
// This may overwrite the cache with the same value
// FIXME: Due to #50507 this overwrites the different values
Expand Down Expand Up @@ -1476,7 +1476,11 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
}

/// Returns `true` if the global caches can be used.
fn can_use_global_caches(&self, param_env: ty::ParamEnv<'tcx>) -> bool {
fn can_use_global_caches(
&self,
param_env: ty::ParamEnv<'tcx>,
pred: ty::PolyTraitPredicate<'tcx>,
) -> bool {
// If there are any inference variables in the `ParamEnv`, then we
// always use a cache local to this particular scope. Otherwise, we
// switch to a global cache.
Expand All @@ -1494,7 +1498,15 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
TypingMode::Coherence => false,
// Avoid using the global cache when we're defining opaque types
// as their hidden type may impact the result of candidate selection.
TypingMode::Analysis { defining_opaque_types } => defining_opaque_types.is_empty(),
//
// HACK: This is still theoretically unsound. Goals can indirectly rely
// on opaques in the defining scope, and it's easier to do so with TAIT.
// However, if we disqualify *all* goals from being cached, perf suffers.
// This is likely fixed by better caching in general in the new solver.
// See: <https://github.com/rust-lang/rust/issues/132064>.
TypingMode::Analysis { defining_opaque_types } => {
defining_opaque_types.is_empty() || !pred.has_opaque_types()
Copy link
Contributor

@lcnr lcnr Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

given that we use can_use_global_caches both on access and on write, could we change this to instead check that pred does not contain an opaque from defining_opaque_types, i.e. it only contains rigid opaque types

On stable pretty much the only time you observe opaque types in their defining scope it for recursive RPIT, so I expect the slowdown to be caused by this triggering for opaques we can't actually define rn

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 actually tested that and it doesn't improve perf very much lol

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah perf only goes from 4:07 to 4:04 when i implement a simple visitor to check if it contains an opaque in the defines list

Copy link
Contributor

@lcnr lcnr Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

yeah perf only goes from 4:07 to 4:04 when i implement a simple visitor to check if it contains an opaque in the defines list

that seems weird to me 🤔 what's causing the regression here, reuse between different MIR borrowck queries?

}
// The global cache is only used if there are no opaque types in
// the defining scope or we're outside of analysis.
//
Expand All @@ -1512,7 +1524,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
let tcx = self.tcx();
let pred = cache_fresh_trait_pred.skip_binder();

if self.can_use_global_caches(param_env) {
if self.can_use_global_caches(param_env, cache_fresh_trait_pred) {
if let Some(res) = tcx.selection_cache.get(&(param_env, pred), tcx) {
return Some(res);
}
Expand Down Expand Up @@ -1562,7 +1574,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
return;
}

if self.can_use_global_caches(param_env) {
if self.can_use_global_caches(param_env, cache_fresh_trait_pred) {
if let Err(Overflow(OverflowError::Canonical)) = candidate {
// Don't cache overflow globally; we only produce this in certain modes.
} else if !pred.has_infer() && !candidate.has_infer() {
Expand Down
Loading