basic `drain` and `into_iter` implementations #33

soruh · 2020-01-25T23:21:04Z

I just took a quick look at implementing drain and into_iter.
The tests seem to pass, but that might just be luck.

This code should probably not be merged, since it seems horrendously inefficient, but maybe it can be used to work towards an actual implementation.

codecov · 2020-01-25T23:26:19Z

Codecov Report

Merging #33 into master will increase coverage by 0.73%.
The diff coverage is 92.46%.

Impacted Files	Coverage Δ
src/iter/iter.rs	`97.01% <100%> (-0.13%)`	⬇️
src/raw/mod.rs	`91.11% <100%> (+0.41%)`	⬆️
src/node.rs	`45.45% <100%> (ø)`	⬆️
src/map.rs	`89.94% <92.02%> (+1.23%)`	⬆️

…ry into implement_drain_and_into_iter

so that both threads start more syncronously

jonhoo · 2020-01-26T20:15:56Z

I'm a little strapped for time at the moment, but one thing to keep in mind is what exactly the contract is for drain. For example, what happens if I write the following code:

std::mem::forget(map.drain());

Is map now empty or full? What happens if I read one element and then drop the Drain? Take a look at hashbrown::RawDrain for some inspiration. I'm genuinely not sure what the best way to express this is a concurrent map, which is why I marked drain as hard in the issue :p

soruh · 2020-01-26T20:45:45Z

I assumed this would get hard, but I thought I'd just experiment with it for a bit and see how far I get...

I think there are two ways we can go about this:

have the Drain iterate over the map bin by bin, try to remove Elements as it goes, and return them if they weren't removed already.
take a snapshot of the Map's keys first and then remove them one by one.

The main difference is that the first option will also remove elements added during the drain while the second one is guaranteed to stop when it iterated over all the keys in it's snapshot, but has to store all of those keys somewhere...

Regarding the return Value there are also two options:

return an Option<V> for each V we tried to remove
ignore removes that returned None and only return V

