How to improve performance on two thread accesing a collection of items concurrently

Question

I'm implementing Producer/Consumer aproach on a .net project 3.5

There is only one producer and one consumer each running on its own thread

The CheckOrderToProcess method checks a table that match certain conditions and adds them to a list (Producer)

The bgOrdenes_DoWork method takes each item on the list and performs some logic (Consumer)

I want to avoid locking the whole list lstOrderToProcessto in order to improve performance, I tried to use ConcurrentQueue but I can not upgrade to .net 4.0 due to limitations that this is going to be used in projects that cannot be upgraded to 4.0

How can I change this implementation to improve perfomance ? I does not have to be a list as long as it can be shared between threads and I can add at the end and take the first element (Queue)

public class DatabaseCache : ICacheDB
{
    private static List<NotificacionOrdenes> lstOrderToProcess;
    private static Object lockOrders;
    private static BackgroundWorker bgOrdenes;
    private static bool pendingOrders = false;
    private static Timer timerCheck;

    public DatabaseCache(string eventSourceName, bool monitorearOrderNotifications)
    {
        bgOrdenes = new BackgroundWorker();
        bgOrdenes.DoWork += new DoWorkEventHandler(bgOrdenes_DoWork);

        lstOrderToProcess = new List<NotificacionOrdenes>();
        lockOrders = new Object();

        CheckOrderToProcess();

        if (!bgOrdenes.IsBusy)
        {
            bgOrdenes.RunWorkerAsync();
        }

        //execute CheckOrderToProcess periodically
        timerCheck = new Timer(2000);
        timerCheck.Elapsed += new ElapsedEventHandler(timerCheck_Elapsed);
        timerCheck.Enabled = true;
        timerCheck.AutoReset = true;

    }
    void timerCheck_Elapsed(object sender, ElapsedEventArgs e)
    {
        CheckOrderToProcess();
    }

    private void CheckOrderToProcess()
    {
        DataSet ds;
        NotificacionOrdenes notif;

        ds = Data.GetOrderNotifications_ToProcess();

        //if there is new info to process
        if ((ds != null) && (ds.Tables[0].Rows.Count != 0))
        {

            foreach (DataRow row in ds.Tables[0].Rows)
            {
                notif = new NotificacionOrdenes();

                //fill NOTIF with info of each row

                lock (lockOrders)
                {
                    lstOrderToProcess.Add(notif);
                }

            }

            pendingOrders = true;
        }
    }

    void bgOrdenes_DoWork(object sender, DoWorkEventArgs e)
    {
        NotificacionOrdenes notif;

        while (true)
        {
            if (pendingOrders)
            {
                lock (lockOrders)
                {
                    notif = lstOrderToProcess[0];
                    lstOrderToProcess.RemoveAt(0);

                    //check to see if there is any pending order
                    pendingOrders = lstOrderToProcess.Any();
                }

                //Execute rest of the logic
            }
        }
    }
}

Answer 1

There's no conceivable reason to alter the locking in this code snippet. It does however suffer from rather nasty design flaws:

• the wait loop inside DoWork is a hot wait loop. It burns 100% core, accomplishing nothing 99.99999% of the time. That's in general very unfriendly to the machine you run this code on. It will also cause your program to be unresponsive to added items, even though you burn lots of cpu cycles on trying to detect that. The operating system's thread scheduler will keep you in the dog house for a while after you burned your quantum.

• the pendingOrders variable is used as a synchronization object but is just a simple bool variable. Lots of things go wrong when you do that. For one, there are good odds that your code never sees the variable being set to true when you run the Release build of your code. A problem in 32-bit code, such a variable must be declared volatile. It is also inefficient, it can take a while before the thread can observe the assigned value.

• the use of lstOrderToProcess.Any() is inefficient. No point in removing just the element at index 0 when you in effect will empty the entire list anyway.

• the consumer runs on a BackgroundWorker. Which uses a thread-pool thread to implement the worker. TP threads should in general not run for more than about half a second. Your thread however runs forever, severe impeding the thread-pool scheduler's job to get pending tp thread requests executed. This negatively affects the responsiveness of your entire app.

Get ahead by using a regular Thread to run the consumer. And use a better design for the list, you want a Blocking Queue. You can get one that runs on old .NET versions by using the code from the threading masters, Joe Duffy's sample implementation solves your hot wait-loop problem. I'll repost it here, tweaked to not wait for the consumer:

public class BlockingQueue<T> { 
    private Queue<Cell<T>> m_queue = new Queue<Cell<T>>(); 
    public void Enqueue(T obj) { 
        Cell<T> c = new Cell<T>(obj);
        lock (m_queue) {
            m_queue.Enqueue(c); 
            Monitor.Pulse(m_queue); 
        }
    } 
    public T Dequeue() { 
        Cell<T> c; 
        lock (m_queue) { 
            while (m_queue.Count == 0) Monitor.Wait(m_queue); 
            c = m_queue.Dequeue();
        } 
        return c.m_obj; 
    } 
}

class Cell<T> { 
    internal T m_obj; 
    internal Cell(T obj) { 
        m_obj = obj;
    } 
}

Answer 2

If I would do producer/consumer in your case, then I would start from List<> as well.

But as I see it now, it's not the best case, because removing item from the list will cause indexes to be changed (and this required locking of the whole list).

Perhaps you could to use array instead? Something like this:

NotificacionOrdenes[] todo = new NotificacionOrdenes[1000];

// producer
// find empty slot (null) in todo somehow
todo[empty_slot] = new NotificacionOrdenes();
...

// consumer
// find non-empty slot somehow
var notif = todo[non_empty_slot]
todo[non_empty_slot] = null;
..

as you can see there is no need for lock (checking for null and setting to null are safe), but you have to handle situation when there are no empty slots, if array is too small, or consumer is too slow.