collections: Make BinaryHeap panic safe in sift_up / sift_down #25856

bluss · May 28, 2015

collections: Make BinaryHeap panic safe in sift_up / sift_down

Use a struct called Hole that keeps track of an invalid location
in the vector and fills the hole on drop.

I include a run-pass test that the current BinaryHeap fails, and the new
one passes.

NOTE: The BinaryHeap will still be inconsistent after a comparison fails. It will
not have the heap property. What we fix is just that elements will be valid
values.

This is actually a performance win -- the new code does not bother to write in zeroed()
values in the holes, it just leaves them as they were.

Net result is something like a 5% decrease in runtime for BinaryHeap::from_vec. This
can be further improved by using unchecked indexing (I confirmed it makes a difference,
not a surprise with the non-sequential access going on), but let's leave that for another PR.
Safety first 😉

Fixes #25842

rust-highfive · May 28, 2015

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

bluss · May 28, 2015

I prototyped this using a general scope guard, but this Hole representation turned out to be nice.

LLVM needs to be on our side here and see that the removed element's Option<T>
is always Some, etc. If you wonder why I use hole.pos() in one location and hole.pos
in another, it's because those choices benched the best, both locations. Optimizing
compilers, they are chaotic.

Edited: Moved useful info from this comment into the merge message itself (above)

cc @alexcrichton @Gankro

bluss · May 28, 2015

Using only unchecked indexing in Hole removes the bench result dependence on hole.pos vs hole.pos() -- just shows how panicking can be an impediment to optimization.

alexcrichton · May 28, 2015

alexcrichton reviewed May 28, 2015

View changes

alexcrichton · May 28, 2015

src/libcollections/binary_heap.rs

+
+impl<'a, T> Hole<'a, T> {
+    /// Create a new Hole at index `pos`.
+    pub fn new(data: &'a mut [T], pos: usize) -> Self {


Could the pub be dropped from these functions? (Hole isn't exposed so they shouldn't need to be exposed either)

It makes the code more portable, though. e.g. we can happily move this to a module without worrying.

It came out with pub, I think pub is natural as soon as the methods are intended to be called from outside the struct itself. I know our privacy doesn't work that way, but it does once the code grows and yes you move it out to a module. I'm fine either way but I prefer pub.

Idiomatically code in the rest of the standard library does not do this, so let's stick to existing conventions. We can add pub if necessary at a later date, but code should be as conservative as possible in exports today.

ok that's fine

alexcrichton · May 28, 2015

Looks good to me, thanks @bluss! Perhaps the test could also be modified to ensure that the destructor for each element in the heap isn't run more than once?

Other than that though r=me, we can always tweak the performance at a later date.

cc @Gankro

bluss · May 28, 2015

Thanks. I'll add a test that makes sure no destructors were called right after catching the panic, before I inspect the data further. That should cover it (and fails on old version of BinaryHeap).

Gankro · May 28, 2015

Gankro reviewed May 28, 2015

View changes

Gankro · May 28, 2015

src/libcollections/binary_heap.rs

-                ptr::write(&mut self.data[pos], x);
-                pos = parent;
+            while hole.pos() > start {
+                let parent = (hole.pos() - 1) >> 1;


Surely LLVM can convert the much more semantically clear /2 to this?

I'll fix it. Haven't really looked at changing anything besides what's needed, though.

Gankro · May 28, 2015

Gankro reviewed May 28, 2015

View changes

Gankro · May 28, 2015

src/libcollections/binary_heap.rs

+/// position with the value that was originally removed.
+struct Hole<'a, T: 'a> {
+    data: &'a mut [T],
+    elt: Option<T>,


Perhaps worth noting that this value is Some until this value is Dropped

Gankro · May 28, 2015

Gankro reviewed May 28, 2015

View changes

Gankro · May 28, 2015

src/libcollections/binary_heap.rs

+        debug_assert!(index != self.pos);
+        let old_pos = self.pos;
+        let x = ptr::read(&mut self.data[index]);
+        ptr::write(&mut self.data[old_pos], x);


Should this not just be a copy_nonoverlapping?

That's a good idea for here. Haven't really looked at changing anything besides what's needed.

Can we leave it for now? It requires switching to unchecked indexing.

You don't need unchecked indexing, you just need to cache &self.data[index] in a *const for a line. Right?

(also at very least, you shouldn't be reading from an &mut :) )

I had a feeling something was off. I promise, a feeling.

Gankro · May 28, 2015

This looks great!

bluss · May 28, 2015

PR updated. Everything is addressed, removed pub. The test checks for number of drop calls now (old binary heap used to drop 1 on panic), but it could still have tracked drops in even more detail.

bluss · May 28, 2015

wait, oh I totally thought I had spotted a bug and wondered how that could have slipped my tests, but no, it's fine. Pushed an update with one scope less that I didn't need, so the diff for sift_down is easier to read.

Gankro · May 28, 2015

@bors r+

Sweet!

bors · May 28, 2015

📌 Commit 5249cbb has been approved by Gankro

bors · May 28, 2015

⌛️ Testing commit 5249cbb with merge efebe45...

bors · May 28, 2015

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, 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

huonw · May 29, 2015

Nice work @bluss!

alexcrichton · Jun 9, 2015

triage: beta-accepted

rust-highfive assigned brson May 28, 2015

bluss force-pushed the bluss:binary-heap-hole branch from 5f44f5b to b221853 May 28, 2015

alexcrichton added the beta-nominated label May 28, 2015

bluss force-pushed the bluss:binary-heap-hole branch from b221853 to c41a5cc May 28, 2015

bluss force-pushed the bluss:binary-heap-hole branch from c41a5cc to 5249cbb May 28, 2015

alexcrichton added the T-libs label May 28, 2015

bors merged commit 5249cbb into rust-lang:master May 28, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

bluss deleted the bluss:binary-heap-hole branch May 28, 2015

alexcrichton added the beta-accepted label Jun 9, 2015

alexcrichton referenced this pull request Jun 10, 2015
Merged
Backport accepted PRs to beta #26161

alexcrichton removed the beta-nominated label Jun 11, 2015

rust-lang/rust

Join GitHub today

collections: Make BinaryHeap panic safe in sift_up / sift_down #25856

Conversation

bluss commented May 28, 2015

rust-highfive assigned brson May 28, 2015

This comment has been minimized.

rust-highfive commented May 28, 2015

This comment has been minimized.

bluss commented May 28, 2015

bluss force-pushed the bluss:binary-heap-hole branch from 5f44f5b to b221853 May 28, 2015

This comment has been minimized.

bluss commented May 28, 2015

alexcrichton reviewed May 28, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton commented May 28, 2015

alexcrichton added the beta-nominated label May 28, 2015

This comment has been minimized.

bluss commented May 28, 2015

Gankro reviewed May 28, 2015 View changes

This comment has been minimized.

This comment has been minimized.

Gankro reviewed May 28, 2015 View changes

This comment has been minimized.

This comment has been minimized.

Gankro reviewed May 28, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Gankro commented May 28, 2015

bluss force-pushed the bluss:binary-heap-hole branch from b221853 to c41a5cc May 28, 2015

This comment has been minimized.

bluss commented May 28, 2015

bluss force-pushed the bluss:binary-heap-hole branch from c41a5cc to 5249cbb May 28, 2015

This comment has been minimized.

bluss commented May 28, 2015

This comment has been minimized.

Gankro commented May 28, 2015

This comment has been minimized.

bors commented May 28, 2015

alexcrichton added the T-libs label May 28, 2015

This comment has been minimized.

bors commented May 28, 2015

bors added a commit that referenced this pull request May 28, 2015

This comment has been minimized.

bors commented May 28, 2015

Hide details View details bors merged commit 5249cbb into rust-lang:master May 28, 2015 2 checks passed

2 checks passed

bluss deleted the bluss:binary-heap-hole branch May 28, 2015

This comment has been minimized.

huonw commented May 29, 2015

This comment has been minimized.

alexcrichton commented Jun 9, 2015

alexcrichton added the beta-accepted label Jun 9, 2015

alexcrichton referenced this pull request Jun 10, 2015

alexcrichton removed the beta-nominated label Jun 11, 2015

bluss force-pushed the bluss:binary-heap-hole branch from `5f44f5b` to `b221853` May 28, 2015

alexcrichton reviewed May 28, 2015

View changes

Gankro reviewed May 28, 2015

View changes

Gankro reviewed May 28, 2015

View changes

Gankro reviewed May 28, 2015

View changes

bluss force-pushed the bluss:binary-heap-hole branch from `b221853` to `c41a5cc` May 28, 2015

bluss force-pushed the bluss:binary-heap-hole branch from `c41a5cc` to `5249cbb` May 28, 2015

bors merged commit `5249cbb` into rust-lang:master May 28, 2015
2 checks passed