`VecDeque::partition_point` and `binary_search_by` check first element in `back` slice only once #136159

urben1680 · 2025-01-27T22:59:34Z

I noticed that the first element of the back slice is potentially checked twice for the partition_point method, first in an if-condition and second inside the respective body.

EDIT: Applied the same change to binary_search_by method.

In this change, the back slice is reduced by the first element and this offset is added to the result.

I am not sure how to trigger benchmarks for this, I assume doing this PR is the way to go for Bors.

Please tell me if this is not how pull requests should be done.

… once I noticed that the first element of the `back` slice is potentially checked twice for the `partition_point` method, first in an if-condition and second inside the respective body. In this change, the back slice is reduced by the first element and this offset is added to the result. I am not sure how to trigger benchmarks for this, I assume doing this PR is the way to go for bors. Please tell me if this is not how pull requests should be done.

rustbot · 2025-01-27T22:59:43Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ibraheemdev (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

…on-point-optimization, r=<try> `VecDeque::partition_point` checks first element in `back` slice only once I noticed that the first element of the back slice is potentially checked twice for the partition_point method, first in an if-condition and second inside the respective body. In this change, the back slice is reduced by the first element and this offset is added to the result. I am not sure how to trigger benchmarks for this, I assume doing this PR is the way to go for Bors. Please tell me if this is not how pull requests should be done.

bors · 2025-01-28T18:11:04Z

⌛ Trying commit 6c3de4f with merge d1b4408...

ibraheemdev · 2025-01-28T18:11:21Z

@bors try @rust-timer queue

bors · 2025-01-28T18:12:36Z

⌛ Trying commit 6c3de4f with merge 7ec1805...

…on-point-optimization, r=<try> `VecDeque::partition_point` checks first element in `back` slice only once I noticed that the first element of the back slice is potentially checked twice for the partition_point method, first in an if-condition and second inside the respective body. In this change, the back slice is reduced by the first element and this offset is added to the result. I am not sure how to trigger benchmarks for this, I assume doing this PR is the way to go for Bors. Please tell me if this is not how pull requests should be done.

bors · 2025-01-28T20:01:36Z

☀️ Try build successful - checks-actions
Build commit: 7ec1805 (7ec180558b05062d9e0503e98d43013de8be3a8f)

rust-timer · 2025-01-28T21:18:10Z

Finished benchmarking commit (7ec1805): comparison URL.

Overall result: ✅ improvements - 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 is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

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

Max RSS (memory usage)

Results (primary -0.7%, secondary 1.1%)

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)	1.1%	[1.1%, 1.1%]	1
Improvements ✅ (primary)	-0.7%	[-0.7%, -0.7%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.7%	[-0.7%, -0.7%]	1

Cycles

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

Binary size

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

Bootstrap: 772.605s -> 770.21s (-0.31%)
Artifact size: 328.25 MiB -> 328.30 MiB (0.01%)

urben1680 · 2025-01-29T19:14:43Z

This seems to look good.

I noticed the same thing can be done to binary_search_by, I will add that too.

…cked once

urben1680 · 2025-01-30T14:22:44Z

@bors try @rust-timer queue

bors · 2025-01-30T14:22:51Z

@urben1680: 🔑 Insufficient privileges: not in try users

ibraheemdev · 2025-01-31T06:39:42Z

@bors try @rust-timer queue

bors · 2025-01-31T06:40:55Z

⌛ Trying commit a12248c with merge 902ee03...

…on-point-optimization, r=<try> `VecDeque::partition_point` and `binary_search_by` check first element in `back` slice only once I noticed that the first element of the back slice is potentially checked twice for the partition_point method, first in an if-condition and second inside the respective body. EDIT: Applied the same change to `binary_search_by` method. In this change, the back slice is reduced by the first element and this offset is added to the result. I am not sure how to trigger benchmarks for this, I assume doing this PR is the way to go for Bors. Please tell me if this is not how pull requests should be done.

bors · 2025-01-31T08:28:28Z

☀️ Try build successful - checks-actions
Build commit: 902ee03 (902ee03616c2e1f1579ac7b7369a1fa37693cdef)

rust-timer · 2025-01-31T22:22:04Z

Finished benchmarking commit (902ee03): 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)

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

Cycles

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

Binary size

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

Bootstrap: 777.277s -> 779.692s (0.31%)
Artifact size: 328.97 MiB -> 328.96 MiB (-0.00%)

urben1680 · 2025-01-31T23:07:51Z

So it looks like partition_point has positive effects but binary_search_by has negative and they cancel each other out together.

rustbot assigned ibraheemdev Jan 27, 2025

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 Jan 27, 2025

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 28, 2025

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 28, 2025

VecDeque::binary_search_by: first element in back slice is only che…

a12248c

…cked once

urben1680 changed the title ~~VecDeque::partition_point checks first element in back slice only once~~ VecDeque::partition_point and binary_search_by check first element in back slice only once Jan 30, 2025

This comment has been minimized.

Sign in to view

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 31, 2025

This comment has been minimized.

Sign in to view

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 31, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`VecDeque::partition_point` and `binary_search_by` check first element in `back` slice only once #136159

`VecDeque::partition_point` and `binary_search_by` check first element in `back` slice only once #136159

urben1680 commented Jan 27, 2025 •

edited

Loading

rustbot commented Jan 27, 2025

bors commented Jan 28, 2025

ibraheemdev commented Jan 28, 2025

This comment has been minimized.

bors commented Jan 28, 2025

bors commented Jan 28, 2025

This comment has been minimized.

rust-timer commented Jan 28, 2025

urben1680 commented Jan 29, 2025 •

edited

Loading

urben1680 commented Jan 30, 2025

This comment has been minimized.

bors commented Jan 30, 2025

ibraheemdev commented Jan 31, 2025

This comment has been minimized.

bors commented Jan 31, 2025

bors commented Jan 31, 2025

This comment has been minimized.

rust-timer commented Jan 31, 2025

urben1680 commented Jan 31, 2025

VecDeque::partition_point and binary_search_by check first element in back slice only once #136159

Are you sure you want to change the base?

VecDeque::partition_point and binary_search_by check first element in back slice only once #136159

Conversation

urben1680 commented Jan 27, 2025 • edited Loading

rustbot commented Jan 27, 2025

bors commented Jan 28, 2025

ibraheemdev commented Jan 28, 2025

This comment has been minimized.

bors commented Jan 28, 2025

bors commented Jan 28, 2025

This comment has been minimized.

rust-timer commented Jan 28, 2025

Overall result: ✅ improvements - no action needed

urben1680 commented Jan 29, 2025 • edited Loading

urben1680 commented Jan 30, 2025

This comment has been minimized.

bors commented Jan 30, 2025

ibraheemdev commented Jan 31, 2025

This comment has been minimized.

bors commented Jan 31, 2025

bors commented Jan 31, 2025

This comment has been minimized.

rust-timer commented Jan 31, 2025

Overall result: no relevant changes - no action needed

urben1680 commented Jan 31, 2025

`VecDeque::partition_point` and `binary_search_by` check first element in `back` slice only once #136159

`VecDeque::partition_point` and `binary_search_by` check first element in `back` slice only once #136159

urben1680 commented Jan 27, 2025 •

edited

Loading

urben1680 commented Jan 29, 2025 •

edited

Loading