CODEWITHSUNDEEP
c#multithreadinglocking.net-4.5
Royi Namir
Royi Namir

Reputation: 148524

Dictionary of lock object in order to reduce latency in C#?

When an online user send a message to a offline user , I keep those messages in a ConcurrentDictionary . each user is running/spinning in its own Task(thread).

 public static ConcurrentDictionary<int, List<MSG>> DIC_PROFILEID__MSGS  = new ConcurrentDictionary...

So the method look like : 

/*1*/   public static void SaveLaterMessages(MSG msg)
/*2*/   {
/*3*/       var dic = Globals.DIC_PROFILEID__MSGS;
/*4*/   
/*5*/   
/*6*/       lock (saveLaterMssagesLocker)
/*7*/       {
/*8*/           List<MSG> existingLst;
/*9*/           if (!dic.TryGetValue(msg.To, out existingLst))
/*10*/           {                                                  
/*11*/               existingLst = new List<MSG>();
/*12*/               dic.TryAdd(msg.To, existingLst);
/*13*/           }
/*14*/           existingLst.Add(msg);
/*15*/       }
/*16*/   }

Please notice the lock at #6. I did this because if 2 threads are in #10 , they will both cause creation of a new List (which is bad).

But I'm bothered by the fact that I lock "too much".

In other words , If I send message to offline-user-20 , there is no reason that one will not be able to send message to offline-user-22.

So I'm thinking about creating additional dictionary of locks : 

Dictionary <int , object> DicLocks = new Dictionary <int , object>();

Where the int key is the userID

Later , initialize each entry with new Object()

So now my method will look like : 

public static void SaveLaterMessages(MSG msg)
{
    var dic = Globals.DIC_PROFILEID__MSGS;

    lock (Globals.DicLocks[msg.To]) //changed here !!!
    {
        List<MSG> existingLst;
        if (!dic.TryGetValue(msg.To, out existingLst))
        {                                                  
            existingLst = new List<MSG>();
            dic.TryAdd(msg.To, existingLst);
        }
        existingLst.Add(msg);
    }
}

Now , users can inset messages to a different offline-users without interfering.

Question

1) Am I right with this approach , or is there any better approach ?

2) I really hate to lock around the ConcurrentDictionary, it is 100% not right. should I make it a regular dictionary ?

Upvotes: 2

Views: 308

Answers (2)

Scott Chamberlain
Scott Chamberlain

Reputation: 127563

ConcurrentDictonary has tools to help you do the situation you are in, if you switch your retrieval/creation of your existing list to a single thread safe operation it makes the problem much simpler.

public static void SaveLaterMessages(MSG msg)
{
    var dic = Globals.DIC_PROFILEID__MSGS;

    List<MSG> existingLst = dic.GetOrAdd(msg.To, (key) => new List<MSG>());

    lock(((ICollection)existingLst).SyncRoot)
    {
        existingLst.Add(msg);
    }
}

This attempts to get the list from the dictionary and creates a new list if it did not exist, it then locks only on the non thread safe operation of adding to the list on the list object itself.

If possible a even better option is replace your List<MSG> with a thread safe collection like ConcurrentQueue<MSG> and you won't need to perform any locks at all (the ability to do this all depends on how messages are used once they are in the list). If you do need to use a List you don't need Globals.DicLocks[msg.To] to lock on, it is perfectly acceptable to lock on the list object that is returned from the collection.

One advantage you could get from a second lock object is if you are going to have lots of reads but very few writes you could use a ReaderWriterLockSlim to allow multiple concurrent readers but only one writer.

public static void SaveLaterMessages(MSG msg)
{
    var dic = Globals.DIC_PROFILEID__MSGS;

    List<MSG> existingLst = dic.GetOrAdd(msg.To, (key) => new List<MSG>());

    var lockingObj = GetLockingObject(existingLst);
    lockingObj.EnterWriteLock();
    try
    {
        existingLst.Add(msg);
    }
    finally
    {
        lockingObj.ExitWriteLock();
    }
}

private static ConcurrentDictionary<List<MSG>, ReaderWriterLockSlim> _msgLocks = new ConcurrentDictionary<List<MSG>, ReaderWriterLockSlim>();

public static ReaderWriterLockSlim GetLockingObject(List<MSG> msgList)
{
    _msgLocks.GetOrAdd(msgList, (key) => new ReaderWriterLockSlim());
}


//Elsewhere in multiple threads.

public MSG PeekNewestMessage(int myId)
{
    var dic = Globals.DIC_PROFILEID__MSGS;
    var list = dic[myId];
    var lockingObj = GetLockingObject(list);

    lockingObj.EnterReadLock();
    try
    {
        return list.FirstOrDefault();
    }
    finally
    {
        lockingObj.ExitReadLock();
    }
}

However I would still recommend the ConcurrentQueue<MSG> approach over this approach.

Upvotes: 5

Murray Foxcroft
Murray Foxcroft

Reputation: 13745

You could also wrap your "List()" code in a class with a singleton pattern that manages the lifetime of the list per user and adding / removing from it, then your concurrent dictionary and support code becomes lock free. Your "List()" could also be a concurrent queue and further reduce the work you need to do around locking for future adds etc.

http://msdn.microsoft.com/en-us/library/dd267265(v=vs.110).aspx

For high volumes, you could also use a service bus type pattern to cross thread boundaries and create a queue for each user and push messages down it for consumption when the user comes back online.

Upvotes: 0

Related Questions