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

Call TryGetValue in overload of ConcurrentDictionary.GetOrAdd to avoid acquiring lock #631

Merged
merged 1 commit into from Feb 5, 2015

Conversation

Projects
None yet
4 participants
@russgray
Copy link

russgray commented Feb 4, 2015

There are two overloads of ConcurrentDictionary.GetOrAdd. One accepts a factory
method to generate the value if the key is not found, and calls TryGetValue for
the existence check which is a lock-free operation.

The other overload, however, does not call TryGetValue - it simply calls
straight through to TryAddInternal, which will obtain a bucket lock before it
does the existence check
. This violates the documented claim of
ConcurrentDictionary of lock-free reads. This commit simply adds the same
TryGetValue check, making the two overloads consistent.

Russell Gray
Call TryGetValue in overload of ConcurrentDictionary.GetOrAdd to avoi…
…d acquiring lock

There are two overloads of ConcurrentDictionary.GetOrAdd. One accepts a factory
method to generate the value if the key is not found, and calls TryGetValue for
the existence check which is a lock-free operation.

The other overload, however, does not call TryGetValue - it simply calls
straight through to TryAddInternal, which will obtain a bucket lock *before it
does the existence check*. This violates the documented claim of
ConcurrentDictionary of lock-free reads. This commit simply adds the same
TryGetValue check, making the two overloads consistent.
@stephentoub

This comment has been minimized.

Copy link
Member

stephentoub commented Feb 4, 2015

Thanks, @russgray.

I agree this change maps better to the stated behavior of ConcurrentDictionary.

I was still a little concerned about the performance impact this could have, as it would make the add case a bit more expensive (a failed lookup) in favor of making the get case less expensive (no lock required). I played around with it a bit, and it seems like worst case this negatively impacts the add case by around 8% or so, whereas this consistently improves the get case by anywhere from 30-100% improvement (and that's without contention, which would decrease the negative impact for add and increase the positive improvement for get), depending on various factors and assuming a relatively trivial GetHashCode, as for Int32 (a more expensive hash code function would just decrease the impact of the change). Since ConcurrentDictionary is typically used in scenarios with frequent gets and less frequent adds, this seems like the right tradeoff to make.

So LGTM.

@russgray

This comment has been minimized.

Copy link

russgray commented Feb 4, 2015

Agreed. Also, from personal experience I've used this in a highly-concurrent scenario and the performance under high contention was a killer - we had to switch all our calls to the value factory overload for this exact reason.

stephentoub added a commit that referenced this pull request Feb 5, 2015

Merge pull request #631 from russgray/master
Call TryGetValue in overload of ConcurrentDictionary.GetOrAdd to avoid acquiring lock

@stephentoub stephentoub merged commit bf204da into dotnet:master Feb 5, 2015

1 check passed

default Merged build finished.
Details

@karelz karelz modified the milestone: 1.0.0-rtm Dec 3, 2016

Olafski pushed a commit to Olafski/corefx that referenced this pull request Jun 15, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment