Improve slice::binary_search_by

[krtab]

This PR aims to improve the performances of std::slice::binary_search.

EDIT: The proposed implementation changed so the rest of this comment is outdated. See #127007 (comment) for an up to date presentation of the PR.

It reduces the total instruction count for the u32 monomorphization, but maybe more remarkably, removes 2 of the 12 instructions of the main loop (on x86).

It changes test_binary_search_implementation_details() so may warrant a crater run.

I will document it much more if this is shown to be interesting on benchmarks. Could we start with a timer run first?

Before the PR

 mov eax, 1  test rsi, rsi  je .LBB0_1  mov rcx, rdx  mov rdx, rsi  mov ecx, dword ptr [rcx]  xor esi, esi  mov r8, rdx .LBB0_3:  shr rdx  add rdx, rsi  mov r9d, dword ptr [rdi + 4*rdx]  cmp r9d, ecx  je .LBB0_4  lea r10, [rdx + 1]  cmp r9d, ecx  cmova r8, rdx  cmovb rsi, r10  mov rdx, r8  sub rdx, rsi  ja .LBB0_3  mov rdx, rsi  ret .LBB0_1:  xor edx, edx  ret .LBB0_4:  xor eax, eax  ret

After the PR

 mov ecx, dword ptr [rdx]  xor eax, eax  xor edx, edx .LBB1_1:  cmp rsi, 1  jbe .LBB1_2  mov r9, rsi  shr r9  lea r8, [r9 + rdx]  sub rsi, r9  cmp dword ptr [rdi + 4*r8], ecx  cmovb rdx, r8  cmova rsi, r9  jne .LBB1_1  mov rdx, r8  ret .LBB1_2:  test rsi, rsi  je .LBB1_3  xor eax, eax  cmp dword ptr [rdi + 4*rdx], ecx  setne al  adc rdx, 0  ret .LBB1_3:  mov eax, 1  ret

[rustbot]

r? @m-ou-se

rustbot has assigned @m-ou-se.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[krtab]

I've run benches with ./x bench core --test-args binary_search and so far this PR is unfortunately not improving. This is likely because the added number of iterations is not compensated by the reduced number of iterations.

[krtab]

A different strategy bore fruits! It relies on the fact that except for ZST, slice.len is less than isize::MAX and so (right + left)/2 is an ok way to compute the mean. By treating the ZST case separately, we can "revert" 3eb5bee and win some instructions.

ZST comparing to Equal special case

Assembly:

Before

 mov rdx, rsi  xor eax, eax  test rsi, rsi  sete al  shr rdx  ret

After

 xor eax, eax  test rsi, rsi  sete al  xor edx, edx  ret

usize on x86_64

Assembly

Before

example::custom_binary_search_gt::hb56715903c41dc94:  mov eax, 1  test rsi, rsi  je .LBB0_1  mov rcx, rdx  mov rdx, rsi  mov rcx, qword ptr [rcx]  xor esi, esi  mov r8, rdx .LBB0_3:  shr rdx  add rdx, rsi  mov r9, qword ptr [rdi + 8*rdx]  cmp r9, rcx  je .LBB0_4  lea r10, [rdx + 1]  cmp r9, rcx  cmova r8, rdx  cmovb rsi, r10  mov rdx, r8  sub rdx, rsi  ja .LBB0_3  mov rdx, rsi  ret .LBB0_1:  xor edx, edx  ret .LBB0_4:  xor eax, eax  ret

After

example::custom_binary_search_1::hc0743890bf3b6f85:  mov rcx, qword ptr [rdx]  xor eax, eax  xor edx, edx .LBB1_1:  cmp rsi, rdx  jbe .LBB1_2  lea r8, [rsi + rdx]  shr r8  lea r9, [r8 + 1]  cmp qword ptr [rdi + 8*r8], rcx  cmovb rdx, r9  cmova rsi, r8  jne .LBB1_1  mov rdx, r8  ret .LBB1_2:  mov eax, 1  ret

Benches

Before

$ ./x bench core --test-args binary_search benchmarks: slice::binary_search_l1 20.69/iter +/- 0.45 slice::binary_search_l1_with_dups 12.17/iter +/- 0.06 slice::binary_search_l1_worst_case 9.21/iter +/- 0.03 slice::binary_search_l2 31.81/iter +/- 0.25 slice::binary_search_l2_with_dups 21.05/iter +/- 0.29 slice::binary_search_l2_worst_case 15.46/iter +/- 0.32 slice::binary_search_l3 75.05/iter +/- 0.95 slice::binary_search_l3_with_dups 53.66/iter +/- 0.89 slice::binary_search_l3_worst_case 27.74/iter +/- 0.03 

After

$ ./x bench core --test-args binary_search benchmarks: slice::binary_search_l1 20.19/iter +/- 3.60 slice::binary_search_l1_with_dups 14.24/iter +/- 0.57 slice::binary_search_l1_worst_case 8.77/iter +/- 3.00 slice::binary_search_l2 29.81/iter +/- 0.64 slice::binary_search_l2_with_dups 19.45/iter +/- 1.32 slice::binary_search_l2_worst_case 14.08/iter +/- 0.65 slice::binary_search_l3 70.80/iter +/- 6.43 slice::binary_search_l3_with_dups 52.35/iter +/- 2.77 slice::binary_search_l3_worst_case 23.62/iter +/- 2.73 

[lqd]

Could we start with a timer run first?

@bors try @rust-timer queue

[bors]

⌛ Trying commit 2f5eec9 with merge 3dc1b1e...

[bors]

☀️ Try build successful - checks-actions
Build commit: 3dc1b1e (3dc1b1e098adb3f6866155993a91f1cad3513a9c)

[rust-timer]

Finished benchmarking commit (3dc1b1e): 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 a highly reliable metric that was used to determine the overall result at the top of this comment.

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

Max RSS (memory usage)

Results (primary 0.0%)

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)	2.5%	[2.5%, 2.5%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.5%	[-2.5%, -2.5%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[-2.5%, 2.5%]	2

Cycles

Results (primary -2.6%, secondary 2.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)	2.1%	[2.1%, 2.1%]	1
Improvements ✅ (primary)	-2.6%	[-2.6%, -2.6%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-2.6%	[-2.6%, -2.6%]	1

Binary size

Results (primary -0.0%, secondary -0.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.1%	[0.0%, 0.2%]	4
Regressions ❌ (secondary)	0.5%	[0.5%, 0.5%]	1
Improvements ✅ (primary)	-0.1%	[-0.2%, -0.1%]	6
Improvements ✅ (secondary)	-0.1%	[-0.2%, -0.0%]	41
All ❌✅ (primary)	-0.0%	[-0.2%, 0.2%]	10

Bootstrap: 694.857s -> 695.931s (0.15%)
Artifact size: 324.45 MiB -> 324.42 MiB (-0.01%)

[Mili-ssm]

Hi, i discover this PR this morning, yesterday a post on zulip about some ideas on how to improve the STD binary search. If it is worth or usefull here is the post on zulip Proposal for Binary Search Algorithm Enhancing.

I am not sure if is desirable, better or worst but if the algorith is under reviw maybe is helpful.

[krtab]

All benchmarks are good and I added comments, so this is ready for review.

[okaneco]

Within the safety comment, there could be an explicit mention of where the mid relations come from.
As a reader, it's easier to evaluate the logic in the comment without needing to consult the line of code above at the same time.

Suggested change

	// SAFETY: We have that left < right, so
	// 0 <= left <= mid < right <= self.len()
	// and the indexing is in-bounds.
	// SAFETY: The while condition ensures left < right,
	// so `left <= (left + right) / 2 < right`. Thus,
	// 0 <= left <= mid < right <= self.len()
	// and the indexing is in-bounds.

[Miksel12]

This binary search implementation by @orlp might also be interesting to look at: https://orlp.net/blog/bitwise-binary-search/

[orlp]

@Miksel12 That article is mostly for educational purposes. I believe this to be the fastest pattern possible: https://mhdm.dev/posts/sb_lower_bound/#update (if you can get the compiler to generate cmovs).

[krtab]

@Miksel12 That article is mostly for educational purposes. I believe this to be the fastest pattern possible: https://mhdm.dev/posts/sb_lower_bound/#update (if you can get the compiler to generate cmovs).

The massive difference is that this version does not have an early return for the equality case. Removing this early return from our implementation would also bring the instruction count down but I don't think this is what we want.

[orlp]

@krtab I don't think the early return is valuable. It only comes into play if you're exceptionally lucky, and then it only saves O(log n) iterations in the best case. It adds an extra branch to every iteration every time you're not exceptionally lucky.

To be more precise, it is expected that the number of comparisons you save for random data is 1/2 * (0 + 1/2 * (1 + 1/2 * 2 + ... = 1/2 + 1/4 + 1/8 + 1/16 + ... = 1. That's right, the early return on average for random data only saves a single comparison.

For what it's worth, in my experience you almost always want partition_point and not binary_search.

[Amanieu]

In #128254 I actually revert to the old algorithm which turns out to be much faster once you ensure that LLVM doesn't turn CMOV into branches.

[bors]

☔ The latest upstream changes (presumably #128254) made this pull request unmergeable. Please resolve the merge conflicts.

[Amanieu]

Superseded by #128254. However I still want to thank @krtab for your effort in looking into this.