Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BinaryHeap: Simplify sift down #29811

Merged
merged 1 commit into from
Nov 13, 2015
Merged

Conversation

bluss
Copy link
Member

@bluss bluss commented Nov 13, 2015

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
Copy link
Collaborator

r? @alexcrichton

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

@brson brson added the relnotes Marks issues that should be documented in the release notes of the next release. label Nov 13, 2015
@bluss
Copy link
Member Author

bluss commented Nov 13, 2015

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?

@bluss bluss force-pushed the binary-heap-sift-less branch 3 times, most recently from 4393a5c to 75c0972 Compare November 13, 2015 02:12
unsafe {
let mut hole = Hole::new(&mut self.data, pos);
let mut child = 2 * pos + 1;
while child < end {
let right = child + 1;
// compare with the greater the two children
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

of the two

@Gankra
Copy link
Contributor

Gankra commented Nov 13, 2015

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.

@@ -521,29 +521,30 @@ impl<T: Ord> BinaryHeap<T> {

while hole.pos() > start {
let parent = (hole.pos() - 1) / 2;
if hole.removed() <= hole.get(parent) { break }
if hole.element() <= hole.get(parent) { break }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do

@Gankra
Copy link
Contributor

Gankra commented Nov 13, 2015

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

r=me with nits

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()
@bluss
Copy link
Member Author

bluss commented Nov 13, 2015

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

@bluss
Copy link
Member Author

bluss commented Nov 13, 2015

@bors r=gankro

@bors
Copy link
Contributor

bors commented Nov 13, 2015

📌 Commit 81fcdd4 has been approved by gankro

@bors
Copy link
Contributor

bors commented Nov 13, 2015

⌛ Testing commit 81fcdd4 with merge ec8ae4b...

bors added a commit that referenced this pull request Nov 13, 2015
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()
@bors bors merged commit 81fcdd4 into rust-lang:master Nov 13, 2015
@bluss bluss deleted the binary-heap-sift-less branch November 13, 2015 17:54
@bluss
Copy link
Member Author

bluss commented Nov 14, 2015

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
Copy link
Contributor

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
Copy link
Member Author

bluss commented Nov 16, 2015

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

@bluss
Copy link
Member Author

bluss commented Nov 16, 2015

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).

bors added a commit that referenced this pull request Jan 11, 2016
BinaryHeap: Use full sift down in .pop()

.sift_down can either choose to compare the element on the way down (and
place it during descent), or to sift down an element fully, then sift
back up to place it.

A previous PR changed .sift_down() to the former behavior, which is much
faster for relatively small heaps and for elements that are cheap to
compare.

A benchmarking run suggested that BinaryHeap::pop() suffers
improportionally from this, and that it should use the second strategy
instead. It's logical since .pop() brings last element from the
heapified vector into index 0, it's very likely that this element will
end up at the bottom again.

Closes #29969
Previous PR #29811
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants