Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid unnecessary comparison in partition_equal #117179

Merged

Conversation

Voultapher
Copy link
Contributor

The branchy Hoare partition partition_equal as part of slice::sort_unstable has a bug that makes it perform a comparison of the last element twice.

Measuring inputs with a Zipfian distribution with characterizing exponent s == 1.0, yields a ~0.05% reduction in the total number of comparisons performed.

The branchy Hoare partition `partition_equal` as part of `slice::sort_unstable`
has a bug that makes it perform a comparison of the last element twice.

Measuring inputs with a Zipfian distribution with characterizing exponent s ==
1.0, yields a ~0.05% reduction in the total number of comparisons performed.
@rustbot
Copy link
Collaborator

rustbot commented Oct 25, 2023

r? @Mark-Simulacrum

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 25, 2023
@Voultapher
Copy link
Contributor Author

This is not meant as a big perf improvement, more as a bug fix.

@lqd
Copy link
Member

lqd commented Oct 25, 2023

That may not be hot in the compiler, but maybe it would show up in the runtime benchmarks: @bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 25, 2023
@bors
Copy link
Contributor

bors commented Oct 25, 2023

⌛ Trying commit e501add with merge c2c287e...

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 25, 2023
…tion-equal, r=<try>

Avoid unnecessary comparison in partition_equal

The branchy Hoare partition `partition_equal` as part of `slice::sort_unstable` has a bug that makes it perform a comparison of the last element twice.

Measuring inputs with a Zipfian distribution with characterizing exponent s == 1.0, yields a ~0.05% reduction in the total number of comparisons performed.
@Voultapher
Copy link
Contributor Author

That may not be hot in the compiler, but maybe it would show up in the runtime benchmarks: @bors try @rust-timer queue

I very much doubt it, afaic the rustc runtime benchmarks don't have sort specific stuff, and even then it's such a minuscule change it shouldn't have a noticeable impact. If my goal was perf here, I'd replace the whole partition_equal with a call to partition with flipped comparator. According to the pdqsort author having a branchy equal partition was a bespoke decision that doesn't hold up anymore. I have some larger sort perf stuff in the works, this is just a small bugfix I cam across while working on that.

@rust-timer

This comment was marked as outdated.

@bors
Copy link
Contributor

bors commented Oct 25, 2023

☀️ Try build successful - checks-actions
Build commit: c2c287e (c2c287eb8ca6fd5d9f4af7614d6e1c1562350b96)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c2c287e): comparison URL.

