Skip to content

Prefer sort_unstable*() over sort*() - #52672

Closed
ljedrz wants to merge 1 commit into
rust-lang:masterfrom
ljedrz:unstable_sorts
Closed

Prefer sort_unstable*() over sort*()#52672
ljedrz wants to merge 1 commit into
rust-lang:masterfrom
ljedrz:unstable_sorts

Conversation

@ljedrz

@ljedrzljedrz commented Jul 24, 2018

Copy link
Copy Markdown
Contributor

Since sort_unstable is considered typically faster than the regular sort (benchmarks), it might be a good idea to check for potential wins in the compiler.

@rust-highfive

rust-highfive commented Jul 24, 2018

Copy link
Copy Markdown
Contributor

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 24, 2018
kennytm
kennytm reviewed Jul 24, 2018
View reviewed changes
Comment threadsrc/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 } );

@kennytmkennytmJul 24, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This one could be data.sort_unstable_by_key(|k| Reverse(&k.3)), which also handles the case self1 == self2 correctly

@ljedrzljedrzJul 24, 2018•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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.

@leonardo-m

leonardo-m commented Jul 24, 2018

Copy link
Copy Markdown

Are you sure that an unstable sorting keeps the code semantics correct in every one of those cases?

@ljedrz

ljedrz commented Jul 24, 2018

Copy link
Copy Markdown
ContributorAuthor

@leonardo-m I ran test --stage 1, had to remove a few cases. I'm not sure, but it may be worth at least to benchmark a try build to know if it's worth investigating more thoroughly.

@kennytm

kennytm commented Jul 24, 2018

Copy link
Copy Markdown
Member

I'd like to ensure the CI to give a ✅ before perf.

@petrochenkov

petrochenkov commented Jul 24, 2018

Copy link
Copy Markdown
Contributor

Travis is green.
@bors try

@bors

bors commented Jul 24, 2018

Copy link
Copy Markdown
Collaborator

⌛ Trying commit b520c42 with merge 90ec471...

bors added a commit that referenced this pull request Jul 24, 2018
@bors
Auto merge of#52672- ljedrz:unstable_sorts, r=<try>
90ec471
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.
@bors

bors commented Jul 24, 2018

Copy link
Copy Markdown
Collaborator

☀️ Test successful - status-travis
State: approved= try=True

@kennytm

kennytm commented Jul 24, 2018

Copy link
Copy Markdown
Member

@rust-timer build 90ec471

@rust-timer

rust-timer commented Jul 24, 2018

Copy link
Copy Markdown
Collaborator

Success: Queued 90ec471 with parent f498e4e, comparison URL.

scottmcm
scottmcm reviewed Jul 25, 2018
View reviewed changes
Comment threadsrc/librustc/ty/layout.rs
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.

@scottmcmscottmcmJul 25, 2018•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

scottmcm
scottmcm reviewed Jul 25, 2018
View reviewed changes
Comment threadsrc/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>)| {

@scottmcmscottmcmJul 25, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: looks like this could be _by_key too?

@kennytm

kennytm commented Jul 25, 2018•
edited
Loading

Copy link
Copy Markdown
Member

Perf is ready. Heh, some checks have even become slower. No big impact in total.

@ljedrz

ljedrz commented Jul 25, 2018

Copy link
Copy Markdown
ContributorAuthor

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.

@ljedrzljedrz closed this Jul 25, 2018
@ghost

ghost commented Jul 25, 2018

Copy link
Copy Markdown

Looks like some nll-checks got faster. Perhaps it'd be worth switching to sort_unstable in the three NLL-related files only?

@ljedrz

ljedrz commented Jul 25, 2018

Copy link
Copy Markdown
ContributorAuthor

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?

@kennytm

kennytm commented Jul 25, 2018

Copy link
Copy Markdown
Member

Let's reuse this PR.

@kennytmkennytm reopened this Jul 25, 2018
@ljedrz

ljedrz commented Jul 25, 2018

Copy link
Copy Markdown
ContributorAuthor

@kennytm done.

@kennytm

kennytm commented Jul 25, 2018

Copy link
Copy Markdown
Member

@bors try

@bors

bors commented Jul 25, 2018

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 33d5362 with merge 15c727e088d78920db2c4590d723286550ce429b...

@bors

bors commented Jul 25, 2018

Copy link
Copy Markdown
Collaborator

☀️ Test successful - status-travis
State: approved= try=True

@kennytm

kennytm commented Jul 25, 2018

Copy link
Copy Markdown
Member

@rust-timer build 15c727e088d78920db2c4590d723286550ce429b

@rust-timer

rust-timer commented Jul 25, 2018

Copy link
Copy Markdown
Collaborator

Success: Queued 15c727e088d78920db2c4590d723286550ce429b with parent fefe816, comparison URL.

@ljedrz

ljedrz commented Jul 26, 2018

Copy link
Copy Markdown
ContributorAuthor

Where have the NLL gains gone 😖?

@petrochenkov

petrochenkov commented Jul 26, 2018

Copy link
Copy Markdown
Contributor

r? @kennytm

@pnkfelix

pnkfelix commented Jul 27, 2018

Copy link
Copy Markdown
Contributor

None of these changes are for hot or even lukewarm code. Closing PR

@ljedrz
ljedrz deleted the unstable_sorts branch July 28, 2018 06:50
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kennytmkennytmkennytm left review comments
@scottmcmscottmcmscottmcm left review comments

Assignees

@kennytmkennytm

Labels

S-waiting-on-reviewStatus: Awaiting review from the assignee but also interested parties.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@ljedrz@rust-highfive@leonardo-m@kennytm@petrochenkov@bors@rust-timer@pnkfelix@scottmcm