Pass more Copy types by value.
#72494
Conversation
rust-highfive
added
the
S-waiting-on-review
| @@ -1034,7 +1034,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> { | |||
|
|
|||
| Some(self.commit_if_ok(|snapshot| { | |||
| let (ty::SubtypePredicate { a_is_expected, a, b }, placeholder_map) = | |||
| self.replace_bound_vars_with_placeholders(predicate); | |||
| self.replace_bound_vars_with_placeholders(&predicate); | |||
There was a problem hiding this comment.
Somewhat related, I think it might be a good idea to change Binder to also require T: Copy and to move the reference inwards where this is not the case, i.e. &Binder(T) -> Binder(&T).
There was a problem hiding this comment.
The changes all seem reasonable to me. I guess cc @oli-obk or @eddyb on the final MIR commit, but it doesn't seem likely to be especially controversial. We've been slowly migrating over in the compiler to making more and more things Copy and, as we do, I think it makes sense to be shifting away from taking things via reference and towards taking them by value.
|
Added one more commit here which somewhat builds on the first one. This hopefully fixes most of the regression introduces in #72055 |
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit f15e4b3 with merge 493b2bfa42db10f469c41cd86a5a3dc9921299f2... |
|
☀️ Try build successful - checks-azure |
|
Queued 493b2bfa42db10f469c41cd86a5a3dc9921299f2 with parent 75b0a68, future comparison URL. |
This makes sense to me, although it's Still, I can it see it being nicer overall. |
|
Finished benchmarking try commit 493b2bfa42db10f469c41cd86a5a3dc9921299f2, comparison URL. |
|
The above comparison link does not work for me 😅 Tried to compare it to the current master, which should still be fairly representative: https://perf.rust-lang.org/compare.html?start=3137f8e2d141d7d7c65040a718a9193f50e1282e&end=493b2bfa42db10f469c41cd86a5a3dc9921299f2&stat=instructions%3Au This fixes slightly less than a third of the previous regression :/ |
|
If I didn't miss something, this PR should be ready for merge. |
|
@bors r+ |
|
📌 Commit f15e4b3 has been approved by |
bors
added
S-waiting-on-bors
|
@nikomatsakis This PR changes perf, so we probably should use @bors rollup=never here |
|
@lcnr: 🔑 Insufficient privileges: not in try users |
|
@bors rollup=never |
|
@bors p=1 |
|
☀️ Test successful - checks-azure |
|
Perf results from the landing show a small win. Nice work! |
There are a lot of locations where we pass
&T where T: Copyby reference,which should both be slightly less performant and less readable IMO.
This PR currently consists of three fairly self contained commits:
ty::Predicateby value and stops depending onAsRef<ty::Predicate>.<&List<_>>::into_iterto iterate over the elements by value. This would breakListsof non copy types. But as the only list constructor requires
Tto be copy anyways, I thinkthe improved readability is worth this potential future restriction.
mir::PlaceElemby value. Mir currently has quite a few copy types which are passed by reference, e.g.Local. As I don't have a lot of experience working with MIR, I mostly did this to get some feedback from people who use MIR more frequentlyty::Predicatein case it did not change in some places, which should hopefullyfix the regression caused by Intern predicates #72055
r? @nikomatsakis for the first commit, which continues the work of #72055 and makes adding
PredicateKind::ForAllslightly more pleasant. Feel free to reassign though