Skip to content

Implement placement-in protocol for BinaryHeap #39062

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jan 20, 2017
Merged

Implement placement-in protocol for BinaryHeap #39062

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fb3483c
Add initial impl of placement-in for `BinaryHeap`
Jan 14, 2017
90fbe15
Fix BinaryHeap place by only constructing vec::PlaceBack once
Jan 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 76 additions & 1 deletion src/libcollections/binary_heap.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@
#![allow(missing_docs)]
#![stable(feature = "rust1", since = "1.0.0")]

use core::ops::{Deref, DerefMut};
use core::ops::{Deref, DerefMut, Place, Placer, InPlace};
use core::iter::{FromIterator, FusedIterator};
use core::mem::{swap, size_of};
use core::ptr;
Expand Down Expand Up @@ -688,6 +688,22 @@ impl<T: Ord> BinaryHeap<T> {
}
}

fn sift_up_ind(&mut self, start: usize, pos: usize) -> usize {
Copy link
Member

Choose a reason for hiding this comment

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

You can simply adjust the sift_up to return the index, since its a private implementation detail and will also avoid the code duplication.

unsafe {
// Take out the value at `pos` and create a hole.
let mut hole = Hole::new(&mut self.data, pos);

while hole.pos() > start {
let parent = (hole.pos() - 1) / 2;
if hole.element() <= hole.get(parent) {
return hole.pos();
}
hole.move_to(parent);
}
hole.pos()
}
}

/// Take an element at `pos` and move it down the heap,
/// while its children are larger.
fn sift_down_range(&mut self, pos: usize, end: usize) {
Expand Down Expand Up @@ -889,6 +905,19 @@ impl<T: Ord> BinaryHeap<T> {
}
}

impl<T> BinaryHeap<T>
where T: Clone + Ord {
/// kek
#[unstable(feature = "collection_placement",
reason = "placement protocol is subject to change",
issue = "30172")]
pub fn place(&mut self) -> PlaceIn<T> {
PlaceIn {
heap: self,
}
}
}

/// Hole represents a hole in a slice i.e. an index without valid value
/// (because it was moved from or duplicated).
/// In drop, `Hole` will restore the slice by filling the hole
Expand Down Expand Up @@ -1189,3 +1218,49 @@ impl<'a, T: 'a + Ord + Copy> Extend<&'a T> for BinaryHeap<T> {
self.extend(iter.into_iter().cloned());
}
}

#[unstable(feature = "collection_placement",
reason = "placement protocol is subject to change",
issue = "30172")]
pub struct PlaceIn<'a, T: 'a>
where T: Clone + Ord {
heap: &'a mut BinaryHeap<T>,
}

#[unstable(feature = "collection_placement",
reason = "placement protocol is subject to change",
issue = "30172")]
impl<'a, T> Place<T> for PlaceIn<'a, T>
where T: Clone + Ord {
fn pointer(&mut self) -> *mut T {
self.heap.data.place_back().pointer()
Copy link
Member

Choose a reason for hiding this comment

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

This seems… incorrect. Read the comment for Placer.

}
}

#[unstable(feature = "collection_placement",
reason = "placement protocol is subject to change",
issue = "30172")]
impl<'a, T> Placer<T> for PlaceIn<'a, T>
where T: Clone + Ord {
type Place = PlaceIn<'a, T>;

fn make_place(self) -> Self {
let _ = self.heap.data.place_back().make_place();
self
Copy link
Member

Choose a reason for hiding this comment

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

Implementation of this is going in the right direction but not quite there yet.

Probably the most notable issue here is that the Place that you create in the underlying vector gets immediatelly dropped, after which you are not allowed to put anything into the place in question anymore.

While this may work in current implementation, because PlaceBack for Vec has no Drop defined, consider what would happen if it had one:

// self.heap.data here is just big enough that a call to make_place would resize the vector.
let place = self.heap.data.place_back().make_place(); // resizes vector
drop(place); // resizes the vector back to the original size if was implemented weirdly
// now the placement protocol asks for a pointer to put data in
self.heap.data.place_back().pointer() // returns a pointer out of bounds!

This implementation therefore does not follow the placement protocol.

What ought be done instead is a new structure such as this (I think, feel free to explore alternative designs! generics omitted for brevity):

struct BinaryHeapPlace {
    heap: &mut BinaryHeap
    place: vec::PlaceBack
}

and then use the place in this structure to produce the pointer and also call the finalize on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried this initially, but quickly ran into problems, as vec::PlaceBack borrows the vec its created from. Might there be some clever way of bypassing this problem? I would like to reuse vec::PlaceBack somehow, as it does exactly what I try to do, and just throw in the heap.sift_up in InPlace::finalize, but it seems to be hard wrt. borrowing and aliasing stuff.

}
}

#[unstable(feature = "collection_placement",
reason = "placement protocol is subject to change",
issue = "30172")]
impl<'a, T> InPlace<T> for PlaceIn<'a, T>
where T: Clone + Ord {
type Owner = &'a T;

unsafe fn finalize(self) -> &'a T {
let len = self.heap.len();
let _ = self.heap.data.place_back().finalize();
let i = self.heap.sift_up_ind(0, len);
&mut self.heap.data[i]
Copy link
Member

Choose a reason for hiding this comment

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

Returning an immutable reference seems sensible, but you needn’t to do a &mut self.heap.data[i] here; while return value of finalize cannot be reused, you can still avoid a bounds check by simply doing self.heap.get_unchecked(i), as the index is guaranteed to be in bounds.

}
}
21 changes: 21 additions & 0 deletions src/libcollectionstest/binary_heap.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

use std::panic;
use std::collections::BinaryHeap;
use std::collections::binary_heap::{Drain, PeekMut};

Expand Down Expand Up @@ -310,6 +311,26 @@ fn test_extend_specialization() {
assert_eq!(a.into_sorted_vec(), [-20, -10, 1, 2, 3, 3, 5, 43]);
}

#[test]
fn test_placement() {
let mut a = BinaryHeap::new();
a.place() <- 2;
a.place() <- 4;
a.place() <- 3;
assert_eq!(a.peek(), Some(&4));
assert_eq!(a.len(), 3);
a.place() <- 1;
assert_eq!(a.into_sorted_vec(), vec![1, 2, 3, 4]);
}

#[test]
fn test_placement_panic() {
let mut heap = BinaryHeap::from(vec![1, 2, 3]);
fn mkpanic() -> usize { panic!() }
let _ = panic::catch_unwind(panic::AssertUnwindSafe(|| { heap.place() <- mkpanic(); }));
assert_eq!(heap.len(), 3);
}

#[allow(dead_code)]
fn assert_covariance() {
fn drain<'new>(d: Drain<'static, &'static str>) -> Drain<'new, &'new str> {
Expand Down