[core] CSndUList use notify_one() instead of notify_all()

[maxsharabayko]

The CSndUList::signalInterrupt() and CSndUList::realloc_() are using Condition::notify_all() (pthread_cond_broadcast) to notify the sending queue is non empty or needs to be interrupted and closed.
The only receiver of this notification is the CSndQueue::worker(void*) thread. As of now, there is only one instance of this thread, and there is no need to use the broadcast notification.

Note. Can the notify_all() be less performant than the notify_one()?

SRT version: SRT v1.4.4, after PR #2045.

[maxsharabayko]

The performance of the notify_one() (pthread_cond_signal()) and the notify_all (pthread_cond_broadcast()) seems to be more or less similar.
Tested release build on a MacBookPro:

• Processor Name: Quad-Core Intel Core i7
• Processor Speed: 2,5 GHz
• Number of Processors: 1
• Total Number of Cores: 4
• L2 Cache (per Core): 256 KB
• L3 Cache: 6 MB

Maybe the notify_one() has a bit higher chance to hit below 30 microseconds, while the notify_all seems to have a bit more stable results around 40 microseconds.

TEST(SyncEvent, WaitNotifyOne)
{
    Mutex mutex;
    Condition cond;
    cond.init();

    bool force_break = false;
    steady_clock::time_point timestamp;

    auto wait_async = [](Condition* cond, Mutex* mutex, steady_clock::time_point& ts, bool& must_break) {
        while (true) {
            UniqueLock lock(*mutex);
            cond->wait(lock);
            steady_clock::time_point end = steady_clock::now();
            if (must_break)
                break;
            std::cerr << count_microseconds(end - ts) << '\n';
        }
    };
    auto wait_async_res = async(launch::async, wait_async, &cond, &mutex, std::ref(timestamp), std::ref(force_break));
    EXPECT_EQ(wait_async_res.wait_for(chrono::milliseconds(50)), future_status::timeout);

    for (int i = 0; i < 10000; ++i)
    {
        {
            ScopedLock lock(mutex);
            timestamp = steady_clock::now();
            cond.notify_one();
        }
        std::this_thread::sleep_for(std::chrono::microseconds(500));
    }

    force_break = true;
    cond.notify_one();
    wait_async_res.wait();
    cond.destroy();
}

TEST(SyncEvent, WaitNotifyAll)
{
    Mutex mutex;
    Condition cond;
    cond.init();

    bool force_break = false;
    steady_clock::time_point timestamp;

    auto wait_async = [](Condition* cond, Mutex* mutex, steady_clock::time_point& ts, bool& must_break) {
        while (true) {
            UniqueLock lock(*mutex);
            cond->wait(lock);
            steady_clock::time_point end = steady_clock::now();
            if (must_break)
                break;
            std::cerr << count_microseconds(end - ts) << '\n';
        }
    };
    auto wait_async_res = async(launch::async, wait_async, &cond, &mutex, std::ref(timestamp), std::ref(force_break));
    EXPECT_EQ(wait_async_res.wait_for(chrono::milliseconds(50)), future_status::timeout);

    for (int i = 0; i < 10000; ++i)
    {
        {
            ScopedLock lock(mutex);
            timestamp = steady_clock::now();
            cond.notify_all();
        }
        std::this_thread::sleep_for(std::chrono::microseconds(500));
    }

    force_break = true;
    cond.notify_one();
    wait_async_res.wait();
    cond.destroy();
}

[gou4shi1]

Maybe unlock before notify would be better (minor).
isocpp/CppCoreGuidelines#1272

[maxsharabayko]

Windows 10 C++11 ENABLE_STDCXX_SYNC=ON

[maxsharabayko]

Maybe unlock before notify would be better (minor). isocpp/CppCoreGuidelines#1272

The results are quite similar.

[jlsantiago0]

NOTE: Notify one tends to be a misfeature IMO in many contexts. It makes code very hard to maintain and algorithms hard to conceptualize. Because if someone later adds code to use that same conditional variable in another part of the code, they may not know that notify-one is being used in some other parts of the code. And this can lead to deadlocks or users of the conditional variable not getting woken up when they expect to be anymore, because this other part of the code base got woken up instead and they never will be woken up or may be at some unexpected much later time in a very inconsistent way. And will likely have dramatic variance of outcome on different systems and on systems under different workloads.

It is really only useful if it is hidden for instance inside a FIFO type system where multiple threads can be waiting on data to push or pop to/from a FIFO type object and might need to wait for data or wait for space then, it might make sense to only wake one. But calling wake one, manually to implement algorithms where later some code would also start waiting and potentially get woken instead of the original code that was being woken by wake one. Leads to maintainence nightmares as you have to document this requirement and make sure that it is maintained over time. Which tends to get lost.

[maxsharabayko]

Yeap, I thought about this disadvantage. Still, there is only one thread listening for the notification, and it makes sense to call notify_one(). If there were several sending threads listening, a proper synchronization and scheduling would have arisen anyway, and the whole related logic would need to be reconsidered.
I didn't see a significant performance improvement though. Maybe on some other platforms, e.g. mobile, it differs.

[ethouris]

The idea behind the CVs is to allow threads to sleep until there is a signal that would be interesting for them. If approached this way, nofify_one() makes sense only if you have exactly one such thread (and never expand the code to add more), but notify_all() makes actually the same thing.

The only case when notify_one() might make sense is when you have multiple waiting threads, of which one after being woken up, having acquired a mutex, can do some time consuming things and during that time can't do the processing for the signal on a CV, while, say, 4 other threads can do this, so on the next signal there's woken up only one of them. The condition should be that all these threads should do merely the same thing by "responding to a signal", and it doesn't matter which one picks up the request, just only one of them should do it, while previously started handlers can be executed also parallelly. Of course the same thing would be achieved if you have just one waiting handler and it would spawn a new thread when signaled, but spawning a thread might cost more than having some pre-spawned and woken up at this time for takeover.

If you have simply a CV and potentially more than one thread that should get woken up on CV signal, acquire a socket, do something with the data inside, and each one is of completely different purpose, then notify_all() is the only sensible soluition, even if you have only one such "responder thread" presently in the code.

[jlsantiago0]

Yep. I understand the desire to only wake one thread. And that is much better performance. I have been involved in the past in debugging a number of hard to find bugs that get introduced but not discovered until much later where adding another waiter breaks things in subtle hard to find ways. It is often the case that the second waiter is introduced at a much later time when the original code was implemented. And then much later than that, someone identifies that there is a problem of some kind. You are right that you want that desired effect. Sometimes the single waiter requirements for the algorithm even when documented get lost subsequently by other changes much later.

But yeah. you are right. I was just indicating that we consider the consequences. Quite often it helps to hide the wait/wake in a way that it cant be used accidentally after the context of why the single waiter was implemented and must be only a single waiter.

Also if you have a single waiter and only one wake event, YOU MUST make sure that you are holding the mutex when calling notify. Otherwise it could be lost. You cant in that situation notify after you release the mutex. I noticed there was some discussion above about notifying after releasing the mutex to improve performance.

[ethouris]

Also "desire to wake up one thread" isn't helped by notify_one(). If you have a procedure that something "gets available", all threads wait on a CV and will acquire a mutex atomically after being woken up, rest of the threads will still be frozen until the mutex is unlocked. So the matter to consider here is only if the notification on the CV means "ONE exactly new gem has appeared, catch it the first that is fast enough!" or rather "some vital data were updated here, you might want to consider some synchronization on your side". There could be some desire to minimize waking up threads that wouldn't do a thing with these updated data - but then, if they wouldn't, then likely they shouldn't also sleep on CV.

[jlsantiago0]

Right when it matters which particular waiter gets woken by notify_once, then you have a problem of maintainence, documentation, and potential breakage. If it doesnt matter if a particular thread ever gets woken in the notify context, then you are fine.

Like i said, i was just asking to consider the consequences. If you guys think that benefits of using notify_once overrides the potential downsides, then it is likely the correct decision. Just keep in mind that the consequences tend to happen over time often, unless you can make sure that CV cant be acidently used for some other purpose.