Improve fast rejection. #97136
Improve fast rejection. #97136
Conversation
rustbot
added
the
T-compiler
rust-highfive
added
the
S-waiting-on-review
|
Currently, this has the smallest possible change that helps
|
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit 0374729b9ba6b4f7f054d3adeb9c62ed5634c0d5 with merge 75978414bb55092c36addd0a84ead8b1c19d82f6... |
|
☀️ Try build successful - checks-actions |
|
Queued 75978414bb55092c36addd0a84ead8b1c19d82f6 with parent 77972d2, future comparison URL. |
|
Finished benchmarking commit (75978414bb55092c36addd0a84ead8b1c19d82f6): comparison url. Summary:
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. @bors rollup=never Footnotes |
rustbot
removed
the
S-waiting-on-perf
In particular, for `bitmaps-3.1.0` which does a huge number of comparisons between `BitsImpl<M>` and `BitsImpl<N>` where `M` and `N` are const integers.
nnethercote
force-pushed
the
improve-fast-rejection
branch
from
0374729 to
af68005
Compare
|
@lcnr: I have updated the code to avoid As for the |
| @@ -214,6 +213,122 @@ enum BuiltinImplConditions<'tcx> { | |||
| Ambiguous, | |||
| } | |||
|
|
|||
| fn tys_may_be_equivalent<'tcx>(oblig_ty: Ty<'tcx>, impl_ty: Ty<'tcx>) -> bool { | |||
There was a problem hiding this comment.
please move this into the fast_reject module
| // First, look for obvious equivalences, e.g. the types are equal. The most | ||
| // common case is to find such an equivalence trivially, e.g. `ty::Bool` | ||
| // and `ty::Bool`. | ||
| let mut maybe_equiv = match oblig_ty.kind() { |
There was a problem hiding this comment.
I prefer one large match (oblig_ty.kind(), impl_ty.kind()) here, starting with
(_, ty::Param(_) | ty::Projection(_) | ty::Error(_)) => true,There was a problem hiding this comment.
This is what I was referring to when I said I chose to structure it with exhaustive matches. Using _ here makes it easy to overlook updating this when new variants are added to Ty.
| substs1.len() == 1 | ||
| && substs2.len() == 1 | ||
| && let ty::subst::GenericArgKind::Const(c1) = substs1[0].unpack() | ||
| && let ty::subst::GenericArgKind::Const(c2) = substs2[0].unpack() | ||
| && let Some(s1) = c1.val().try_to_scalar_int() | ||
| && let Some(s2) = c2.val().try_to_scalar_int() | ||
| && s1 != s2 |
There was a problem hiding this comment.
please iterate over all generic arguments here.
For types, you can again call tys_may_be_equalivalent while for constant you can also return false for ConstKind::Param on the lhs and anything apart from Param | Unevaluated on the rhs.
I think it makes sense to add fn generic_arg_may_be_equivalent for that
| _ => false, | ||
| }, | ||
| ty::Str => matches!(k, ty::Str), | ||
| ty::Array(..) => matches!(k, ty::Array(..)), |
There was a problem hiding this comment.
you can recurse into the element type and length here
| }, | ||
| ty::Str => matches!(k, ty::Str), | ||
| ty::Array(..) => matches!(k, ty::Array(..)), | ||
| ty::Slice(..) => matches!(k, ty::Slice(..)), |
| trait_info.principal_def_id() == trait_info2.principal_def_id() | ||
| ) | ||
| } | ||
| ty::Ref(_, _, mutbl) => matches!(k, ty::Ref(_, _, mutbl2) if mutbl == mutbl2), |
| ) | ||
| } | ||
| ty::Ref(_, _, mutbl) => matches!(k, ty::Ref(_, _, mutbl2) if mutbl == mutbl2), | ||
| ty::FnDef(def_id, _) => matches!(k, ty::FnDef(def_id2, _) if def_id == def_id2), |
| ty::Param(_) => { | ||
| // Note, we simplify parameters for the obligation but not the impl | ||
| // so that we do not reject a blanket impl but do reject more | ||
| // concrete impls if we're searching for `T: Trait`. | ||
| matches!(k, ty::Placeholder(..)) | ||
| } |
There was a problem hiding this comment.
this can just be
(ty::Param(_), _) => false,parameters only unify with themselves (or inference vars/projections which get normalized/inferred to that param)
| ty::Projection(_) => { | ||
| // When treating `ty::Param` as a placeholder, projections also | ||
| // don't unify with anything else as long as they are fully normalized. | ||
| // | ||
| // We will have to be careful with lazy normalization here. | ||
| oblig_ty.has_infer_types_or_consts() || matches!(k, ty::Placeholder(..)) | ||
| } |
There was a problem hiding this comment.
same here
(ty::Projection(_), _) => oblig_ty.has_infer_types_or_consts(),
| } | ||
| ty::Infer(_) => true, | ||
|
|
||
| ty::GeneratorWitness(..) | ty::Placeholder(..) | ty::Bound(..) | ty::Error(_) => { |
There was a problem hiding this comment.
surprised that ty::Placeholder is unreachable here, that variant should be reachable afaik
`match_impl` has two call sites. For one of them (within `rematch_impl`) the fast reject test isn't necessary, because any rejection would represent a compiler bug. This commit moves the fast reject test to the other `match_impl` call site, in `assemble_candidates_from_impls`. This lets us move the fast reject test outside the `probe` call in that function. This avoids the taking of useless snapshots when the fast reject test succeeds, which gives a performance win when compiling the `bitmaps` and `nalgebra` crates.
|
@lcnr: thanks for the comments. It's late here, I will get to them tomorrow. But note that I've added a new commit that gives an additional performance boost. |
It'll likely be faster that way, I'm fine if you want to take over. |
|
This has been superseded by #97166. |
add a deep fast_reject routine continues the work on rust-lang#97136. r? `@nnethercote` Actually agree with you on the match structure 😆 let's see how that impacted perf 😅
In particular, for
bitmaps-3.1.0which does a huge number ofcomparisons between
BitsImpl<M>andBitsImpl<N>whereMandNare const integers.
r? @lcnr