But the first option will result in returning an Option<Option<V>> but will let the user know that an entry they tried to drain was removed. (I don't really know what they would do with that information, but it could be useful)

I personally tend towards not Snapshotting the map, just returning a V, and ignoring entries that were already removed.

I'd be interested in hearing your opinion on this, but I probably also won't have too much free time until next weekend.

cuviper

ISTM that since you clone the keys, you should also be cloning the values.

Alternatively, for Drain you could just return references (&'g K, &'g V) which the user could choose to clone. Then the difference from plain Iter is just that they are marked for removal along the way.

IntoIter should have exclusive ownership of the map, no outstanding references, so it just needs a way for the map to avoid re-dropping items that the iterator already returned, like a true immediate removal.

cuviper · 2020-01-29T17:51:16Z

src/iter/iter.rs

+
+            if let Some(value) = self.map.remove(&node.key, self.guard) {
+                return Some((node.key.clone(), unsafe {
+                    std::ptr::read(value as *const V)


Won't this lead to a double-drop of V? Once for the internal guard.defer_destroy(val), and again whenever the user is done with this read copy of the value.

You're right, I added this when playing around with giving the user a V instead of a &V, but we probably should just always give out &Vs and let them clone when they need to...

I'd be nice to have the same signature as std::HashMap, but since we're not implementing a trait I guess we can just have the user deal with it...

This should be fixed now. I'll write some tests to make sure there actually aren't any double-frees tomorrow.

cuviper · 2020-01-29T17:52:20Z

src/iter/iter.rs

+
+        let key = node.key.clone();
+        let value = node.value.load(Ordering::SeqCst, self.guard);
+        let value = unsafe { std::ptr::read(value.as_ref().unwrap() as *const V) };


Also seems like a double-drop of the value, between the returned value and the whole map dropping.

Same as above

soruh · 2020-01-29T18:37:54Z

IntoIter should have exclusive ownership of the map, no outstanding references

Are we sure that's true? I think, since we load our values under the guard, the user can still have references to them even if they don't have access to the map anymore. I could be wrong about that though.

Thank you for taking a look at this

cuviper · 2020-01-29T18:49:24Z

IntoIter should have exclusive ownership of the map, no outstanding references

Are we sure that's true? I think, since we load our values under the guard, the user can still have references to them even if they don't have access to the map anymore.

AFAIK all user references have double-borrowed lifetimes, from both &'g self and &'g guard, which means they can't outlive their reference to the map. IntoIterator consumes self by value, so it should be clear of all borrows at that point. You'd still need to make sure your IntoIter doesn't keep internal references too long, but I guess that's the unsafety of epoch::unprotected().

soruh · 2020-01-29T18:52:11Z

IntoIter should have exclusive ownership of the map, no outstanding references

Are we sure that's true? I think, since we load our values under the guard, the user can still have references to them even if they don't have access to the map anymore.

AFAIK all user references have double-borrowed lifetimes, from both &'g self and &'g guard, which means they can't outlive their reference to the map. IntoIterator consumes self by value, so it should be clear of all borrows at that point. You'd still need to make sure your IntoIter doesn't keep internal references too long, but I guess that's the unsafety of epoch::unprotected().

Nice, we're good then, I wasn't sure if all Vs were necessarily also bound to the map.

cuviper · 2020-01-29T18:52:59Z

FWIW, there's similar reasoning about exclusive access in Drop for HashMap:

flurry/src/map.rs

Lines 1389 to 1396 in 9544337

    
           // safety: we have &mut self _and_ all references we have returned are bound to the 
        
           // lifetime of their borrow of self, so there cannot be any outstanding references to 
        
           // anything in the map. 
        
           // 
        
           // NOTE: we _could_ relax the bounds in all the methods that return `&'g ...` to not also 
        
           // bound `&self` by `'g`, but if we did that, we would need to use a regular `epoch::Guard` 
        
           // here rather than an unprotected one. 
        
           let guard = unsafe { crossbeam_epoch::unprotected() };

drain now returns references reworked `IntoIter`

…ry into implement_drain_and_into_iter

jonhoo · 2020-01-29T21:50:58Z

I added that comment in Drop after having basically the same realization as is going on here in a9c6890

jonhoo · 2020-02-17T22:30:45Z

I'm going to close this for the time being, and then we can re-open if you find time to pick it up again @soruh :)

soruh · 2020-02-17T22:37:54Z

@jonhoo Great, thank you. I'll probably still be quite busy for a while but I should be able to contribute to discussions regarding this.

soruh and others added 2 commits January 26, 2020 00:14

implemented really basic drain and into_iter

1d7538e

Merge branch 'master' into implement_drain_and_into_iter

fe06497

soruh added 3 commits January 26, 2020 00:36

added concurent_drain test

11d8b37

Merge branch 'implement_drain_and_into_iter' of github.com:soruh/flur…

3f387e0

…ry into implement_drain_and_into_iter

assert that both threads get entries in concurent_drain

93e1218

soruh mentioned this pull request Jan 25, 2020

Implement "standard" map/collection methods #10

Closed

1 task

soruh added 3 commits January 26, 2020 00:46

make sure both threads in concurent_drain start at the same time

e7d944b

reordered barrier_clone.wait(); in concurent_drain,

6e25ada

so that both threads start more syncronously

make concurent_drain use more entries

f36c392

cuviper reviewed Jan 29, 2020

View reviewed changes

soruh and others added 5 commits January 29, 2020 22:25

removed two double frees

f5b2487

drain now returns references reworked `IntoIter`

Merge branch 'master' into implement_drain_and_into_iter

ca19111

simplified the new IntoIter implementation

91ed5d1

Merge branch 'implement_drain_and_into_iter' of github.com:soruh/flur…

c131578

…ry into implement_drain_and_into_iter

added missing lifetime

a14b2f7

jonhoo added this to the 1.0 milestone Jan 31, 2020

jonhoo mentioned this pull request Feb 17, 2020

Implement Drain and IntoIterator #62

Open

jonhoo closed this Feb 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

basic `drain` and `into_iter` implementations #33

basic `drain` and `into_iter` implementations #33

soruh commented Jan 25, 2020 •

edited

Loading

codecov bot commented Jan 25, 2020 •

edited

Loading

jonhoo commented Jan 26, 2020

soruh commented Jan 26, 2020

cuviper left a comment

cuviper Jan 29, 2020

soruh Jan 29, 2020

soruh Jan 29, 2020

soruh Jan 29, 2020

cuviper Jan 29, 2020

soruh Jan 29, 2020

soruh commented Jan 29, 2020 •

edited

Loading

cuviper commented Jan 29, 2020

soruh commented Jan 29, 2020

cuviper commented Jan 29, 2020

jonhoo commented Jan 29, 2020

jonhoo commented Feb 17, 2020

soruh commented Feb 17, 2020

basic drain and into_iter implementations #33

basic drain and into_iter implementations #33

Conversation

soruh commented Jan 25, 2020 • edited Loading

codecov bot commented Jan 25, 2020 • edited Loading

Codecov Report

jonhoo commented Jan 26, 2020

soruh commented Jan 26, 2020

cuviper left a comment

Choose a reason for hiding this comment

cuviper Jan 29, 2020

Choose a reason for hiding this comment

soruh Jan 29, 2020

Choose a reason for hiding this comment

soruh Jan 29, 2020

Choose a reason for hiding this comment

soruh Jan 29, 2020

Choose a reason for hiding this comment

cuviper Jan 29, 2020

Choose a reason for hiding this comment

soruh Jan 29, 2020

Choose a reason for hiding this comment

soruh commented Jan 29, 2020 • edited Loading

cuviper commented Jan 29, 2020

soruh commented Jan 29, 2020

cuviper commented Jan 29, 2020

jonhoo commented Jan 29, 2020

jonhoo commented Feb 17, 2020

soruh commented Feb 17, 2020

basic `drain` and `into_iter` implementations #33

basic `drain` and `into_iter` implementations #33

soruh commented Jan 25, 2020 •

edited

Loading

codecov bot commented Jan 25, 2020 •

edited

Loading

soruh commented Jan 29, 2020 •

edited

Loading