Is this a safe way to use a condition variable without mutex

Question

My current code looks like this

void XXX::waitForUpdates()
{
    boost::unique_lock<boost::mutex> lock(mutex_agentDone);
    while(!allAgentUpdatesDone()) {
        COND_VAR_AGENT_DONE.wait(lock);
    }
}

void XXX::onAgentUpdate(YYY argums){
    Agent * target = const_cast<Agent*>(argums.GetAgent());
    boost::unique_lock<boost::mutex> lock(mutex_agentDone);
    REGISTERED_AGENTS.setDone(target,true);
        COND_VAR_AGENT_DONE.notify_all();
}

everything is fine, except when onAgentUpdate is called about a million times every 1 second, I have to worry about performance and optimization.

So I figured if I change the wait(lock) to timed_wait version that does the allAgentUpdatesDone() check, I can skip .notify()s which is otherwise invoked in the order of hundreds of thousands every second! Don't gasp, this is a simulation framework :)

Then I asked myseld: what do I need the mutex_agentDone for? I can modify the two functions like this:

void XXX::waitForUpdates()
{
    //this lock will become practically useless, coz there is no other 
    // mutex_agentDone being locked in any other function.
    boost::unique_lock<boost::mutex> lock(mutex_agentDone);
    while(!allAgentUpdatesDone()) {     
        COND_VAR_AGENT_DONE.timed_wait(lock,some_time_interval);
    }
}

void XXX::onAgentUpdate(YYY argums){
    Agent * target = const_cast<Agent*>(argums.GetAgent());
    REGISTERED_AGENTS.setDone(target,true)
}

The question is: is this safe?

thank you

A small note: Assume that the rest of operations in the two functions are already safeguarded by their own mutex (REGISTERED_AGENTS is a class object having a container and its own mutex being invoked in every accessor and iteration method, so allAgentUpdatesDone() is iterating through the same container using same mutex as REGISTERED_AGENTS)

Answer 1

You can do this:

void XXX::waitForUpdates()
{
    boost::unique_lock<boost::mutex> lock(mutex_agentDone);
    while(!allAgentUpdatesDone()) {
        ++waiters;
        COND_VAR_AGENT_DONE.wait(lock);
        --waiters;
    }
}

void XXX::onAgentUpdate(YYY argums){
    Agent * target = const_cast<Agent*>(argums.GetAgent());
    boost::unique_lock<boost::mutex> lock(mutex_agentDone);
    REGISTERED_AGENTS.setDone(target,true);
    if (waiters != 0)
        COND_VAR_AGENT_DONE.notify_all();
}

The mutex protects the waiters count. Make sure to set it to zero to start.

You would expect the condition variable to already have something like this, but just the overhead to invoke notify_all may be significant.

This assumes most of the time there are no waiters. If the issue is that most of the time allAgentUpdatesDone returns false, then don't call notify_all unless all updates are done.

Answer 2

I am not very acquainted with C++11 atomics but on Solaris you can use combination of volatile bool and membar ops like this

volatile bool done = false;
void XXX::waitForUpdates()
{
//    boost::unique_lock<boost::mutex> lock(mutex_agentDone);
    while(!allAgentUpdatesDone()) {
               while ( !done )
               {
                     usleep(1000000);
                     membar_consumer();
               }
    }
}

void XXX::onAgentUpdate(YYY argums){
    Agent * target = const_cast<Agent*>(argums.GetAgent());
    //boost::unique_lock<boost::mutex> lock(mutex_agentDone);
    membar_producer();
    REGISTERED_AGENTS.setDone(target,true);
        done = true;
}

Thanks Niraj Rathi