Sound lags in multi-thread version

[Codeplug]

Here's a simple shared Q that I often use:

#include <deque>
#include <stdexcept>

template <typename T>
class deque_MT
{
    mutable CRITICAL_SECTION m_cs;
    mutable HANDLE m_consumerEvent;

    // RAII for CS
    struct Lock
    {
        CRITICAL_SECTION &m_cs;
        Lock(CRITICAL_SECTION &cs) : m_cs(cs) {::EnterCriticalSection(&m_cs);}
        ~Lock() {::LeaveCriticalSection(&m_cs);}
    };//Lock

    std::deque<T> m_queue;

public:
    deque_MT() throw (runtime_error)
    {
        m_consumerEvent = ::CreateEventW(0, TRUE, FALSE, 0);
        if (!m_consumerEvent)
            throw std::runtime_error("deque_MT::deque_MT() CreateEvent failed");
        
        if (!::InitializeCriticalSectionAndSpinCount(&m_cs, 4000))
        {
            ::CloseHandle(m_consumerEvent);
            throw std::runtime_error("deque_MT::deque_MT() Init-CS failed");
        }//if
    }//constructor

    ~deque_MT()
    {
        ::CloseHandle(m_consumerEvent);
        ::DeleteCriticalSection(&m_cs);
    }//destructor

    HANDLE consumer_event() const {return m_consumerEvent;}

    size_t size() const {Lock l(m_cs); return m_queue.size();}
    bool empty() const {return size() == 0;}

    void clear() 
    {
        Lock l(m_cs); 
        m_queue.clear();
        ::ResetEvent(m_consumerEvent);
    }//clear

    // Add an object to the queue and signal the consumer event
    void enqueue(const T& val)
    {
        Lock l(m_cs); 
        m_queue.push_front(val);
        ::SetEvent(m_consumerEvent);
    }//enqueue

    // Remove next object from the queue, returns false if Q is empty
    // NOTE: With multiple consumers, event can be signaled with an empty Q
    bool dequeue(T &val) throw(runtime_error)
    {
        Lock l(m_cs);
        size_t sz = m_queue.size();
        if (sz == 0)
            return false;

        if (sz == 1) // this will drain the Q
            ::ResetEvent(m_consumerEvent);

        val = m_queue.back();
        m_queue.pop_back();
        return true;
    }//dequeue
};//class deque_MT

Things to note:

STL container completely protected by CS.

Manual reset event - stays signaled until Q is drained.

Scope of CS limited to only what needs protection. I noticed that Play(m_PlayingID) is called within the scope of m_CS - which would unnecessarily block threads calling DXSoundEmitter::playSound(), diminishing throughput. If multiple "things" need synchronization, consider using a synchronization object for each "thing".

So here is how I would re-factor the posted code:

void DXSoundEmitter::Loop()
{
    const size_t NumWaitObjs = 2;
    const HANDLE WaitObjs[NumWaitObjs] =
    {
        m_ExitEvent, // <-- new member variable, auto reset event
        m_PlayList.consumer_event(),
    };//WaitObjs

    enum
    {
        // must keep these in sync with WaitObjs array
        EXIT_SIGNALED     = WAIT_OBJECT_0 + 0,
        PLAYLIST_SIGNALED = WAIT_OBJECT_0 + 1,
    };

    DWORD status = WaitForMultipleObjects(NumWaitObjs, WaitObjs, 
                                          FALSE, INFINITE);
    if (status == PLAYLIST_SIGNALED)
    {
        // if only a single consumer (ie this is the only thread that can cause
        // the consumer event to reset), then a signaled event guarantees there's
        // something in the Q - but safety first
        unsigned int nextID;
        if (m_PlayList.dequeue(nextID))
        {
            m_PlayingID = nextID;
            Play(m_PlayingID);
        }//if
    }//if
    else if (status == EXIT_SIGNALED)
    {
        Stop();
        return;
    }//else if
    else
    {
        // TODO - log unexpected return from WFMO
    }//else
}//Loop

void DXSoundEmitter::playSound(unsigned int ID)
{
    m_PlayList.enqueue(ID);
}//playSound

void DXSoundEmitter::flushThread()
{
    ::SetEvent(m_ExitEvent);
}//flushThread

I used another event for exit notification. Not much different from what you had - but since deque_MT<> took ownership of the the protecting CS, another synchronization object was needed. If m_SoundState ever has more states than just run/stop, what you can do is rename m_ExitEvent to m_SoundStateEvent - then signal the event whenever a state change is made externally (you'll need to assume atomicity of reads/writes or use interlocked reads/writes).

When using WFMO (and not waiting on all objects to be signaled), keep in mind that the first event in the array has "priority" over the second event. So in the above example, if the exit event were second, the thread may not exit as quickly as you would like (or at all) under a high m_PlayList load.

As an exercise, an alternative method for signaling thread exit could be to use a sentinel ID of 0:

void DXSoundEmitter::playSound(unsigned int ID)
{
    if (ID != 0)
        m_PlayList.enqueue(ID);
}//playSound

void DXSoundEmitter::flushThread()
{
    m_PlayList.clear();
    m_PlayList.enqueue(0);
}//flushThread

Couple things to note here: The call to clear() will reset the consumer event. This means that the "status == PLAYLIST_SIGNALED" code could be executing even though the Q is actually empty. We check for a false return from dequeue() so we're ok there.
You may also be thinking: "Hey, we can now spin on WFSO instead of WFMO. Surely, checking a single event is faster than checking an array of events!" Premature optimization at its finest! The metric to monitor in this case is throughput. Any small speedup in the looping construct would be completely washed out by the overhead incurred by the work-horse: Play().

So that "exercise" was really a lesson in premature optimization - not that anyone *needed* a lesson of course :) I simply found myself needing the lesson as I thought "how can I make this exit notification as fast and efficient as possible?". As always, follow K.I.S.S., profile, then optimize.

gg