BinaryHeap: Use full sift down in .pop() #30534
Conversation
.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.
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
r? @gankro |
|
Can you post the numbers, for posterity? |
|
Sure, they're here #29969 (comment) and the direct link is https://gist.github.com/bluss/b179df31a8c683c087c7 I guess they could be interpreted in different ways. It's only testing two different data set sizes, so it's not exactly a well resolved picture of the asymptotics. |
|
TL;DR version for time travellers (EDIT: THIS IS BACKWARDS, SEE BELOW): This reduces benchmark time to be 1/2 to 1/5th for most inputs (small heaps, or cheap comparisons), but for certain inputs makes the benchmark time 2x slower (large heap of expensive comparisons). |
|
ARGH. Ok I had it backwards. This PR is is basically regressing the latest perf of pop ops for small/simple cases in favour of asymptotic gains on large/complex cases. I'm used to integer/float keys for heaps, so this seems backwards! CC @rust-lang/libs, what do you think we should optimize more for? |
|
cc @dgrunwald |
alexcrichton
added
the
T-libs-api
|
We discussed this at the libs team triage today. There was some wondering if we could dynamically (or statically?) branch on anything to "guess" which strategy would be best. In particular, we could check the size of the heap? The "large heap" case seems like the more pressing one than the "complex key" one. What do you think? |
|
I'd prefer to merge this change, and put down improved BinaryHeap strategy in the issue list. I'm unlikely to set out for such a major project right now. |
|
Ok, that seems reasonable. @bors r+ |
|
📌 Commit 52883ab has been approved by |
|
⌛ Testing commit 52883ab with merge ffdfeeb... |
|
💔 Test failed - auto-win-gnu-64-nopt-t |
|
@bors: retry On Mon, Jan 11, 2016 at 3:24 PM, bors [email protected] wrote:
|
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
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