10

I stumbled over the following code in Bjarne Stroustrup's "The C++ Programming Language, 4th Edition" on page 119:

queue<Message> mqueue;
condition_variable mcond;
mutex mmutex;

void consumer()
{
    while(true) {
        unique_lock<mutex> lck{mmutex};
        mcond.wait(lck);

        auto m = mqueue.front();
        mqueue.pop();
        lck.unlock();
        // process m
    }
}

There is also a producer thread which pushes Message to the queue and notifies the waiting thread in a loop.

My question is: Is it required to create a new unique_lock in every iteration of the loop? It seems unneccesary to me, because in the next line, mcond.wait(lck), the lock gets unlocked directly after locking it in the line before.

From a performance point of view, couldn't the lck variable just be initialized before the loop starts?

Bobface
  • 2,782
  • 4
  • 24
  • 61
  • 2
    It's usually a good habit to keep things close to where they are used, close as in scope. It generally makes code easier to read, understand and most importantly to maintain. – Some programmer dude Jul 31 '17 at 12:27
  • @Someprogrammerdude I edited my question because I am interessted in the performance aspect. – Bobface Jul 31 '17 at 12:28
  • 1
    Most likely the optimizer will hoist that lock variable out of the loop for you. In short; it doesn't matter. Don't worry about it and spend your time on something more productive. – Jesper Juhl Jul 31 '17 at 12:28
  • As for the "because in the next line, mcond.wait(lck), the lock gets unlocked directly after locking it in the line before" part, moving the definition of `lck` out of the loop would not really make much difference here. The lock would be locked outside the loop, then immediately inside the loop be unlocked by the very same `wait` call. – Some programmer dude Jul 31 '17 at 12:28
  • @Someprogrammerdude In the first iteration of course. But how about the iterations after it? When the lock gets unlocked and `m` is processed, the loop starts again. However this time, the mutex does not get locked again before the wait call. Am I right with this one? – Bobface Jul 31 '17 at 12:30
  • 1
    Regarding performance worries, don't do that. Premature optimization is usually not a good way to start with a program. Instead write code that is simple, easy to read, understand, modify and maintain. *Then* if (and only if, remember that "good enough" usually *is* good enough) the performance is lacking you *measure* and *profile* to find the bottlenecks, and concentrate on the worst ones only. And remember to document and comment about the optimizations, since those tend to obfuscate code. – Some programmer dude Jul 31 '17 at 12:31
  • 1
    Ah, but the [`wait`](http://en.cppreference.com/w/cpp/thread/condition_variable/wait) function *expects* the lock to be locked! If it's not locked then you have *undefined behavior*. So the placement is not only about "closeness", but about avoiding problems. – Some programmer dude Jul 31 '17 at 12:32
  • Okay, thanks for the advice. If `wait` expects the lock to be locked then of course it has to be done this way. – Bobface Jul 31 '17 at 12:36
  • 5
    From a performance point of view, locking and unlocking a mutex is far more expensive than creating a `unique_lock`. (You're basically worrying about whether driving barefoot might improve your gas mileage.) – molbdnilo Jul 31 '17 at 12:50
  • According to http://en.cppreference.com/w/cpp/thread/condition_variable/wait : "When unblocked, regardless of the reason, lock is reacquired and wait exits". It seems the thread still has ownership of the mutex but it releases it while it sleeps to allow other threads to enter critical parts of their code. As such, requiring that the mutex is locked beforhand is likely a way to ensure that only the current thread owns the mutex. – patatahooligan Jul 31 '17 at 13:12

1 Answers1

8

As stated on cppreference, the std::unique_lock constructor is as such:

explicit unique_lock( mutex_type& m );

The constructor will do the following:

Constructs a unique_lock with m as the associated mutex. Additionally locks the associated mutex by calling m.lock(). The behavior is undefined if the current thread already owns the mutex except when the mutex is recursive.

So the code locks the mutex on every iteration. Moving it out of the loop would change the logic.

tambre
  • 4,625
  • 4
  • 42
  • 55
Vasiliy Galkin
  • 1,894
  • 1
  • 14
  • 25