Value types and Dictionary retrieval.

The class below raises an event for every new “dataKey” registered and raises an event when a “dataKey” is deregistered and count for that “dataKey is zero.

This class aims to be threadsafe and I am trying to make this as performant as possible.

My question is; in the Deregister method can I somehow remove the 2nd lookup when I update the value (the _data[dataKey] = currentCountValue;)?

I can’t simple update the currentCountValue variable as the value is updated on the local stack only and not in the Dictionary.

Or can you sugguested any performance improvements? I don’t think I can remove the lock and use CAS operations (Interlocked methods) to update the count as the dictionary is not threadsafe for updates when used like this … right?

/I am using c# 3.0.

Thanks for your time.

public sealed class DataCounter
{
    public event EventHandler NewKeyEvent;
    public event EventHandler ZeroCountEvent;
    private readonly Dictionary<string, int> _data = new Dictionary<string, int>();

    public void Register(string dataKey)
    {
        lock (_data)
        {
            if (_data.ContainsKey(dataKey))
            {
                _data[dataKey]++;
            }
            else
            {
                _data.Add(dataKey, 1);
                if (NewKeyEvent != null) NewKeyEvent(this, null);
            }
        }
    }

    public void Deregister(string dataKey)
    {
        lock (_data)
        {
            int currentCountValue;
            if (_data.TryGetValue(dataKey, out currentCountValue))
            {
                if (currentCountValue > 0)
                {
                    currentCountValue--;
                    _data[dataKey] = currentCountValue;
                }

                if (currentCountValue == 0)
                {
                    if (ZeroCountEvent != null) ZeroCountEvent(this, null);
                }
            }
        }
    }
}

From stackoverflow

• As a thought - if you don't want to have to do the "set" via an indexer, you could move the counter out onto a class?

  class CounterBox {
      public int Count {get;set;}
  }
  

  Then have a Dictionary<string,CounterBox>. You can now update Count outside of the dictionary, and only call Remove(dataKey) when .Count is zero. This will have an extra de-reference, but you won't have to assign via the indexer.

  As for which is faster: you'd need to profile.

  Something like:

  public sealed class DataCounter
  {
      private class CounterBox
      {
          public int Count { get; set; }
      }
      public event EventHandler NewKeyEvent;
      public event EventHandler ZeroCountEvent;
      private readonly Dictionary<string, CounterBox> _data
          = new Dictionary<string, CounterBox>();
  
      public void Register(string dataKey)
      {
          lock (_data)
          {
              CounterBox box;
              if (_data.TryGetValue(dataKey, out box))
              {
                  box.Count++;
              }
              else
              {
                  _data.Add(dataKey, new CounterBox { Count = 1 });
                  EventHandler handler = NewKeyEvent;
                  if (handler != null) handler(this, EventArgs.Empty);
              }
          }
      }
  
      public void Deregister(string dataKey)
      {
          lock (_data)
          {
              CounterBox box;
              if (_data.TryGetValue(dataKey, out box))
              {
                  if (box.Count > 0)
                  {
                      box.Count--;
                  }
  
                  if (box.Count == 0)
                  {
                      EventHandler handler = ZeroCountEvent;
                      if (handler != null) handler(this, EventArgs.Empty);
                      _data.Remove(dataKey);
                  }
              }
          }
      }
  }
  

• Your event handling is not thread-safe.

  // Execute this ...
  if (NewKeyEvent != null)
  
  // ... other threads remove all event handlers here ...
  
  // ... NullReferenceException here.
      NewKeyEvent(this, null);
  

  So better do it as follows.

  EventHandler newKeyEvent = this.newKeyEvent;
  
   if (newKeyEvent != null)
   {
       newKeyEvent(this, null);
   }
  

• You should be careful with the way you're raising the event (and someone already mentioned your registering is not thread safe).

  You're calling the event handlers inside the lock. That's not thread unsafe per se, but you risk stalling your datastructure entirely. Since you obviously can't control what's happening in the event handlers you're calling, if the event handler itself blocks for a long time, or freezes, your dictionary is locked out until the handler returns.

  In a lock, you SHOULD NEVER call methods you don't have control over and never call any method whose execution time is non determinist (anything that does not access memory in some way or other). If you do, you're vulnerable to have your lock blocking indefinitely, even if your code is thread safe.

  So in the referencing and dereferencing, you should either have a copy of your invocation list and call it OUTSIDE of the lock, or call the delegate itself outside (using the pattern Daniel mentioned).