Weak pointers #14

vitvakatu · May 8, 2017

Weak pointers are used to avoid memory leaking when trying to drop structures with cycled linking (e.g. Base has pointers to Childs and every Child has pointer to `Base)

Implementation is very rough atm, there're some places where we need to optimize performance or to provide better API.
I'm looking for your feedback.

Fixes #9

kvark · May 8, 2017

kvark requested changes May 8, 2017

View changes

Unfortunately, I don't think this is going to work just yet.

Supposing you have a weak pointer. It's counter is in meta[id].weak. The item loses the last strong pointer, gets added to the free list, then gets re-used. It's weak counter is reset then. Will you be able to access the newly created element by an old weak pointer?

Here are 2 suggestions to make it great:

have an epoch associated with any weak pointer and stored in the meta. This way you'll be able to detect when an old weak pointer is trying to access the newer element of the storage.
hide the whole thing behind a feature. Since it's expanding a meta item size from 1 word to 3, the cache utilization will suffer, and we don't want most of the users to pay for that.

kvark · May 8, 2017

examples/weak.rs

+
+struct Base {
+    value: i32,
+    childs: Vec<Pointer<Child>>,


nit: children

kvark · May 8, 2017

examples/weak.rs

+        };
+        println!("{} has parent with value {}", child.value, parent_value);
+    }
+}


ni: newline missing

kvark · May 8, 2017

examples/weak.rs

+
+    base.childs.push(child1.clone());
+    base.childs.push(child2.clone());
+    let base_storage = Storage::new();


it's great to have cross-storage support, but maybe we should make the example really simple and use a single storage instead? just for clarity - people will use it for learning purposes, not just feature demonstration

kvark · May 8, 2017

src/lib.rs

+        for index in pending.sub_ref_strong.drain(..) {
+            s.meta[index].strong -= 1;
+            if s.meta[index].strong == 0 {
+                s.meta[index].weak -= 1;


where does this come from?

kvark · May 8, 2017

kvark reviewed May 8, 2017

View changes

kvark · May 8, 2017

src/lib.rs

-            sub_ref: Vec::new(),
+            add_ref_strong: Vec::new(),
+            sub_ref_strong: Vec::new(),
+            add_ref_weak: Vec::new(),


I wonder why do we even need to store the weak counters? They aren't holding anything alive anyway, so we might as well not bother.

kvark · May 8, 2017

kvark reviewed May 8, 2017

View changes

kvark · May 8, 2017

src/lib.rs

-    sub_ref: Vec<usize>,
+    add_ref_strong: Vec<usize>,
+    sub_ref_strong: Vec<usize>,
+    add_ref_weak: Vec<usize>,


Do we actually need to store the weak pointer count in the meta? It's not obvious to me, why we'd do that.

vitvakatu · May 8, 2017

Well, it was a big mistake. I misunderstood the implementation of Weak from standard library.
Now I rewrote the whole stuff with attention to your notices.
It's not ready yet, need to add epoch or similar mechanic, as you noticed.

kvark · May 9, 2017

kvark reviewed May 9, 2017

View changes

I think we are almost there. A few more notes.

kvark · May 9, 2017

src/lib.rs

+impl<T> Pointer<T> {
+    /// Creates a new `WeakPointer` to this component.
+    pub fn downgrade(&self) -> WeakPointer<T> {
+        let epoch = self.target.read().unwrap().epoch[self.index];


I'd like to never ever lock the target of a pointer inside the pointer methods. Imagine locking a storage for writing, then trying to downgrade one of its pointers. BAM, deadlock!
Instead, every strong pointer should carry the epoch internally, passed at creation, since the one it target.epoch is guaranteed to be the same for the lifetime of any such pointer.

kvark · May 9, 2017

src/lib.rs

+        match self.target.upgrade().and(self.pending.upgrade()) {
+            None => None,
+            Some(arc) => {
+                let target = self.target.upgrade().unwrap();


calling self.target.upgrade() twice - let's just bail out early:

let target = match self.target.upgrade() { Some(target) => target, None => return None, };

side note: I wonder if/when the ? syntax would start working with options, it would help here

kvark · May 9, 2017

src/lib.rs

+            None => None,
+            Some(arc) => {
+                let target = self.target.upgrade().unwrap();
+                if target.read().unwrap().meta[self.index] == 0 ||


Again, I think locking the target here is just asking for a deadlock.
Perhaps, we could have upgrade as a method of Storage instead? (or ReadLock/WriteLock - needs investigation)

Well, interesting proposal.
I think upgrade as Storage method makes no sense - you still need lock.
As the most often use case will be accessing Pointer soon after upgradeing it, we can add new methods for Locks: access_weak and access_weak_mut which will return Result<Pointer<T>>. But we still need regular upgrade method.

If possible, I'd like the pointer to not even be able to lock the storage. The fact it's currently possible is purely accidental.

access_weak and access_weak_mut which will return Result<Pointer>.
that would be inconsistent with our current access semantics

But we still need regular upgrade method.

Do we? That use-case is not obvious to me.

Oh, I made a mistake. Not Result<Pointer<T>>, but Result<&T> and Result<&mut T>.
As for upgrade method - I'm not sure it's useful, but in some situations user can need store Pointer produced by Weak

kvark · May 9, 2017

src/lib.rs

+                let target = self.target.upgrade().unwrap();
+                if target.read().unwrap().meta[self.index] == 0 ||
+                    target.read().unwrap().epoch[self.index] != self.epoch {
+                    return None;


I think we can introduce richer semantic here at no cost. Make it return a Result, where the error may be either the storage itself is dead, or the element is dead, or the epoch is wrong.

kvark · May 9, 2017

src/lib.rs

+impl<T> PartialEq for WeakPointer<T> {
+    fn eq(&self, other: &WeakPointer<T>) -> bool {
+        match self.upgrade().and(other.upgrade()) {
+            Some(other_arc) => self.upgrade().unwrap() == other_arc,


similarly, this is calling self.upgrade twice

do we even need to upgrade the pointers for comparison?..

Do we even need to compare pointers? :)

It's not critical but could be useful, I think

kvark · May 10, 2017

kvark requested changes May 10, 2017

View changes

kvark · May 10, 2017

src/lib.rs

            target: self.storage.clone(),
            pending: self.pending.clone(),
        }
    }
+
+    /// Upgrades the `WeakPointer` to an `Pointer`, if possible.


"an" -> "a"

kvark · May 10, 2017

src/lib.rs

+/// TODO: control by a cargo feature
+type Epoch = u16;
+
+pub enum UpgradeErr {


that must be issuing a warning for missing docs

kvark · May 10, 2017

src/lib.rs

 /// Shared pointer to the pending updates.
 type PendingRef = Arc<Mutex<Pending>>;
+type WeakPendingRef = Weak<Mutex<Pending>>;


this, as well as WeakStorageRef, are only used once. Perhaps, these typedefs are not really useful here

kvark · May 10, 2017

src/lib.rs

+    index: usize,
+    epoch: Epoch,
+    target: WeakStorageRef<T>,
+    pending: WeakPendingRef,


I don't think we need to fiddle with the weak pending reference, let's just use PendingRef here

kvark · May 10, 2017

src/lib.rs

            target: self.storage.clone(),
            pending: self.pending.clone(),
        }
    }
+
+    /// Upgrades the `WeakPointer` to an `Pointer`, if possible.


"an" -> "a"

kvark · May 10, 2017

src/lib.rs

            target: self.storage.clone(),
            pending: self.pending.clone(),
        }
    }
+
+    /// Upgrades the `WeakPointer` to an `Pointer`, if possible.
+    /// Returns `Err` if the strong count has reached zero and the inner value was destroyed.


"and" -> "or"

kvark · May 10, 2017

src/lib.rs

+
+    /// Upgrades the `WeakPointer` to an `Pointer`, if possible.
+    /// Returns `Err` if the strong count has reached zero and the inner value was destroyed.
+    pub fn upgrade(&self, ptr: &WeakPointer<T>) -> Result<Pointer<T>, UpgradeErr> {


we'll need to seriously make this thing nice
first off, when epoch is bumped on deletion, this code no longer needs to check for the refcount == 0
secondly, now that we don't need guard, we don't actually need &self to be ReadLock/WriteLock either!
so we can make upgrade just a method of WeakPointer, as you initially wanted, but without the danger of dead-locking the Storage.

kvark · May 10, 2017

src/lib.rs

+                if pending.epoch[ptr.index] != ptr.epoch {
+                    return Err(UpgradeErr::WrongEpoch);
+                }
+                else {


else is not needed

kvark · May 10, 2017

src/lib.rs

+                }
+                else {
+                    pending.add_ref.push(ptr.index);
+                    return Ok(Pointer {


return is not needed

kvark · May 10, 2017

kvark requested changes May 10, 2017

View changes

One more step!

kvark · May 10, 2017

src/lib.rs

+}
+
+impl Pending {
+    fn prepare_sub_drain(&mut self) -> (&mut Vec<usize>, &Vec<Epoch>) {


let's return (Drain<usize>, &[Epoch]) pair here and rename the method to drain_sub

kvark · May 10, 2017

src/lib.rs

@@ -213,6 +289,14 @@ impl<'a, T> ReadLock<'a, T> {
        &self.guard.data[ptr.index]
    }

+    /// Borrow a weak pointed component for reading.
+    pub fn access_weak(&self, pointer: &WeakPointer<T>) -> Result<&T, UpgradeErr> {


I don't think access_weak brings anything on the table, really. It may seem useful, but you can do the same from user space and that would be as efficient, so I'd be in favour of reducing the API surface here

kvark · May 10, 2017

src/lib.rs

            s.meta[index] -= 1;
            if s.meta[index] == 0 {
-                s.free_list.push(index);
+                s.free_list.push((index, epoch[index] + 1));


I think we actually need to bump the epoch here, as in epoch[index] += 1, unless I'm missing the place where you are already doing that.

kvark · May 11, 2017

kvark approved these changes May 11, 2017

View changes

kvark · May 11, 2017

examples/weak.rs

-            storage.read()
-            .access_weak(next)
-            .ok().map_or("None".into(), |item| item.value.clone())
+        let value = node.next.as_ref().map_or("None".into(), |ref next| {


this seems quite complicated, I'm going to think on reducing it, but it's not essential to the PR

kvark · May 11, 2017

src/lib.rs

@@ -55,8 +55,8 @@ struct Pending {
 }

 impl Pending {
-    fn prepare_sub_drain(&mut self) -> (&mut Vec<usize>, &Vec<Epoch>) {
-        (&mut self.sub_ref, &self.epoch)
+    fn drain_sub(&mut self) -> (std::vec::Drain<usize>, &mut [Epoch]) {


niiiice. Just one nit: make the use of Drain consistent with the rest of the code, so have use std::vec::Drain at the top (right after use std::sync... line).

kvark · May 11, 2017

kvark reviewed May 11, 2017

View changes

kvark · May 11, 2017

examples/weak.rs

+
+    for node in storage.read().iter() {
+        let value = node.next.as_ref().map_or("None".into(), |ref next| {
+            next.upgrade().ok().map_or("None".into(), |ref ptr| {


can't we just unwrap here, considering that we know the storage and target elements are alive?

We can, of course. I'll do it to simplify example, but I think we need to reduce it somehow.

kvark · May 11, 2017

IT HAPPENED! 🍰 🍰 🍰

Merge remote-tracking branch 'upstream/master'

3c162cd

kvark self-requested a review May 8, 2017

Merge remote-tracking branch 'upstream/master'

3171693

vitvakatu force-pushed the vitvakatu:weak-pointers branch from 2c33185 to 1abf891 May 8, 2017

Merge remote-tracking branch 'upstream/master'

b2fc8fc

vitvakatu force-pushed the vitvakatu:weak-pointers branch from 1abf891 to 853a4b5 May 8, 2017

vitvakatu force-pushed the vitvakatu:weak-pointers branch from d3b6dac to 225b49f May 10, 2017

vitvakatu added some commits May 8, 2017

Add: weak example

7dc9184

Add: WeakPointer

90df145

Fix downgrading and upgrading

d7bca86

Introduce epoch

5bf5e54

Rewrite PartialEq for WeakPointer

5b8ec69

Restructuration

Loading status checks…

cbc2238

vitvakatu force-pushed the vitvakatu:weak-pointers branch from 225b49f to cbc2238 May 10, 2017

Storing `epoch` in `free_list`

4c13742

vitvakatu added some commits May 10, 2017

Moving `upgrade` back

f2fbdba

Updating docs

1b65343

Introduce `access_weak` and `access_weak_mut`

Loading status checks…

79053ac

Fix unneeded return

Loading status checks…

729c698

vitvakatu added some commits May 10, 2017

Revert "Introduce `access_weak` and `access_weak_mut`"
This reverts commit 79053ac.

8991104

Fix drain_sub

Loading status checks…

34a2bee

kvark changed the title ~~[WIP] Weak pointers~~ Weak pointers May 11, 2017

Simplifying example

Loading status checks…

a0a26c5

kvark merged commit 64fa5b2 into kvark:master May 11, 2017
1 check passed

1 check passed

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

vitvakatu deleted the vitvakatu:weak-pointers branch May 11, 2017

kvark/froggy

Join GitHub today

Weak pointers #14

Conversation

vitvakatu commented May 8, 2017 • edited

kvark self-requested a review May 8, 2017

vitvakatu force-pushed the vitvakatu:weak-pointers branch from 2c33185 to 1abf891 May 8, 2017

kvark requested changes May 8, 2017 View changes

kvark left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

kvark reviewed May 8, 2017 View changes

This comment has been minimized.

kvark reviewed May 8, 2017 View changes

This comment has been minimized.

vitvakatu force-pushed the vitvakatu:weak-pointers branch from 1abf891 to 853a4b5 May 8, 2017

This comment has been minimized.

vitvakatu commented May 8, 2017 • edited

kvark reviewed May 9, 2017 View changes

kvark left a comment

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.

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.

vitvakatu force-pushed the vitvakatu:weak-pointers branch from d3b6dac to 225b49f May 10, 2017

vitvakatu added some commits May 8, 2017

vitvakatu force-pushed the vitvakatu:weak-pointers branch from 225b49f to cbc2238 May 10, 2017

kvark requested changes May 10, 2017 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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

vitvakatu added some commits May 10, 2017

kvark requested changes May 10, 2017 View changes

kvark left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

vitvakatu added some commits May 10, 2017

kvark approved these changes May 11, 2017 View changes

This comment has been minimized.

This comment has been minimized.

kvark reviewed May 11, 2017 View changes

This comment has been minimized.

This comment has been minimized.

kvark changed the title [WIP] Weak pointers Weak pointers May 11, 2017

Hide details View details kvark merged commit 64fa5b2 into kvark:master May 11, 2017 1 check passed

1 check passed

This comment has been minimized.

kvark commented May 11, 2017

vitvakatu deleted the vitvakatu:weak-pointers branch May 11, 2017

vitvakatu commented May 8, 2017 •

edited

vitvakatu force-pushed the vitvakatu:weak-pointers branch from `2c33185` to `1abf891` May 8, 2017

kvark requested changes May 8, 2017

View changes

kvark reviewed May 8, 2017

View changes

kvark reviewed May 8, 2017

View changes

vitvakatu force-pushed the vitvakatu:weak-pointers branch from `1abf891` to `853a4b5` May 8, 2017

vitvakatu commented May 8, 2017 •

edited

kvark reviewed May 9, 2017

View changes

vitvakatu force-pushed the vitvakatu:weak-pointers branch from `d3b6dac` to `225b49f` May 10, 2017

vitvakatu force-pushed the vitvakatu:weak-pointers branch from `225b49f` to `cbc2238` May 10, 2017

kvark requested changes May 10, 2017

View changes

kvark requested changes May 10, 2017

View changes

kvark approved these changes May 11, 2017

View changes

kvark reviewed May 11, 2017

View changes

kvark merged commit `64fa5b2` into kvark:master May 11, 2017
1 check passed