RSDK-2011 Change CHashMap to DashMap for better performance #31
Conversation
Cargo.toml
Outdated
| @@ -45,6 +44,7 @@ tracing = {version = "0.1.34"} | |||
| tracing-subscriber = {version = "0.3.11", features = ["env-filter"]} | |||
| webpki-roots = "0.21.1" | |||
| webrtc = "0.6.0" | |||
| dashmap = "5.4.0" | |||
There was a problem hiding this comment.
(nit) would love to keep dependencies alphabetized
| match self.receiver_bodies.remove(&stream_id) { | ||
| Some(entry) => Ok(entry.1), | ||
| None => Err(anyhow::anyhow!("Tried to receive stream {stream_id} but it didn't exist!")), | ||
| } |
There was a problem hiding this comment.
[minor] why can't we (or shouldn't we) keep using the Option#ok_or method with this new data structure?
There was a problem hiding this comment.
the remove api is slightly different with DashMap, it doesn't give us exactly what we want (the stored value) but rather a key value tuple. the match is a bit cleaner for indexing into the tuple (note the .1 on line 168).
edit: I guess "cleaner" is just opinion, and we certainly could use the ok_or and add a .1 at the end, but it seems a little ugly personally!
There was a problem hiding this comment.
CHashMap::remove returned an Option<V> while DashMap::remove returns an Option<(K,V)>. We don't really care about the K (a u64 ID), and only want to return the V (a stream). Extracting the stream requires accessing it from the tuple with .1; I wasn't able to do that with ok_or, but definitely possible I just didn't know how.
RSDK-2011
DashMaphas an almost identical API toCHashMapbut anecdotally has better performance under high workloads (e.g. lots of concurrent writes). Switching toDashMapseems to resolve hangs with async calls from the Python SDK (see RSDK-2011 and RSDK-2070).Note that users will still run into the issue of stream ID integer overflow in the underlying
webrtc-rscrate until we do RSDK-2268.