views:

104

answers:

4

I have a function thats the main bottleneck of my application, because its doing heavy string comparisions against a global list shared among the threads. My question is basicly this:

Is it bad practive to lock the list ( called List gList ) multiple times in 1 function. For then to lock it again later ( Basicly locking when doing the lookup, unlocking getting a new item ready for insertion then locking it again and adding the new item).

When i you a profiler i dont see any indication that im paying a heavy price for this, but could i be at a later point or when the code it out in the wild? Anyone got any best practive or personal experence in this?

+1  A: 

In general, you want to lock for as short a time as possible. The cost of a contention is much much higher (must go to kernel) than the cost of a contention-free lock acquisition (can be done in userspace), so finer-grained locking will usually be good for performance even if it means acquiring the lock more times.

That said, make sure you profile in an appropriate situation for this: one with a high amount of simultaneous load. Otherwise your results will have little relationship to reality.

Jacob B
A: 

In my opinion there are to few data to give a concrete answer. Generally not the number of locks creates a performance issue, but the number of threads that are waiting for that lock.

Jenea
+5  A: 

How do you perform the locking? You may want to look into using ReaderWriterLockSlim, if that is not already the case.

Here is a simple usage example:

class SomeData
{
    private IList<string> _someStrings = new List<string>();
    private ReaderWriterLockSlim _lock = new ReaderWriterLockSlim();

    public void Add(string text)
    {
        _lock.EnterWriteLock();            
        try
        {
            _someStrings.Add(text);
        }
        finally
        {
            _lock.ExitWriteLock();
        }

    }

    public bool Contains(string text)
    {
        _lock.EnterReadLock();
        try
        {
            return _someStrings.Contains(text);
        }
        finally
        {
            _lock.ExitReadLock();
        }
    }
}
Fredrik Mörk
Definitely only lock it for read access unless you are actually changing the list
John JJ Curtis
+1  A: 

It sounds like you don't want to be releasing the lock between the lookup and the insertion. Either that, or you don't need to lock during the lookup at all.

Are you trying to add to the list only if the element is not already there? If so, then releasing the lock between the two steps allows another thread to add to the list while you are preparing your element. By the time you are ready to add, your lookup is out of date.

If it is not a problem that the lookup might be out of date, then you probably don't need to lock during the lookup at all.

UncleO