Rewrite binary search implementation

[Amanieu]

This PR builds on top of #128250, which should be merged first.

This restores the original binary search implementation from #45333 which has the nice property of having a loop count that only depends on the size of the slice. This, along with explicit conditional moves from #128250, means that the entire binary search loop can be perfectly predicted by the branch predictor.

Additionally, LLVM is able to unroll the loop when the slice length is known at compile-time. This results in a very compact code sequence of 3-4 instructions per binary search step and zero branches.

Fixes #53823
Fixes #115271

[rustbot]

r? @tgross35

rustbot has assigned @tgross35.
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

[Amanieu]

Benchmarks

AMD Ryzen 9 3950X

Old

test binary_search_l1              ... bench:          28.09 ns/iter (+/- 0.74)
test binary_search_l1_with_dups    ... bench:          18.12 ns/iter (+/- 0.35)
test binary_search_l1_worst_case   ... bench:          14.19 ns/iter (+/- 1.14)
test binary_search_l2              ... bench:          40.88 ns/iter (+/- 1.11)
test binary_search_l2_with_dups    ... bench:          27.79 ns/iter (+/- 0.70)
test binary_search_l2_worst_case   ... bench:          25.57 ns/iter (+/- 0.83)
test binary_search_l3              ... bench:         109.16 ns/iter (+/- 1.25)
test binary_search_l3_with_dups    ... bench:          70.63 ns/iter (+/- 4.93)
test binary_search_l3_worst_case   ... bench:          44.09 ns/iter (+/- 0.74)

New

test binary_search_l1              ... bench:          11.87 ns/iter (+/- 0.98)
test binary_search_l1_with_dups    ... bench:          11.73 ns/iter (+/- 0.47)
test binary_search_l1_worst_case   ... bench:           9.80 ns/iter (+/- 0.22)
test binary_search_l2              ... bench:          18.23 ns/iter (+/- 3.47)
test binary_search_l2_with_dups    ... bench:          17.57 ns/iter (+/- 1.12)
test binary_search_l2_worst_case   ... bench:          14.70 ns/iter (+/- 1.17)
test binary_search_l3              ... bench:          61.18 ns/iter (+/- 1.63)
test binary_search_l3_with_dups    ... bench:          61.67 ns/iter (+/- 2.21)
test binary_search_l3_worst_case   ... bench:          24.73 ns/iter (+/- 0.64)

ARM Cortex-A72

Old

test binary_search_l1              ... bench:          69.01 ns/iter (+/- 0.19)
test binary_search_l1_with_dups    ... bench:          42.80 ns/iter (+/- 0.13)
test binary_search_l1_worst_case   ... bench:          41.70 ns/iter (+/- 1.32)
test binary_search_l2              ... bench:         108.10 ns/iter (+/- 0.66)
test binary_search_l2_with_dups    ... bench:          68.70 ns/iter (+/- 0.22)
test binary_search_l2_worst_case   ... bench:          67.60 ns/iter (+/- 0.14)
test binary_search_l3              ... bench:         807.14 ns/iter (+/- 19.50)
test binary_search_l3_with_dups    ... bench:         491.36 ns/iter (+/- 7.95)
test binary_search_l3_worst_case   ... bench:         137.74 ns/iter (+/- 0.37)

New

test binary_search_l1              ... bench:          44.36 ns/iter (+/- 0.10)
test binary_search_l1_with_dups    ... bench:          44.41 ns/iter (+/- 0.07)
test binary_search_l1_worst_case   ... bench:          32.33 ns/iter (+/- 0.02)
test binary_search_l2              ... bench:          75.38 ns/iter (+/- 0.22)
test binary_search_l2_with_dups    ... bench:          74.90 ns/iter (+/- 0.33)
test binary_search_l2_worst_case   ... bench:          50.03 ns/iter (+/- 0.14)
test binary_search_l3              ... bench:         742.06 ns/iter (+/- 26.84)
test binary_search_l3_with_dups    ... bench:         742.38 ns/iter (+/- 9.52)
test binary_search_l3_worst_case   ... bench:          76.37 ns/iter (+/- 0.10)

[tgross35]

The result change for equal values seems fine to me, considering docs explicitly allow it. We could also consider special-casing on T::IS_ZST either to ensure those results do not change, or to provide an algorithm-agnostic deterministic output like #127007.

