Implements get_or_insert for write once workloads. #293
Conversation
This change allows retrieving values from the map without having to acquire a write lock, unless the value is missing. For read once (or mostly read) workloads this is superior to taking a write lock everytime the entry is read. It also saves having to allocate value if it is not inserted and having to compute the usize hash / shard index twice versus the approach of calling get then using entry to insert (as there is not try_insert yet) Unfortunately, it doesn't seem like it is possible to avoid the second read without changing the behavior of HashMap.
|
I didn't touch dependencies so not sure why home package all of sudden is requiring a newer rustc. |
| let c: K = ptr::read(&key); | ||
| let mut shard = self._yield_write_shard(idx); | ||
|
|
||
| shard.insert(key, SharedValue::new(value(&c))); |
There was a problem hiding this comment.
i think there could be a race here? when multiple threads are racing with get_or_insert() for the same missing key, all threads could reach here, resulting replacing writes from other threads? That's because it's not atomic to drop the read shard above and yield write shard here?
There was a problem hiding this comment.
This isn't any better than calling .get() and insert if it isn't there. So this doesn't add anything.
|
@xacrimon I wonder this pr could ever be merged soonish? if more efforts are needed, i can work on. cc: @geekbeast |
@xacrimon (thanks for recent release with perf increase!) friendly reminder. if this api sounds okay, I'm available to work on this pr. cc: @geekbeast |
|
@xacrimon a friendly ping. :) |
|
@xacrimon a friendly ping. sorry for being persistent here. my work is kind of blocked on this. can i jump ahead and recreate a pr? |
| let c: K = ptr::read(&key); | ||
| let mut shard = self._yield_write_shard(idx); | ||
|
|
||
| shard.insert(key, SharedValue::new(value(&c))); |
There was a problem hiding this comment.
This isn't any better than calling .get() and insert if it isn't there. So this doesn't add anything.
| drop(shard); | ||
|
|
||
| unsafe { | ||
| let c: K = ptr::read(&key); |
There was a problem hiding this comment.
Here, K is duplicated illegally, can't do this. Violated standard rust rules and this also means K is dropped twice. If you use allocating string keys this segfaults.
| Ref::new(shard, kptr, vptr) | ||
| } | ||
| } else { | ||
| drop(shard); |
There was a problem hiding this comment.
At this point, someone else can insert key, which causes us to loose updates.
hihi, sorry ive been away and very busy. yes, i'd take prs to implement this API if they're implemented in an atomic way that makes it better to use than get + insert in sequence. thank you <3 |
if you mean the illegal and very bug-causing ptr::read call that's not really an option to just ignore. |
This change allows retrieving values from the map without having to acquire a write lock, unless the value is missing. For read once (or mostly read) workloads this is superior to taking a write lock everytime the entry is read. It also saves having to allocate value if it is not inserted and having to compute the usize hash / 8000 shard index twice versus the approach of calling get then using entry to insert (as there is not try_insert yet)
Unfortunately, it doesn't seem like it is possible to avoid the second read without changing the behavior of HashMap.