Overall result: no relevant changes - no action needed

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
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.9% [2.9%, 2.9%] 1
Improvements ✅
(primary)
-3.7% [-3.7%, -3.7%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -3.7% [-3.7%, -3.7%] 1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.9% [2.9%, 2.9%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.0% [0.0%, 0.0%] 4
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.2% [-0.2%, -0.1%] 2
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.0% [-0.2%, 0.0%] 6

Bootstrap: missing data
Artifact size: 304.42 MiB -> 304.41 MiB (-0.00%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 25, 2023
@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 4, 2023

📌 Commit e501add has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 4, 2023
@bors
Copy link
Contributor

bors commented Nov 4, 2023

⌛ Testing commit e501add with merge 8ba8254...

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 4, 2023
…tion-equal, r=Mark-Simulacrum

Avoid unnecessary comparison in partition_equal

The branchy Hoare partition `partition_equal` as part of `slice::sort_unstable` has a bug that makes it perform a comparison of the last element twice.

Measuring inputs with a Zipfian distribution with characterizing exponent s == 1.0, yields a ~0.05% reduction in the total number of comparisons performed.
@rust-log-analyzer
Copy link
Collaborator

The job aarch64-gnu failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
   Compiling rayon v1.7.0
[RUSTC-TIMING] rayon_core test:false 1.667
[RUSTC-TIMING] tar test:false 2.087
[RUSTC-TIMING] rayon test:false 3.587
##[error]The runner has received a shutdown signal. This can happen when the runner service is stopped, or a manually started runner is canceled.
##[group]Clock drift check
  local time: Sun Nov  5 00:01:53 UTC 2023
  local time: Sun Nov  5 00:01:53 UTC 2023
Session terminated, killing shell... ...killed.
##[error]The operation was canceled.
Cleaning up orphan processes

@bors
Copy link
Contributor

bors commented Nov 5, 2023

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 5, 2023
@Voultapher
Copy link
Contributor Author

It's not clear to me if this is a true positive CI failure ##[error]The runner has received a shutdown signal. This can happen when the runner service is stopped, or a manually started runner is canceled..

@Mark-Simulacrum
Copy link
Member

@bors retry

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 5, 2023
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Nov 5, 2023
@bors
Copy link
Contributor

bors commented Nov 5, 2023

⌛ Testing commit e501add with merge 5103173...

@bors
Copy link
Contributor

bors commented Nov 5, 2023

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 5103173 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 5, 2023
@bors bors merged commit 5103173 into rust-lang:master Nov 5, 2023
12 checks passed
@rustbot rustbot added this to the 1.75.0 milestone Nov 5, 2023
bors-ferrocene bot added a commit to ferrocene/ferrocene that referenced this pull request Nov 6, 2023
84: Automated pull from upstream `master` r=Dajamante a=github-actions[bot]


This PR pulls the following changes from the upstream repository:

* rust-lang/rust#117585
* rust-lang/rust#117576
* rust-lang/rust#96979
* rust-lang/rust#117191
* rust-lang/rust#117179
* rust-lang/rust#117574
* rust-lang/rust#117537
* rust-lang/rust#117608
  * rust-lang/rust#117596
  * rust-lang/rust#117588
  * rust-lang/rust#117524
  * rust-lang/rust#116017
* rust-lang/rust#117504
* rust-lang/rust#117469
* rust-lang/rust#116218
* rust-lang/rust#117589
* rust-lang/rust#117581
* rust-lang/rust#117503
* rust-lang/rust#117590
  * rust-lang/rust#117583
  * rust-lang/rust#117570
  * rust-lang/rust#117562
  * rust-lang/rust#117534
  * rust-lang/rust#116894
  * rust-lang/rust#110340
* rust-lang/rust#113343
* rust-lang/rust#117579
* rust-lang/rust#117094
* rust-lang/rust#117566
* rust-lang/rust#117564
  * rust-lang/rust#117554
  * rust-lang/rust#117550
  * rust-lang/rust#117343
* rust-lang/rust#115274
* rust-lang/rust#117540
* rust-lang/rust#116412
* rust-lang/rust#115333
* rust-lang/rust#117507
* rust-lang/rust#117538
  * rust-lang/rust#117533
  * rust-lang/rust#117523
  * rust-lang/rust#117520
  * rust-lang/rust#117505
  * rust-lang/rust#117434
* rust-lang/rust#117535
* rust-lang/rust#117510
* rust-lang/rust#116439
* rust-lang/rust#117508



Co-authored-by: Ben Wiederhake <[email protected]>
Co-authored-by: SabrinaJewson <[email protected]>
Co-authored-by: J-ZhengLi <[email protected]>
Co-authored-by: koka <[email protected]>
Co-authored-by: bjorn3 <[email protected]>
Co-authored-by: Joshua Liebow-Feeser <[email protected]>
Co-authored-by: lengyijun <[email protected]>
Co-authored-by: Zalathar <[email protected]>
Co-authored-by: Oli Scherer <[email protected]>
Co-authored-by: Philipp Krones <[email protected]>
Co-authored-by: y21 <[email protected]>
Co-authored-by: bors <[email protected]>
Co-authored-by: bohan <[email protected]>
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (5103173): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.7% [0.5%, 1.0%] 4
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.5% [-4.0%, -0.5%] 5
Improvements ✅
(secondary)
-1.1% [-1.1%, -1.1%] 2
All ❌✅ (primary) -0.5% [-4.0%, 1.0%] 9

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.4% [0.4%, 0.4%] 1
Regressions ❌
(secondary)
0.4% [0.4%, 0.4%] 1
Improvements ✅
(primary)
-0.4% [-0.4%, -0.4%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.0% [-0.4%, 0.4%] 2

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.0% [0.0%, 0.1%] 6
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.0% [0.0%, 0.1%] 6

Bootstrap: 638.861s -> 635.766s (-0.48%)
Artifact size: 304.51 MiB -> 304.41 MiB (-0.03%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants