Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implements get_or_insert for write once workloads. #293

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

geekbeast
Copy link

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.

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.
@geekbeast
Copy link
Author

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)));
Copy link

@ryoqun ryoqun Apr 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@ryoqun
Copy link

ryoqun commented May 8, 2024

@xacrimon I wonder this pr could ever be merged soonish? if more efforts are needed, i can work on. cc: @geekbeast

@ryoqun
Copy link

ryoqun commented Jun 20, 2024

@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

@ryoqun
Copy link

ryoqun commented Aug 2, 2024

@xacrimon a friendly ping. :)

@ryoqun
Copy link

ryoqun commented Aug 30, 2024

@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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants