Prefer sort_unstable*() over sort*() #52672
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
src/librustc_driver/profile/trace.rs
Outdated
| data.sort_by(|&(_,_,_,self1),&(_,_,_,self2)| | ||
| if self1 > self2 { Ordering::Less } else { Ordering::Greater } ); | ||
| data.sort_unstable_by(|&(_,_,_,self1),&(_,_,_,self2)| | ||
| if self1 > self2 { Ordering::Less } else { Ordering::Greater } ); |
There was a problem hiding this comment.
This one could be data.sort_unstable_by_key(|k| Reverse(&k.3)), which also handles the case self1 == self2 correctly
There was a problem hiding this comment.
The compiler seems to prefer data.sort_unstable_by_key(|&k| Reverse(k.3)). Looks like a readability win, I'll be happy to squash it in if the PR is accepted.
|
Are you sure that an unstable sorting keeps the code semantics correct in every one of those cases? |
|
@leonardo-m I ran |
|
I'd like to ensure the CI to give a ✅ before perf. |
|
Travis is green. |
Prefer sort_unstable*() over sort*() Since `sort_unstable` is considered typically faster than the regular `sort` ([benchmarks](#40601 (comment))), it might be a good idea to check for potential wins in the compiler.
|
☀️ Test successful - status-travis |
|
@rust-timer build 90ec471 |
|
Success: Queued 90ec471 with parent f498e4e, comparison URL. |
src/librustc/ty/layout.rs
Outdated
| @@ -288,15 +288,15 @@ impl<'a, 'tcx> LayoutCx<'tcx, TyCtxt<'a, 'tcx, 'tcx>> { | |||
| match kind { | |||
| StructKind::AlwaysSized | | |||
| StructKind::MaybeUnsized => { | |||
| optimizing.sort_by_key(|&x| { | |||
| optimizing.sort_unstable_by_key(|&x| { | |||
| // Place ZSTs first to avoid "interesting offsets", | |||
| // especially with only one or two non-ZST fields. | |||
There was a problem hiding this comment.
Struct layout not being stable sounds scary, even if technically allowed. I never want "my code got faster/slower because I added another ZST" to be something that happens.
src/librustc_driver/lib.rs
Outdated
| lints.sort_by(|&(x, _): &(&'static str, Vec<lint::LintId>), | ||
| &(y, _): &(&'static str, Vec<lint::LintId>)| { | ||
| lints.sort_unstable_by(|&(x, _): &(&'static str, Vec<lint::LintId>), | ||
| &(y, _): &(&'static str, Vec<lint::LintId>)| { |
There was a problem hiding this comment.
nit: looks like this could be _by_key too?
|
Perf is ready. Heh, some checks have even become slower. No big impact in total. |
|
Yeah, it looks like there is no added value to changing to an unstable sort on this scale. I'll close this PR and file another one with the readability improvements. |
|
Looks like some |
|
Those wins are minor, but they seem consistent. The exclusion of other changes might even boost them, as some of them were regressions. I could limit the PR to NLL-only changes; @kennytm: shall I? If so, in this PR with a rebase or in another one? |
|
Let's reuse this PR. |
|
@kennytm done. |
|
@bors try |
|
⌛ Trying commit 33d5362 with merge 15c727e088d78920db2c4590d723286550ce429b... |
|
☀️ Test successful - status-travis |
|
@rust-timer build 15c727e088d78920db2c4590d723286550ce429b |
|
Success: Queued 15c727e088d78920db2c4590d723286550ce429b with parent fefe816, comparison URL. |
|
Where have the NLL gains gone 😖? |
|
r? @kennytm |
|
None of these changes are for hot or even lukewarm code. Closing PR |
Since
sort_unstableis considered typically faster than the regularsort(benchmarks), it might be a good idea to check for potential wins in the compiler.