I think this is easily justified by the performance wins. However, since it does change user-visible stable behavior, it seems reasonable to get group consensus or decide if crater results are wanted first (as suggested at

rust/library/core/tests/slice.rs

Lines 83 to 97 in 21ee157

	#[test]
	// Test implementation specific behavior when finding equivalent elements.
	// It is ok to break this test but when you do a crater run is highly advisable.
	fn test_binary_search_implementation_details() {
	let b = [1, 1, 2, 2, 3, 3, 3];
	assert_eq!(b.binary_search(&1), Ok(1));
	assert_eq!(b.binary_search(&2), Ok(3));
	assert_eq!(b.binary_search(&3), Ok(6));
	let b = [1, 1, 1, 1, 1, 3, 3, 3, 3];
	assert_eq!(b.binary_search(&1), Ok(4));
	assert_eq!(b.binary_search(&3), Ok(8));
	let b = [1, 1, 1, 1, 3, 3, 3, 3, 3];
	assert_eq!(b.binary_search(&1), Ok(3));
	assert_eq!(b.binary_search(&3), Ok(8));
	}

).

@rustbot label +I-libs-nominated

r=me for the actual implementation change after the above is addressed in some way, and a rebase once #128250 merges.

[Amanieu]

I don't think it is worth special casing ZSTs since realistically nobody is ever going to sort a ZST slice. As long as we're still generating correct results for ZSTs, I would just ignore them.

[bors]

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

[joboet]

I'd like to mention @scandum's collection of binary search algorithms, the current version in this PR is identical to the "monobound binary search". But maybe it'd be worth switching to the "tripple-tapped" variant instead?

[Amanieu]

The problem with the triple-tapped variant is that the number of iterations in the second loop is data-dependent. One of the main reasons the current algorithm is so fast is that it achieves perfect branch prediction no matter which key is being searched for.

[Amanieu]

@bors try

[tgross35]

@bors r+

[bors]

📌 Commit bb58488 has been approved by tgross35

It is now in the queue for this repository.

[scottmcm]

Big fan of going back to an implementation that can fully-unroll. That's really helpful for doing lookups in static tables 👍

[bors]

⌛ Testing commit bb58488 with merge 1932602...

[bors]

☀️ Test successful - checks-actions
Approved by: tgross35
Pushing 1932602 to master...

[rust-timer]

Finished benchmarking commit (1932602): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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.5%	[0.2%, 1.7%]	16
Regressions ❌ (secondary)	6.2%	[0.2%, 19.5%]	6
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-4.1%	[-4.1%, -4.1%]	1
All ❌✅ (primary)	0.5%	[0.2%, 1.7%]	16

Max RSS (memory usage)

Results (primary 3.6%, secondary 2.4%)

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

Cycles

Results (primary 1.7%, secondary 0.9%)

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)	1.7%	[1.3%, 2.1%]	3
Regressions ❌ (secondary)	11.1%	[10.3%, 12.0%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-3.2%	[-4.9%, -1.1%]	5
All ❌✅ (primary)	1.7%	[1.3%, 2.1%]	3

Binary size

Results (primary 0.1%, secondary 0.5%)

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.3%]	29
Regressions ❌ (secondary)	0.5%	[0.1%, 2.9%]	42
Improvements ✅ (primary)	-0.8%	[-0.8%, -0.8%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[-0.8%, 0.3%]	30

Bootstrap: 758.912s -> 755.218s (-0.49%)
Artifact size: 336.84 MiB -> 336.77 MiB (-0.02%)

[Kobzol]

This is an optimization of the binary search algorithm, which should make runtime faster. The regression seems to be caused by LLVM spending more time in optimizing the binary search (since it is now more amenable to optimization and unrolling).

@rustbot label: +perf-regression-triaged

[craterbot]

🚧 Experiment pr-128254 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-128254 is completed!
📊 318 regressed and 134 fixed (487456 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[Amanieu]

@rust-lang/release

Refer to these crater results when evaluating the beta crater run: these are crates whose tests depends on implementation details of binary_search when there are multiple matching elements. This PR changed the exact index that is returned in such cases, which is allowed by the documentation of binary_search.