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

[russgray]

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]

Thanks, @russgray. The fix LGTM. Stylewise, I'd prefer to see this simplified to avoid multiple return statements for the same thing, e.g. instead of:

TValue resultingValue;
if (TryGetValue(key, out resultingValue))
{
    return resultingValue;
}
TryAddInternal(key, value, false, true, out resultingValue);
return resultingValue;

doing:

TValue resultingValue;
if (!TryGetValue(key, out resultingValue))
{
    TryAddInternal(key, value, false, true, out resultingValue);
}
return resultingValue;

I realize you were just copying the style that was already there for the other overload. Want to change both?

[stephentoub]

Actually, scratch that. This should be on the CoreFX repository, not in CoreCLR (I didn't notice which repository we were talking about). Can you close this PR and reopen an appropriate one over on CoreFX for the ConcurrentDictionary there rather than the ConcurrentDictionary in mscorlib?

(Note that the ConcurrentDictionary in CoreFX has evolved from the one here, so you'll see some slightly different code involving TryGetValueInternal in order to avoid duplicate hashcode generation.)

[russgray]

No problem, I'll duplicate the change with the style tweak on the other repo

[stephentoub]

Thanks.