From ef574a7d768a6ebc88b889c23e83fc9435b03d73 Mon Sep 17 00:00:00 2001 From: jwang Date: Mon, 22 Jan 2018 22:30:30 +0800 Subject: [PATCH 1/3] optimize timeout judgement according to different condition 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 --- rmw_fastrtps_cpp/src/rmw_wait.cpp | 39 +++++++++++++++---------------- 1 file changed, 19 insertions(+), 20 deletions(-) diff --git a/rmw_fastrtps_cpp/src/rmw_wait.cpp b/rmw_fastrtps_cpp/src/rmw_wait.cpp index 2b7560255..7f6e5a175 100644 --- a/rmw_fastrtps_cpp/src/rmw_wait.cpp +++ b/rmw_fastrtps_cpp/src/rmw_wait.cpp @@ -148,21 +148,28 @@ rmw_wait( // If wait_timeout is not null and either of its fields are nonzero, we have to wait bool hasToWait = (wait_timeout && (wait_timeout->sec > 0 || wait_timeout->nsec > 0)) || !wait_timeout; - hasToWait &= !check_wait_set_for_data(subscriptions, guard_conditions, services, clients); + bool hasData = check_wait_set_for_data(subscriptions, guard_conditions, services, clients); + hasToWait &= !hasData; bool timeout = false; - - if (hasToWait) { - if (!wait_timeout) { + if (hasToWait && wait_timeout) { + auto predicate = [subscriptions, guard_conditions, services, clients]() { + return check_wait_set_for_data(subscriptions, guard_conditions, services, clients); + }; + auto n = std::chrono::duration_cast( + std::chrono::seconds(wait_timeout->sec)); + n += std::chrono::nanoseconds(wait_timeout->nsec); + timeout = !conditionVariable->wait_for(lock, n, predicate); + } else { + if (hasToWait && !wait_timeout) { conditionVariable->wait(lock); - } else { - auto predicate = [subscriptions, guard_conditions, services, clients]() { - return check_wait_set_for_data(subscriptions, guard_conditions, services, clients); - }; - auto n = std::chrono::duration_cast( - std::chrono::seconds(wait_timeout->sec)); - n += std::chrono::nanoseconds(wait_timeout->nsec); - timeout = !conditionVariable->wait_for(lock, n, predicate); + hasData = check_wait_set_for_data(subscriptions, guard_conditions, services, clients); + } + // Even if this was a non-blocking wait, signal a timeout if there's no data. + // This makes the return behavior consistent with rcl expectations for zero timeout value. + // Do this before detaching the listeners because the data gets cleared for guard conditions. + if (!hasData && wait_timeout && wait_timeout->sec == 0 && wait_timeout->nsec == 0) { + timeout = true; } } @@ -173,14 +180,6 @@ rmw_wait( // after we check, it will be caught on the next call to this function). lock.unlock(); - // Even if this was a non-blocking wait, signal a timeout if there's no data. - // This makes the return behavior consistent with rcl expectations for zero timeout value. - // Do this before detaching the listeners because the data gets cleared for guard conditions. - bool hasData = check_wait_set_for_data(subscriptions, guard_conditions, services, clients); - if (!hasData && wait_timeout && wait_timeout->sec == 0 && wait_timeout->nsec == 0) { - timeout = true; - } - if (subscriptions) { for (size_t i = 0; i < subscriptions->subscriber_count; ++i) { void * data = subscriptions->subscribers[i]; From 08ab218d51e5429ac1580d02cfef8dffd5930604 Mon Sep 17 00:00:00 2001 From: jwang Date: Thu, 25 Jan 2018 21:36:31 +0800 Subject: [PATCH 2/3] replace cv->wait(lock) with cv->wait(lock, predicate) and simplify timeout logic Signed-off-by: jwang --- rmw_fastrtps_cpp/src/rmw_wait.cpp | 30 +++++++++++------------------- 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/rmw_fastrtps_cpp/src/rmw_wait.cpp b/rmw_fastrtps_cpp/src/rmw_wait.cpp index 7f6e5a175..eee6f6e58 100644 --- a/rmw_fastrtps_cpp/src/rmw_wait.cpp +++ b/rmw_fastrtps_cpp/src/rmw_wait.cpp @@ -149,27 +149,19 @@ rmw_wait( bool hasToWait = (wait_timeout && (wait_timeout->sec > 0 || wait_timeout->nsec > 0)) || !wait_timeout; bool hasData = check_wait_set_for_data(subscriptions, guard_conditions, services, clients); - hasToWait &= !hasData; + auto predicate = [subscriptions, guard_conditions, services, clients]() { + return check_wait_set_for_data(subscriptions, guard_conditions, services, clients); + }; bool timeout = false; - if (hasToWait && wait_timeout) { - auto predicate = [subscriptions, guard_conditions, services, clients]() { - return check_wait_set_for_data(subscriptions, guard_conditions, services, clients); - }; - auto n = std::chrono::duration_cast( - std::chrono::seconds(wait_timeout->sec)); - n += std::chrono::nanoseconds(wait_timeout->nsec); - timeout = !conditionVariable->wait_for(lock, n, predicate); - } else { - if (hasToWait && !wait_timeout) { - conditionVariable->wait(lock); - hasData = check_wait_set_for_data(subscriptions, guard_conditions, services, clients); - } - // Even if this was a non-blocking wait, signal a timeout if there's no data. - // This makes the return behavior consistent with rcl expectations for zero timeout value. - // Do this before detaching the listeners because the data gets cleared for guard conditions. - if (!hasData && wait_timeout && wait_timeout->sec == 0 && wait_timeout->nsec == 0) { - timeout = true; + if (hasToWait && !hasData) { + if (!wait_timeout) { + conditionVariable->wait(lock, predicate); + } else { + auto n = std::chrono::duration_cast( + std::chrono::seconds(wait_timeout->sec)); + n += std::chrono::nanoseconds(wait_timeout->nsec); + timeout = !conditionVariable->wait_for(lock, n, predicate); } } From 128309e180f679090d720e944d9b71f596d6cbea Mon Sep 17 00:00:00 2001 From: jwang Date: Thu, 1 Feb 2018 04:57:33 +0800 Subject: [PATCH 3/3] Improve logic to return TIMEOUT when not hasData && timeout->sec/nsec==0 Signed-off-by: jwang --- rmw_fastrtps_cpp/src/rmw_wait.cpp | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/rmw_fastrtps_cpp/src/rmw_wait.cpp b/rmw_fastrtps_cpp/src/rmw_wait.cpp index eee6f6e58..30f53250a 100644 --- a/rmw_fastrtps_cpp/src/rmw_wait.cpp +++ b/rmw_fastrtps_cpp/src/rmw_wait.cpp @@ -143,25 +143,22 @@ rmw_wait( // otherwise the decision to wait might be incorrect std::unique_lock lock(*conditionMutex); - // First check variables. - // If wait_timeout is null, wait indefinitely (so we have to wait) - // If wait_timeout is not null and either of its fields are nonzero, we have to wait - bool hasToWait = (wait_timeout && (wait_timeout->sec > 0 || wait_timeout->nsec > 0)) || - !wait_timeout; bool hasData = check_wait_set_for_data(subscriptions, guard_conditions, services, clients); auto predicate = [subscriptions, guard_conditions, services, clients]() { return check_wait_set_for_data(subscriptions, guard_conditions, services, clients); }; bool timeout = false; - if (hasToWait && !hasData) { + if (!hasData) { if (!wait_timeout) { conditionVariable->wait(lock, predicate); - } else { + } else if (wait_timeout->sec > 0 || wait_timeout->nsec > 0) { auto n = std::chrono::duration_cast( std::chrono::seconds(wait_timeout->sec)); n += std::chrono::nanoseconds(wait_timeout->nsec); timeout = !conditionVariable->wait_for(lock, n, predicate); + } else { + timeout = true; } }