BinaryHeap: Simplify sift down

[bluss]

BinaryHeap: Simplify sift down

Sift down was doing all too much work: it can stop directly when the
current element obeys the heap property in relation to its children.

In the old code, sift down didn't compare the element to sift down at
all, so it was maximally sifted down and relied on the sift up call to
put it in the correct location.

This should speed up heapify and .pop().

Also rename Hole::removed() to Hole::element()

[rust-highfive]

r? @alexcrichton

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

[bluss]

I don't quite understand why sift down was so complicated to start with. It's certainly not needed for heapify, push or pop, but something else?

[Gankra]

of the two

[Gankra]

I'm guessing this was an artifact of refactoring that wasn't caught in prior review. I'm nervous about this just because I don't understand what the old code was trying to do.

[Gankra]

iirc the style is to add the semi after the break?

[bluss]

will do

[Gankra]

I double checked the defn of heap sort, and this seems to check out.

r=me with nits

[bluss]

Applied the fixes, and improved the commit log text to point out why the old code worked. Thank you for the review!

[bluss]

@bors r=gankro

[bors]

📌 Commit 81fcdd4 has been approved by gankro

[bors]

⌛ Testing commit 81fcdd4 with merge ec8ae4b...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

[bluss]

Here's a microbenchmark

Comparing rust versions:

• before: rustc 1.6.0-nightly (d5fde83 2015-11-12)
• after: rustc 1.6.0-nightly (bdfb135 2015-11-14)

It suggest a slight improvment with this PR.

It's a minimum spanning tree computation on a graph of 450 edges. The algorithm spends most of its time popping off the least edge from a BinaryHeap, and its runtime improves slightly:

before
test bench_mst                 ... bench:      53,023 ns/iter (+/- 391)

after
test bench_mst                 ... bench:      47,357 ns/iter (+/- 243)

[dgrunwald]

From the Python source code, which also maximally sifts down and then back up:

# We *could* break out of the loop as soon as we find a pos where newitem <=
# both its children, but turns out that's not a good idea, and despite that
# many books write the algorithm that way.  During a heap pop, the last array
# element is sifted in, and that tends to be large, so that comparing it
# against values starting from the root usually doesn't pay (= usually doesn't
# get us out of the loop early).  See Knuth, Volume 3, where this is
# explained and quantified in an exercise.

I think this change could use some more benchmarks; I suspect whether you win or lose performance depends on how expensive the comparison function is compared to moving elements around in memory.

[bluss]

@dgrunwald In Python, list element swaps are very cheap (one PyObject *), and comparisons expensive (python function call).

[bluss]

Indeed, in the same kind of benchmark where the edge weight is a type with more expensive comparison function (&str), it instead uses 5% longer to complete the benchmark.

Maybe we can use separate sift_down methods for pop and for heapify (BinaryHeap::from).