optimize timeout judgement according to different condition

[jwang11]

Under the condition that "hasToWait && wait_timeout", there is no need to re-calc hasData to judge timeout because predicate already include the logic. We only need do it if hasToWait && !wait_timeout. This commit optimize timeout judgement which slightly improve the performance in some conditions

Signed-off-by: jwang [email protected]

[sloretz]

std::condition_variable::wait(...) is allowed to spuriously wake up. This could cause rmw_wait() to return with no data despite being told to wait forever. Would you mind including a fix for this by changing it to conditionVariable->wait(lock, predicate)?

[sloretz]

I think this code will work fine. If I understand correctly in pseudo code it says

if (no data and timeout > 0)  // line 155
    wait until there is data with timeout
    return timeout error if still no data  // line 162
else  // line 163
    if (no data and timeout == NULL)  // line 164
        wait until there is data
    if (no data and timeout == 0)  // line 171
        return timeout error

What do you think about separating having data initially from the timeout logic?

if (no data)
    if (timeout == NULL)
        wait until there is data
    else if (timeout > 0)
        wait until there is data with timeout
        return timeout error if still no data
    else
        return timeout error

[jwang11]

Ok, I'll replace conditionVariable->wait(lock) with conditionVariable->wait(lock, predicate) and update the timeout logic to make it more readable.

[sloretz]

@jwang11 Thanks! It looks good to me. I started some CI jobs.

Edit: Ooops passed wrong link for repos, restarted CI

• Linux
• Linux-aarch64
• macOS
• Windows

[wjwwood]

Looks like there are failing tests related to this change (or it seems so).

[jwang11]

I paste error test as below

21:39:15 32: [ RUN      ] WaitSetTestFixture__rmw_fastrtps_cpp.zero_timeout
21:39:15 32: /home/rosbuild/ci_scripts/ws/src/ros2/rcl/rcl/test/rcl/test_wait.cpp:160: Failure
21:39:15 32: Expected equality of these values:
21:39:15 32:   2
21:39:15 32:   ret
21:39:15 32:     Which is: 0
21:39:15 32: error not set
21:39:15 32: [  FAILED  ] WaitSetTestFixture__rmw_fastrtps_cpp.zero_timeout (1 ms)
21:39:15 32: [ RUN      ] WaitSetTestFixture__rmw_fastrtps_cpp.zero_timeout_triggered_guard_condition
21:39:15 32: [       OK ] WaitSetTestFixture__rmw_fastrtps_cpp.zero_timeout_triggered_guard_condition (0 ms)
21:39:15 32: [ RUN      ] WaitSetTestFixture__rmw_fastrtps_cpp.canceled_timer
21:39:15 32: [       OK ] WaitSetTestFixture__rmw_fastrtps_cpp.canceled_timer (10 ms)
21:39:15 32: [ RUN      ] WaitSetTestFixture__rmw_fastrtps_cpp.excess_capacity
21:39:15 32: [       OK ] WaitSetTestFixture__rmw_fastrtps_cpp.excess_capacity (1 ms)
21:39:15 32: [ RUN      ] WaitSetTestFixture__rmw_fastrtps_cpp.multi_wait_set_threaded
21:39:15 32: [       OK ] WaitSetTestFixture__rmw_fastrtps_cpp.multi_wait_set_threaded (24 ms)
21:39:15 32: [ RUN      ] WaitSetTestFixture__rmw_fastrtps_cpp.guard_condition
21:39:15 32: [       OK ] WaitSetTestFixture__rmw_fastrtps_cpp.guard_condition (10 ms)
21:39:15 32: [----------] 9 tests from WaitSetTestFixture__rmw_fastrtps_cpp (66 ms total)
21:39:15 32: 
21:39:15 32: [----------] Global test environment tear-down
21:39:15 32: [==========] 9 tests from 1 test case ran. (66 ms total)
21:39:15 32: [  PASSED  ] 8 tests.
21:39:15 32: [  FAILED  ] 1 test, listed below:
21:39:15 32: [  FAILED  ] WaitSetTestFixture__rmw_fastrtps_cpp.zero_timeout

[wjwwood]

@jwang11 can you reproduce that locally? I'd expect that's related to your change and will require a change in the tests or in your changes.

[jwang11]

@wjwwood I'll try this test case locally today, looks like caused by this change.

[mikaelarguedas]

Removing this is what causes the tests to fail. If the timeout is reached (even if the timeout is 0) RMW_RET_TIMEOUT should be returned (or at least that's what the test expects)

[jwang11]

Ok, I got the point. If wait_timeout->sec == 0 && wait_timeout->nsec == 0 and there is no data, it should return TIMEOUT. thanks @mikaelarguedas . IMHO, TIMEOUT is not good enough in such case, anyway it is better than return 0.

[jwang11]

17: [==========] Running 9 tests from 1 test case.
17: [----------] Global test environment set-up.
17: [----------] 9 tests from WaitSetTestFixture__rmw_fastrtps_cpp
17: [ RUN ] WaitSetTestFixture__rmw_fastrtps_cpp.test_resize_to_zero
17: [ OK ] WaitSetTestFixture__rmw_fastrtps_cpp.test_resize_to_zero (0 ms)
17: [ RUN ] WaitSetTestFixture__rmw_fastrtps_cpp.finite_timeout
17: [ OK ] WaitSetTestFixture__rmw_fastrtps_cpp.finite_timeout (10 ms)
17: [ RUN ] WaitSetTestFixture__rmw_fastrtps_cpp.negative_timeout
17: [ OK ] WaitSetTestFixture__rmw_fastrtps_cpp.negative_timeout (10 ms)
17: [ RUN ] WaitSetTestFixture__rmw_fastrtps_cpp.zero_timeout
17: [ OK ] WaitSetTestFixture__rmw_fastrtps_cpp.zero_timeout (0 ms)
17: [ RUN ] WaitSetTestFixture__rmw_fastrtps_cpp.zero_timeout_triggered_guard_condition
17: [ OK ] WaitSetTestFixture__rmw_fastrtps_cpp.zero_timeout_triggered_guard_condition (0 ms)
17: [ RUN ] WaitSetTestFixture__rmw_fastrtps_cpp.canceled_timer
17: [ OK ] WaitSetTestFixture__rmw_fastrtps_cpp.canceled_timer (10 ms)
17: [ RUN ] WaitSetTestFixture__rmw_fastrtps_cpp.excess_capacity
17: [ OK ] WaitSetTestFixture__rmw_fastrtps_cpp.excess_capacity (1 ms)
17: [ RUN ] WaitSetTestFixture__rmw_fastrtps_cpp.multi_wait_set_threaded
17: [ OK ] WaitSetTestFixture__rmw_fastrtps_cpp.multi_wait_set_threaded (21 ms)
17: [ RUN ] WaitSetTestFixture__rmw_fastrtps_cpp.guard_condition
17: [ OK ] WaitSetTestFixture__rmw_fastrtps_cpp.guard_condition (10 ms)
17: [----------] 9 tests from WaitSetTestFixture__rmw_fastrtps_cpp (62 ms total)
17:
17: [----------] Global test environment tear-down
17: [==========] 9 tests from 1 test case ran. (62 ms total)
17: [ PASSED ] 9 tests.

[sloretz]

Started CI (testing 128309e, but github UI displays it after since the commit time stamp is 4 hours ahead of this comment).

• Linux
• Linux-aarch64
• macOS
• Windows

[sloretz]

Thanks for the pull request!

[mikaelarguedas]

lgtm