Uh oh!
There was an error while loading. Please reload this page.
Pass more Copy types by value. - #72494
Conversation
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).
nikomatsakis
left a comment
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.
lcnr
commented
May 23, 2020
Added one more commit here which somewhat builds on the first one. This hopefully fixes most of the regression introduces in #72055 |
Dylan-DPC-zz
commented
May 23, 2020
@bors try @rust-timer queue |
rust-timer
commented
May 23, 2020
Awaiting bors try build completion |
bors
commented
May 23, 2020
⌛ Trying commit f15e4b3 with merge 493b2bfa42db10f469c41cd86a5a3dc9921299f2... |
bors
commented
May 23, 2020
☀️ Try build successful - checks-azure |
rust-timer
commented
May 23, 2020
Queued 493b2bfa42db10f469c41cd86a5a3dc9921299f2 with parent 75b0a68, future comparison URL. |
eddyb
commented
May 23, 2020
This makes sense to me, although it's Still, I can it see it being nicer overall. |
rust-timer
commented
May 24, 2020
Finished benchmarking try commit 493b2bfa42db10f469c41cd86a5a3dc9921299f2, comparison URL. |
lcnr
commented
May 24, 2020
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 :/ |
lcnr
commented
May 25, 2020
If I didn't miss something, this PR should be ready for merge. |
nikomatsakis
commented
May 26, 2020
@bors r+ |
bors
commented
May 26, 2020
📌 Commit f15e4b3 has been approved by |
lcnr
commented
May 26, 2020
@nikomatsakis This PR changes perf, so we probably should use @bors rollup=never here |
bors
commented
May 26, 2020
@lcnr: 🔑 Insufficient privileges: not in try users |
nikomatsakis
commented
May 27, 2020
@bors rollup=never |
Dylan-DPC-zz
commented
May 27, 2020
@bors p=1 |
bors
commented
May 28, 2020
bors
commented
May 28, 2020
☀️ Test successful - checks-azure |
nnethercote
commented
Jun 1, 2020
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