Lock on Dictionary's TryGetValue() - Performance issues

Question

I've profiled my application and ran some performance tests which led me to believe that the following if-lock-if arrangement:

private float GetValue(int id)
{
    float value;
    if (!dictionary.TryGetValue(id, out value))
    {
      lock (lockObj)
      {
        if (!dictionary.TryGetValue(id, out value))
        {
          value = ComputeValue(id);
          dictionary.Add(id, value);
        }
      }
    }
}

seems to perform faster than "lock-if" or using ReaderWriterLockSlim. But very rarely, I get the following exception:

1) Exception Information
*********************************************
Exception Type: System.NullReferenceException
Message: Object reference not set to an instance of an object.
Data: System.Collections.ListDictionaryInternal
TargetSite: Int32 FindEntry(TKey)
HelpLink: NULL
Source: mscorlib

StackTrace Information
*********************************************
  at System.Collections.Generic.Dictionary`2.FindEntry(TKey key)
  at System.Collections.Generic.Dictionary`2.TryGetValue(TKey key, TValue& value)
  at MyNamespace.GetValue()
  .....
  .....

What am I doing wrong here ?

Edit: To clarify, this method is called on average more than 50 million times, and the conflict is generally less than 5,000.

Thanks

Why not just use the indexer to set the value, i.e. `dictionary[id] = value`? That way you wouldn't need to worry about checking; if it's not there, it gets added and if it's there, the value is updated. You could still wrap the operation in a lock if you're using threading. — Michael Todd, May 04 '11 at 14:46
it doesn't matter how few times it aborts. whether it's 1 time or a million it is just a symptom of the real problem. you need to be using a 'ConcurrentDictionary' which is the newest multi-threaded data storage solution released in .NET4. — phillip, May 04 '11 at 14:52
possible duplicate of [is locking necessary for Dictionary lookup?](http://stackoverflow.com/questions/4000347/is-locking-necessary-for-dictionary-lookup) — bzlm, May 04 '11 at 14:52
http://stackoverflow.com/questions/2624301/how-to-show-that-the-double-checked-lock-pattern-with-dictionarys-trygetvalue-is — alhazen, May 04 '11 at 15:09

JaredPar · Accepted Answer · 2011-05-04T14:48:59.657

What you're trying to do here is simply not a supported scenario. The TryGetValue occurs outside of the lock which means is very possible for one thread to be writing to the dictionary while others are simultaneously calling TryGetValue. The only threading scenario inherently supported by Dictionary<TKey, TValue> is reads from multiple threads. Once you start reading and writing from multiple threads all bets are off.

In order to make this safe you should do one of the following

Use a single lock for all read or write accesses to the Dictionary
Use a type like ConcurrentDictionary<TKey, TValue> which is designed for multi-threaded scenarios.

yeah he needs to be using the 'ConcurrentDictionary' for sure +1 — phillip, May 04 '11 at 14:49

score 3 · Answer 2 · answered May 04 '11 at 14:44

3

Either the usage of that collection by your code is thread-safe, in which case you don't need the lock, or it's not thread-safe, in which case you ALWAYS need the lock.

Try this using ConcurrentDictionary instead, which is thread-safe.

answered May 04 '11 at 14:44

Steve Townsend

53,498
9
91
140

score 2 · Answer 3 · answered May 04 '11 at 14:44

Dictionary isn't thread-safe. If anything's adding to the dictionary when you do a TryGetValue, things can go badly. Your first call to TryGetValue is not protected by a lock. So if thread A is doing an Add and thread B enters that first TryGetValue, it can throw an exception.

Consider using System.Collections.Concurrent.ConcurrentDictionary. Or be sure to lock the dictionary on every access. Probably using ReaderWriterLockSlim.

score 0 · Answer 4 · answered Mar 25 '19 at 11:41

0

You can wrap lockless TryGetValue with try catch and retry/fallback to lock if you get an exception. That way in common case where there is no write or you lucky and not get exception during write you can trust the value returned by TryGetValue..

answered Mar 25 '19 at 11:41

TakeMeAsAGuest

957
6
11

score 0 · Answer 5 · answered May 04 '11 at 14:46

0

I think removing the first if that's not thread-safe should solve the issue.

Is there a reason for doing the same if statement twice?

answered May 04 '11 at 14:46

Chris Snowden

4,982
1
25
34

3

This is the double-checked locking antipattern. – Steve Townsend May 04 '11 at 14:48
2

Both of the TryGetValue should be inside the lock then to prevent one thread reading while another is modifying the dictionary. – Chris Snowden May 04 '11 at 14:50
You are correct that that would then be thread-safe, but then why check it twice? The idea of the 'pattern' is that the first check is a quick one to ensure you actually need to lock and recheck. As OP has seen, this does not work with more non-threadsafe variable state predicates. Typically double-checked locking is used to check a supposedly-atomic thing like a singleton instance pointer being `NULL`. – Steve Townsend May 04 '11 at 14:53
@steve: he said in the post that if-lock-if performs better than lock-if (the sentence that has the first code block in the middle of it). Clearly that performance comes at a bad cost but that is why he is trying to run the code this way. – Chris May 04 '11 at 14:59
Agree that checking if null is probably fine as it's only comparing the address of the object and not calling a function on the object like done by the OP which is obviously not safe to do like it is. – Chris Snowden May 04 '11 at 15:00

Lock on Dictionary's TryGetValue() - Performance issues

5 Answers5