Implement placement-in protocol for `HashMap` #40390

F001 · 2017-03-09T14:38:17Z

nagisa · 2017-03-09T16:53:05Z

While this works technically, the implementation is not correct. The point of the placement-in protocol is to put value directly into some place, in this case into the HashMap in such a way that copies are avoided. So the Place::pointer should return a pointer to some place directly inside the HashMap allocated storage, and not field in EntryPlace.

To implement this you will likely need to do some internal changes to the Entry(-ies), so it would be possible to obtain a pointer for both Vacant and Occupied entry.

See: rust-lang#40390

nrc · 2017-03-09T20:09:57Z

@bors: delegate @nagisa

F001 · 2017-03-09T22:11:19Z

Thank you for the review comment!

cc @arthurprs Please correct me if anything is wrong.

I used a temporary field to store the value because of panic safety.

AFAK, if the Place::pointer need to return a pointer to some place directly inside the HashMap allocated storage, I have to do robin_hood first in make_place phase. This will affect existing elements in HashMap. If panic occurs later, I don't know any roll back mechanism to restore valid state. It is the main difference of the implementation of placement-in between VecDeque and HashMap.

I'm looking forward to your suggestions.

arthurprs · 2017-03-10T09:21:28Z

Your suggestion sounds ok to me. It will avoid unnecessary V copies for Entry::Vacant. To avoid any unnecessary V copies for Entry::Occupied you probably need a variant of robin_hood that will make space without copying the uninitialized V into the bucket.

For rollback you can implement Drop for EntryPlace (drop still runs in case of panics) and use pop_internal to fix the table if it comes to that (forget what it returns). BinaryHeap uses a similar strategy to avoid corrupting the structure if T comparisons panics.

F001 · 2017-03-10T15:25:15Z

Thanks for your suggestion. I have updated the implementation. For now, it can avoid unnecessary V copy for Entry::Vacant. I'll continue to investigate more optimization.

arthurprs · 2017-03-10T16:21:47Z

src/libstd/collections/hash/map.rs

+           issue = "30172")]
+pub struct EntryPlace<'a, K: 'a, V: 'a> {
+    bucket: Option<FullBucketMut<'a, K, V>>,
+    panicked: Cell<bool>,


I suggest using a finalized flag instead. Also, the flag should probably be the last field as it may save 7 bytes of stack.

You are unlikely to have more than one of these per hashmap alive at a time, so this is not very concerning. Also we’re getting field reordering soon, which will do this for everybody automatically.

As suggested below, using forget can avoid the flag.

arthurprs · 2017-03-10T16:22:47Z

src/libstd/collections/hash/map.rs

+           reason = "struct name and placement protocol is subject to change",
+           issue = "30172")]
+pub struct EntryPlace<'a, K: 'a, V: 'a> {
+    bucket: Option<FullBucketMut<'a, K, V>>,


I'm probably missing something obvious but do we really need to wrap the bucket with Option?

I'm just lazy that I want to use existing FullBucket::take to remove the entry. It takes a self parameter. But in the drop method, there is only &mut self, the bucket field can't move.

It is fixed by adding another FullBucket::remove method, which takes a &mut self parameter. In drop method, I can call this remove now.

arthurprs · 2017-03-10T16:27:15Z

src/libstd/collections/hash/map.rs

+impl<'a, K, V> InPlace<V> for EntryPlace<'a, K, V> {
+    type Owner = ();
+
+    unsafe fn finalize(self) {


Thinking a bit more about this you can forget(self) here, avoiding the flag altogether.

Good suggestion. Fixed.

nagisa

Nice improvement over the previous version! @arthurprs’ notes seem very relevant (and they are also much more familiar with the HashMap code), so these should be fixed.

nagisa · 2017-03-10T16:59:17Z

src/libstd/collections/hash/map.rs

+           issue = "30172")]
+pub struct EntryPlace<'a, K: 'a, V: 'a> {
+    bucket: Option<FullBucketMut<'a, K, V>>,
+    panicked: Cell<bool>,


You are unlikely to have more than one of these per hashmap alive at a time, so this is not very concerning. Also we’re getting field reordering soon, which will do this for everybody automatically.

nagisa · 2017-03-10T17:14:57Z

I realised there’s one possible alternative in behaviour. Current implementation tries to recover the previous value if the placement expression fails, however it is not obvious to me whether this is a better approach compared to, say, simply making the key vacant in case of panic.

Here are some points in favour of leaving the entry vacant instead of restoring the value if panic happens:

Saving the old value involves a copy, thus negating most/all of the point of placement-in (as few copies as possible);
Panicking is a very exceptional situation that is not supposed to be recovered from. This means that all the Drop should be responsible for is restoring HashMap into a state that’s safe to Drop, that’s all. Making entry vacant seems equivalent to restoring the old value in that sense.

arthurprs · 2017-03-10T17:32:45Z

Very good points, leaving a previous filled bucket empty on panic sounds reasonable.

nagisa · 2017-03-10T19:40:04Z

cc @rust-lang/libs

aturon · 2017-03-10T19:40:44Z

cc @rust-lang/libs, anyone have feedback on @nagisa's last comment?

sfackler · 2017-03-10T20:49:40Z

I agree that the precise state of the value being modified doesn't matter too much.

nagisa · 2017-03-11T08:37:06Z

src/libstd/collections/hash/table.rs

+        self.table.size -= 1;
+        unsafe {
+            *self.raw.hash = EMPTY_BUCKET;
+            ptr::read(self.raw.pair); // drop right now


This is possibly incorrect. I think you’ll notice why if you add a test that looks like this (you probably should one similar to it):

struct Banana<'a>(&'a mut bool); impl Drop for Banana { fn drop(&mut self) { if !*self.0 { panic!("double drop!"); } *self.0 = false; } } let mut hm = HashMap::new(); let mut can_drop = true; hm.insert(0, Banana(&mut can_drop)); hm.entry(0) <- panic!("boom") ; // first drop happens in `make_place`, where the `Banana(true)` gets dropped and `can_drop` is set to false // then a `*place.pointer() = panic!("boom")` is executed, which unwinds, thus dropping the place // place destructor drops the `Banana(false)`, and thus double-panic occurs and the process aborts. // // In other words, current implementation of Drop reads uninitialized memory.

Note: that the code might not reproduce exactly the way I described it, but it is still reading uninitialized memory.

Yeah! Good point. Fixed.

nagisa

2 more, likely final, tweaks.

nagisa · 2017-03-11T08:40:39Z

src/libstd/collections/hash/table.rs

+        self.table.size -= 1;
+        unsafe {
+            *self.raw.hash = EMPTY_BUCKET;
+            ptr::read(self.raw.pair); // drop right now


Note: that the code might not reproduce exactly the way I described it, but it is still reading uninitialized memory.

nagisa · 2017-03-11T08:41:16Z

src/libstd/collections/hash/map.rs

+        let b = match self {
+            Occupied(mut o) => {
+                let uninit = unsafe { mem::uninitialized() };
+                o.insert(uninit);


You can avoid doing this mem::uninitialized dance by simply doing a

std::ptr::drop_in_place(o.elem.bucket.read_mut().1)

arthurprs · 2017-03-11T08:45:23Z

src/libstd/collections/hash/map.rs

+           issue = "30172")]
+impl<'a, K, V> Drop for EntryPlace<'a, K, V> {
+    fn drop(&mut self) {
+        self.bucket.remove();


I think this will drop and uninitialized V as you only inserted the key?

Yes, nagisa has mentioned this. I'm fixing it.

nagisa

I’ve only got nits left. Marking the functions internal functions as unsafe makes sense as they leave around uninitialized data which the caller should handle appropriately.

r=me once nits are fixed

nagisa · 2017-03-11T11:50:57Z

src/libstd/collections/hash/map.rs

+        assert_eq!(map.len(), 9);
+        assert!(!map.contains_key(&100));
+
+        // correctly drop


This can probably be factored out into a separate test. (i.e. a different #[test] function)

nagisa · 2017-03-11T11:54:02Z

src/libstd/collections/hash/table.rs

+
+    /// Remove this bucket's key and value from the hashtable.
+    /// Only used for inplacement insertion.
+    pub fn remove_key(&mut self) {


Similarly here, whole function unsafe.

nagisa · 2017-03-11T11:54:31Z

src/libstd/collections/hash/table.rs

+
+    /// Puts given key, remain value uinitialized.
+    /// It is only used for inplacement insertion.
+    pub fn put_key(mut self, hash: SafeHash, key: K) -> FullBucket<K, V, M> {


I’d probably make this whole function unsafe. (i.e. pub unsafe fn put key)

nagisa · 2017-03-11T11:58:45Z

src/libstd/collections/hash/map.rs

+    }
+
+    // Only used for InPlacement insert. Avoid unnecessary value copy.
+    fn insert_key(self) -> FullBucketMut<'a, K, V> {


This should probably be unsafe fn too.

nagisa · 2017-03-11T13:17:45Z

@bors r+

nagisa · 2017-03-11T13:18:44Z

Oh, bors didn’t notice the delegation above :/

eddyb · 2017-03-12T12:22:16Z

@bors delegate=nagisa

bors · 2017-03-12T12:22:17Z

✌️ @nagisa can now approve this pull request

nagisa · 2017-03-12T12:23:36Z

@bors r+

bors · 2017-03-12T12:23:37Z

📌 Commit 584c798 has been approved by nagisa

@nagisa

Implement placement-in protocol for `HashMap` CC rust-lang#30172 r? @nagisa

Rollup of 5 pull requests - Successful merges: #40369, #40390, #40426, #40449, #40453 - Failed merges:

nagisa mentioned this pull request Mar 9, 2017

Implement placement-in protocol for relevant data structures #30172

Closed

6 tasks

mattico added a commit to mattico/rust that referenced this pull request Mar 9, 2017

Initial broken implementation of placement insert for BTreeMap

3256e95

See: rust-lang#40390

nrc self-assigned this Mar 9, 2017

arthurprs reviewed Mar 10, 2017

View reviewed changes

nagisa reviewed Mar 10, 2017

View reviewed changes

nagisa reviewed Mar 11, 2017

View reviewed changes

arthurprs reviewed Mar 11, 2017

View reviewed changes

nagisa reviewed Mar 11, 2017

View reviewed changes

Implement placement-in protocol for HashMap

584c798

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 12, 2017

Rollup merge of rust-lang#40390 - F001:placementHashMap, r=nagisa

38e4853

Implement placement-in protocol for `HashMap` CC rust-lang#30172 r? @nagisa

frewsxcv mentioned this pull request Mar 12, 2017

Rollup of 5 pull requests #40455

Merged

bors added a commit that referenced this pull request Mar 12, 2017

Auto merge of #40455 - frewsxcv:rollup, r=frewsxcv

0066869

Rollup of 5 pull requests - Successful merges: #40369, #40390, #40426, #40449, #40453 - Failed merges:

bors merged commit 584c798 into rust-lang:master Mar 12, 2017

Implement placement-in protocol for HashMap #40390

Implement placement-in protocol for HashMap #40390

Conversation

F001 commented Mar 9, 2017

nagisa commented Mar 9, 2017

nrc commented Mar 9, 2017

F001 commented Mar 9, 2017 • edited Loading

arthurprs commented Mar 10, 2017 • edited Loading

F001 commented Mar 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arthurprs Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagisa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagisa commented Mar 10, 2017

arthurprs commented Mar 10, 2017

nagisa commented Mar 10, 2017

aturon commented Mar 10, 2017

sfackler commented Mar 10, 2017

nagisa Mar 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagisa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagisa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nagisa commented Mar 11, 2017 • edited Loading

nagisa commented Mar 11, 2017

eddyb commented Mar 12, 2017

bors commented Mar 12, 2017

nagisa commented Mar 12, 2017

bors commented Mar 12, 2017

Implement placement-in protocol for `HashMap` #40390

Implement placement-in protocol for `HashMap` #40390

F001 commented Mar 9, 2017 •

edited

Loading

arthurprs commented Mar 10, 2017 •

edited

Loading

arthurprs Mar 10, 2017 •

edited

Loading

nagisa Mar 11, 2017 •

edited

Loading

nagisa commented Mar 11, 2017 •

edited

Loading