Mappable RwLock guards

[kvark]

This is a logical continuation of #10. It looks like the MappableMutexGuard is implemented, but nothing is done for RwLock. We have a strong case for it: a map with data that needs to be either read or created and then read. Ideally, it would have the following implementation (roughly):

fn get_data(&self, key: Key) -> ReadGuard<Data> {
  // lock the map, but only deal with a particular entry
  let guard = self.lock.upgradable_read().map(|map| map.entry(key));
  // fast path for keys already filled up
  if let Entry::Occupied(e) = *guard {
    return *e;
  }
  // slow path to add a new value
  let data = create_heavy_state(key);
  let gw = guard.upgrade();
  gw.or_insert(data);
  // and return it
  gw.downgrade().or_insert_with(|| ureachable!())
}

[Amanieu]

If you look at the docs, map is implemented for RwLockReadGuard and RwLockWriteGuard.

However it is not implemented for RwLockUpgradableReadGuard because there are soundness issues with upgrading such a map. This is because the closure passed to map can return any arbitrary &T, and it is unsound to assume that this can safely be upgraded to &mut T for all possible reference types.

[Amanieu]

Looking at your example, you might want to defer calling map until after you have downgraded the lock to a normal RwLockReadGuard.

[kvark]

@Amanieu indeed the non-upgradable ones have map, I missed this initially. I've spent quite a bit of time trying to implement anything close to the pseudo-code written here, and I wasn't successful. I figured we need to implement our own map storage type for this.

[Amanieu]

I'm not exactly sure what you are using this for, but generally speaking in most cases you will be returning an existing entry rather than inserting a new one.

You could try explicitly optimizing for this case by first attempting to just read from the map with a normal read lock. If that fails then you can release the read lock and grab a write lock, with which you attempt a lookup again and only insert if that lookup fails as well.

This is slightly less efficient in the case where an insertion is needed, but if that is relatively rare then the overall performance should improve.

[kvark]

@Amanieu I think you understand my use case. Getting a read lock followed by a possible write lock has the following downsides for me:

1. need to re-acquire the lock, although I think this is the least of concerns
2. accessing the map by key twice, even though this only happens when we need to add an entry, so the cost is amortized and we don't optimize for this case anyway
3. impossible to abstract away this in a nice self-contained API, e.g.

fn get_or_create_with<F: FnOnce() -> V>(lock: RwLock<Self>, key: Key, create_fn: F) -> &V {...}

In other words, I'd like this function to borrow the lock immutably and return something that can be de-referenced into the value. If this function tries to mess with locking internally in any way, there is no ability to return such a value wrapper any more.

[Amanieu]

@kvark Here's a quick sketch of how this would look like:

fn get_or_create_with<F: FnOnce() -> V>(lock: &RwLock<Self>, key: Key, create_fn: F) -> MappedRwLockReadGuard<V> {
    {
        let guard = RwLockReadGuard::map(lock.read(), |map| map.get(key));
        if guard.is_some() {
            return RwLockReadGuard::map(guard, |opt| opt.unwrap());
        }
    }

    RwLockWriteGuard::map(lock.write(), |map| map.entry(key).or_insert_with(create_fn)).downgrade()
}

[kvark]

@Amanieu
This code is very interesting, thank you! I'm not yet able to make it work though.
In particular, a problem I'm seeing here is that functions passed to ::map calls require to return references. This is not the case for either HashMap::get (which returns Option<&V>, which is not a reference) or HashMap::entry (which is Entry). Thus, the lock mapping API appears to be fundamentally incompatible with maps, among other things.

[kvark]

Also cc-ing @gankro on the last comment here ^

[Amanieu]

@kvark Oh, good point, I missed that. It seems that there is no way to avoid a double lookup with the current API then:

let guard = lock.read();
if guard.contains_key(key) {
    return RwLockReadGuard::map(guard, |map| map.get(key).unwrap());
}

I think we could add a try_map API for this use case, if there is sufficient motivation.

[kvark]

Try_map would be great to have indeed.

…

On Jul 17, 2018, at 18:19, Amanieu ***@***.***> wrote: @kvark Oh, good point, I missed that. It seems that there is no way to avoid a double lookup with the current API then: let guard = lock.read(); if guard.contains_key(key) { return RwLockReadGuard::map(guard, |map| map.get(key).unwrap()); } I think we could add a try_map API for this use case, if there is sufficient motivation. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[kvark]

@Amanieu heads up, we happen to need try_map now for webgpu-remote component of https://github.com/gfx-rs/wgpu

[Amanieu]

It it now available in lock_api 0.1.4, which parking_lot depends on.