retain method

[danielSanchezQ]

Implemented retain for HashMap
Implemented retain tests for empty map, retain all elements, retain none elements and retain arbitrary elements

[danielSanchezQ]

I used an approach similar to the clone method. I just take the keys that do not pass the filtering function and remove each of them one by one.
How better/worst would it be to rebuild the map using the from_iterator method? (it was my other option, but I went to most basic one).
I think it depends of the worst case and the amount of re-allocations of the new map.

[jonhoo]

I think we probably do not want to construct a new map (with from_iterator). That would likely be slower, unless the reduction method was particularly selective, which we don't know in advance. If it was, then the user could easily enough implement that themselves, so the approach you've chosen seems good.

I would probably avoid the intermediate Vec though. Since we have concurrency, it should be fine to just do

for (k, v) in self.iter() {
    if !f(k, v) {
        self.remove(k);
    }
}

It looks like hashbrown does something special about Drop in their implementation, but I don't think that affects us since we do not immediately call drop regardless. The Java versions (retainAll and removeValueIf) also appear to take the same strategy, so I think that's reason enough to stick with it.

One thing to note is that the Java code does this:

flurry/jsr166/src/ConcurrentHashMap.java

Line 1630 in 0c3bfb6

if (function.test(v) && replaceNode(k, null, v) != null)

That is, it uses the cv argument of replaceNode to only remove the item if the value hasn't changed. We should probably do the same (and document that behavior on retain). It should be a matter of:

1. renaming remove to replace_node
2. adding the v and cv arguments with type Option<V> and probably better name
3. adding a new remove that just calls replace_node(k, None, None, guard)
4. adding this condition so that the right thing happens if cv.is_some()
5. for completeness, adding this condition and making sure we don't unlink the node if v.is_some()

[danielSanchezQ]

@jonhoo I'm having issues implementing it that way:

let n = unsafe { e.deref() }.as_node().unwrap();
let next = n.next.load(Ordering::SeqCst, guard);
if n.hash == h && n.key.borrow() == key {
    let ev = n.value.load(Ordering::SeqCst, guard);
    if new_value.is_none() || old_value.unwrap() == unsafe {ev.deref()} {
        old_val = Some(ev);
        // remove the BinEntry containing the removed key value pair from the bucket
        if new_value.is_some() {
            n.value.store(Owned::new(new_value.unwrap()), Ordering::SeqCst);
        } else if !pred.is_null() {
            // either by changing the pointer of the previous BinEntry, if present
            // safety: as above
            unsafe { pred.deref() }
                .as_node()
                .unwrap()
                .next
                .store(next, Ordering::SeqCst);
        } else {
            // or by setting the next node as the first BinEntry if there is no previous entry
            t.store_bin(i, next);
        }

        // in either case, mark the BinEntry as garbage, since it was just removed
        // safety: as for val below / in put
        unsafe { guard.defer_destroy(e) };

        // since the key was found and only one node exists per key, we can break here
        break;
    }
}

Mostly here if new_value.is_none() || old_value.unwrap() == unsafe {ev.deref()}, related to this condition (maybe the other condition too). I'm not sure how to check if the old_value and the value in the node are the same (I reach the point on where it asks for V to implement PartialEq, which makes sense). But I feel I may be missing something important

[jonhoo]

Ah, the trick there is to not take &V as old_value, but take Shared<'g, V> instead. You want to compare the pointer to the old value to the pointer to the current value (i.e., no deref). That would also mean you do not need V: PartialEq.

Also, keep in mind that if new_value.is_some() (you probably want if let Some(nv) = new_value btw), then you do not want to unlink the node, and you also do not want to free the BinEntry.

[danielSanchezQ]

I added retain_force, similar to retain but do not check on values and just enforces entry removal.

[jonhoo]

Excellent, thank you!

[danielSanchezQ]

Thank you for your patience!!

[jonhoo]

Now let's see if this change passes the new miri and sanitizer CI tests :o
#37