-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[API Proposal]: 'ConcurrentDictionary<TKey, TValue>.AlternateLookup<TAlternateKey>.GetOrAdd' overloads #112974
Comments
Tagging subscribers to this area: @dotnet/area-system-collections |
GetOrAdd still does two lookups; the only cost it saves over doing a get followed by an add yourself is one GetHashCode.
Have you measured the actual overhead here? I'm surprised it registers. |
This manual version can do up to 3 though, if you also happened to lose the race when trying to add the key after the initial check. So it's still mean you'd go down to just 2 in the worst case, which is a small improvement? To clarify, I'm not trying to make an argument for |
You are, though... this proposal is an argument for GetOrAdd being added. :)
You have high amounts of concurrency on your startup path?
They were added for convenience. I'm not concerned about squaring the circle here for alternate lookup; it doesn't need complete parity with every single method. Note, too, that the GetOrAdd overloads have their own overheads, namely a delegate invocation, plus a closure if one is captured. |
You are of course right, I just meant to say that to me this would be different if I had been proposing
I'd use this in CsWinRT, so I can't really speak for the amount of concurrency in consuming applications. I'd be inclined to say I wouldn't expect too many collisions in this specific scenario, but I was just trying to say that getting the maximum theoretical number of lookups from 3 down to 2 still seemed like general goodness. But yeah the main reason would be convenience.
Why? It feels a bit weird that if you decide to use the alternate lookup, you fall of a little cliff there and have to go back to manually handling multiple attempts for lookups, and need to trade the convenience away to save the key allocation. To me it'd seem reasonable to say, you can use the lookup if you have a span, but that's orthogonal to the convenience lookup methods you have.
I'm not too concerned about the delegate invocation in this specific case, since that'd most likely end up just being noise compared to the actual type map lookup, since Jan mentioned that'd probably involve some UTF8 transcoding as well. As for the closure, of course, we'd want to only ever use this without creating one. I mean not unlike with the existing |
It's little different from switching to another dictionary (like |
That's fair. Also talked with Eirik offline, will just close this for now then. Thank you! 🙂 |
Background and motivation
The alternate lookup for
ConcurrentDictionary<TKey, TValue>
is missing theGetOrAdd
overloads, which makes it necessary to do a wasteful secondary lookup to try to get a value and add it if not present. We need to do this in several startup paths in CsWinRT, meaning that without aGetOrAdd
we'd end up hitting the second lookup for basically every single marshalled type requiring dynamic type checks at startup:The code is also very verbose and clunky, on top of being inefficient.
API Proposal
API Usage
The above snippet can then become just this:
Way simpler, and with no repeated lookups and unnecessary overhead.
Risks
None, these APIs already exist and are well established on
ConcurrentDictionary<TKey, TValue>
itself.The text was updated successfully, but these errors were encountered: