Syncer improvements & Concurrency fixes #9159

maloel · 2021-06-04T03:30:42Z

Following the deadlock that Nir experienced, reworked some stuff (and not all because of the deadlock):

split off dispatcher code into dispatcher.cpp
syncer (timestamp) inactive stream handling moved into skip_missing_stream
syncer identity_matcher names now include the stream index
single_consumer_queue::_mutex is now mutable; size() and empty() const
revised logic inside single_consumer_queue
revised logic inside dispatcher
cancellable_time::try_sleep() is now templated, requires actual chrono duration (and not just its ::Rep)
syncer stop() calls now percolate recursively to shut down all queues; new syncs do nothing if syncer was stopped
fix 1 warning on the way :)

…past stop()

Nir-Az

Looks good, just 1 comment

Nir-Az · 2021-06-04T05:34:35Z

src/concurrency.h

        std::unique_lock<std::mutex> lock(_mutex);
-        if (_accepting)
+        _enq_cv.wait( lock, [this]() {


Why do we wait if we are not accepting?

wait( lock, pred )

is the same as:

while( ! pred() ) { wait( lock ); }

If we are not accepting, the queue should be empty, so we shouldn't really wait, no?

So you are relying on logic elsewhere that say (accepting = false) is the same as (size = 0)

Well yes, I guess so: compound predicates get less readable because of the !. Would it be better if I wrote the pred as return _accepting && _queue.size() < _cap; ?

Actually, it gets more complicated...
You want:

while( _accepting && _queue.size() >= _cap ) { wait(); }

So the logic inside the pred should be return ! _accepting || _queue.size() < _cap;...

Nir-Az · 2021-06-06T13:40:03Z

src/concurrency.h

-        _was_flushed = false;
-        const auto ready = [this]() { return (_queue.size() > 0) || _need_to_flush; };
-        if (!ready() && !_deq_cv.wait_for(lock, std::chrono::milliseconds(timeout_ms), ready))
+        if( ! _deq_cv.wait_for( lock,


This if statement is really confusing..
too much "!" operators..
I suggest rewrite it as a positive if so it will be more understandable.

Everything with these predicates is confusing :)

src/sync.cpp

aangerma · 2021-06-08T13:46:04Z

src/concurrency.h

-        if (_accepting)
+        _enq_cv.wait( lock, [this]() {
+            return _queue.size() < _cap; } );
+        if( ! _accepting )


Shouldn't it be inside the predicate ?

If we stop accepting then someone called stop() which calls clear() which empties out the queue which should make the predicate true and stop.

aangerma · 2021-06-08T14:16:12Z

src/dispatcher.cpp

+
+    // Wait until any dispatched is done...
+    {
+        std::lock_guard< std::mutex > lock( _dispatch_mutex );


Its not ensure that the dispatcher completely stopped , it can be on the middle of the last item.

It will make this code wait until the dispatcher is done

src/concurrency.h

aangerma · 2021-06-09T07:39:20Z

src/concurrency.h

-        if (!ready() && !_deq_cv.wait_for(lock, std::chrono::milliseconds(timeout_ms), ready))
+        if( ! _deq_cv.wait_for( lock,
+                                std::chrono::milliseconds( timeout_ms ),
+                                [this]() { return ! _accepting || ! _queue.empty(); } )


_accepting && ! _queue.empty(); will be more understandable and you will not need the || _queue.empty() )

while( ! pred() ) wait();

if pred() returns true then we still check the following line: _queue.empty() -- that's why it's there -- and return false.

ev-mp · 2021-06-10T16:47:22Z

src/dispatcher.cpp

+    // Wait until any dispatched is done...
+    {
+        std::lock_guard< std::mutex > lock( _dispatch_mutex );
+        assert( _queue.empty() );


Asserts will omitted in release builds, and as release is the default for most testing the condition won't be checked

maloel added 8 commits June 3, 2021 12:55

revise syncer inactive stream handling for timestamp-composite

41a7d27

mutable _mutex and const size(), empty() in concurrency.h

5f3aecd

add stream index to identity matchers

f9a4b3f

revise concurrency.h

7008767

cont'd revision of concurrency.h; split into dispatcher.cpp

f453160

concurrency API change lashbacks

c36fc6a

syncer::stop() recursion to all matchers; syncer doesn't do anything …

0c0af63

…past stop()

reduce warning count to 28

f4a9ef3

maloel requested review from ev-mp, aangerma and Nir-Az June 4, 2021 03:30

maloel added 2 commits June 4, 2021 07:25

untabify :(

b3456ae

remove duplicate line

95beebc

Nir-Az reviewed Jun 6, 2021

View reviewed changes

aangerma reviewed Jun 8, 2021

View reviewed changes

src/sync.cpp Show resolved Hide resolved

aangerma reviewed Jun 8, 2021

View reviewed changes

aangerma reviewed Jun 9, 2021

View reviewed changes

src/concurrency.h Show resolved Hide resolved

aangerma reviewed Jun 9, 2021

View reviewed changes

aangerma approved these changes Jun 9, 2021

View reviewed changes

Nir-Az approved these changes Jun 9, 2021

View reviewed changes

ev-mp approved these changes Jun 10, 2021

View reviewed changes

maloel changed the base branch from development to concsync June 12, 2021 12:06

maloel merged commit 5d1f9f3 into IntelRealSense:concsync Jun 12, 2021

maloel mentioned this pull request Jun 13, 2021

Concurrency & Syncer rework #9219

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Syncer improvements & Concurrency fixes #9159

Syncer improvements & Concurrency fixes #9159

maloel commented Jun 4, 2021

Nir-Az left a comment

Nir-Az Jun 4, 2021

maloel Jun 6, 2021

maloel Jun 6, 2021 •

edited

Loading

Nir-Az Jun 6, 2021

maloel Jun 6, 2021

maloel Jun 6, 2021

Nir-Az Jun 6, 2021

maloel Jun 6, 2021

aangerma Jun 8, 2021

maloel Jun 8, 2021

aangerma Jun 8, 2021 •

edited

Loading

maloel Jun 8, 2021

aangerma Jun 9, 2021 •

edited

Loading

maloel Jun 9, 2021

ev-mp Jun 10, 2021

Syncer improvements & Concurrency fixes #9159

Syncer improvements & Concurrency fixes #9159

Conversation

maloel commented Jun 4, 2021

Nir-Az left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maloel Jun 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aangerma Jun 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aangerma Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maloel Jun 6, 2021 •

edited

Loading

aangerma Jun 8, 2021 •

edited

Loading

aangerma Jun 9, 2021 •

edited

Loading