Improve BinaryHeap performance

[SkiFire13]

By changing the condition in the loops from child < end to child < end - 1 we're guaranteed that right = child + 1 < end and since finding the index of the biggest sibling can be done with an arithmetic operation we can remove a branch from the loop body. The case where there's no right child, i.e. child == end - 1 is instead handled outside the loop, after it ends; note that if the loops ends early we can use return instead of break since the check child == end - 1 will surely fail.

I've also removed a call to <[T]>::swap that was hiding a bound check that wasn't being optimized by LLVM.

A quick benchmarks on my pc shows that the gains are pretty significant:

name	before ns/iter	after ns/iter	diff ns/iter	diff %	speedup
find_smallest_1000	352,565	260,098	-92,467	-26.23%	x 1.36
from_vec	676,795	473,934	-202,861	-29.97%	x 1.43
into_sorted_vec	469,511	304,275	-165,236	-35.19%	x 1.54
pop	483,198	373,778	-109,420	-22.64%	x 1.29

The other 2 benchmarks for BinaryHeap (peek_mut_deref_mut and push) weren't impacted and as such didn't show any significant change.

[rust-highfive]

r? @KodrAus

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

[tavianator]

I don't know if this makes a difference performance-wise (or even readability-wise), but you could avoid the comparisons with end - 1 by computing the right child by default instead of the left (child = 2 * pos + 2).

[SkiFire13]

@tavianator I didn't think of that, but I wouldn't expect it to make much of a difference (the compiler stores end-1 in a register before the loop so no other calculation is required inside). I benched it anyway, and surprising it even performs worse.

name	left child ns/iter	right child ns/iter	diff ns/iter	diff %	speedup
find_smallest_1000	260,098	268,056	+7,958	+3.06%	x 0.97
from_vec	473,934	533,500	+59,566	+12.57%	x 0.89
into_sorted_vec	304,275	365,226	+60,951	+20.03%	x 0.83
pop	373,778	392,372	+18,594	+4.97%	x 0.95

I also tested a reduced version on godbolt, and looks like LLVM is doing a worse job at optimizing sift_down_range's loop (13 vs 16 instructions for a u64, that's like 23% more instructions!) which might explain the slowdown, in particular hole.get(child - 1) is not optimized as good as hole.get(child + 1) previously was.

[tavianator]

Huh, thanks for checking. That codegen is kinda odd.

[KodrAus]

Nice! I just left one comment to include your rationale for the new unsafe block inline. r=me after that.

[SkiFire13]

I've added the safety comment, I hope it's clear enough. I'm tempted to document other unsafe usages as well, but that should probably have its own PR.

r=KodrAus

[Xanewok]

@bors r=KodrAus

(You need to mention the bot and be either in the reviewer set or be delegated to, see cheatsheet at https://bors.rust-lang.org/)

[bors]

📌 Commit 387568c has been approved by KodrAus

[bors]

⌛ Testing commit 387568c with merge d75378aab28aecf8b1521a40409823dd9c0f928b...

[bors]

💔 Test failed - checks-actions

[kennytm]

@bors retry