[New Feature] Timeout-capable channels #51

dario-loi · 2025-01-08T18:19:35Z

Features

I tried implementing my spin on #50 in the library while keeping API compatibility.

A timeout can be set on channel ch like this:

ch.setTimeout(std::chrono::milliseconds(100));

Or, if the user is using >=C++14, in the more convenient way:

ch.setTimeout(100ms); // millis
ch.setTimeout(1s); // seconds

This applies a timeout on all operator>>,operator<< performed on the channel by using the already-existing condition variable member. if the timeout is exceeded, a channel_timeout exception is thrown.

The timeout can be removed by calling

ch.clearTimeout();

Which is just a convenient alias for ch.setTimeout(std::chrono::nanoseconds::zero())

The timeout_ member is wrapped in a std::atomic variable to prevent hazards during multi-threaded execution in a (hopefully) lock-free manner

Compatibility with existing code

The new header library runs all previous tests and examples successfully. New tests have been added to the channel_test.cpp suite to cover the new timeout functionality..

Branching on whether a timeout is present or not always costs a bit of overhead on every operation as shown in the Benchmarks section.
Even if the library maintains 100% compatibility with previous versions, the feature provided here is not a zero-cost abstraction, so it is up to the maintainers to decide if its inclusion in the main repo is worth it.

Benchmarks

The timeout code adds a 7ns fixed overhead to operations when no timeout is being used, for a performance pessimisation of around ~20% (benchmarks below run on my i5-6500 with freq scaling off, release build).

>>> MASTER

Run on (4 X 3299.99 MHz CPU s)
CPU Caches:
  L1 Data 32 KiB (x4)
  L1 Instruction 32 KiB (x4)
  L2 Unified 256 KiB (x4)
  L3 Unified 6144 KiB (x1)
Load Average: 8.98, 5.18, 4.13
---------------------------------------------------------------
Benchmark                     Time             CPU   Iterations
---------------------------------------------------------------
BM_ChannelWithInt          33.7 ns         32.8 ns     20944355
BM_ChannelWithString       38.9 ns         38.2 ns     18428982
BM_ChannelWithStruct       41.0 ns         40.7 ns     17215072

>>> TIMEOUT

Run on (4 X 3299.99 MHz CPU s)
CPU Caches:
  L1 Data 32 KiB (x4)
  L1 Instruction 32 KiB (x4)
  L2 Unified 256 KiB (x4)
  L3 Unified 6144 KiB (x1)
Load Average: 8.16, 5.59, 4.33
---------------------------------------------------------------
Benchmark                     Time             CPU   Iterations
---------------------------------------------------------------
BM_ChannelWithInt          40.4 ns         40.2 ns     17433770
BM_ChannelWithString       45.6 ns         45.3 ns     15567840
BM_ChannelWithStruct       47.1 ns         46.9 ns     14965060

dario-loi · 2025-01-08T18:33:01Z

Edit: removed [[likely]] and [[unlikely]] tags for C++11 compliance (sorry, thought they were less modern)

New benchmarks without hand tuning show slight reduction in overhead in the int case.

>>> MASTER

Run on (4 X 3299.99 MHz CPU s)
CPU Caches:
  L1 Data 32 KiB (x4)
  L1 Instruction 32 KiB (x4)
  L2 Unified 256 KiB (x4)
  L3 Unified 6144 KiB (x1)
Load Average: 8.98, 5.18, 4.13
---------------------------------------------------------------
Benchmark                     Time             CPU   Iterations
---------------------------------------------------------------
BM_ChannelWithInt          33.7 ns         32.8 ns     20944355
BM_ChannelWithString       38.9 ns         38.2 ns     18428982
BM_ChannelWithStruct       41.0 ns         40.7 ns     17215072

>>> TIMEOUT

Run on (4 X 3367.31 MHz CPU s)
CPU Caches:
  L1 Data 32 KiB (x4)
  L1 Instruction 32 KiB (x4)
  L2 Unified 256 KiB (x4)
  L3 Unified 6144 KiB (x1)
Load Average: 5.70, 3.24, 2.79
---------------------------------------------------------------
Benchmark                     Time             CPU   Iterations
---------------------------------------------------------------
BM_ChannelWithInt          36.6 ns         36.5 ns     19156562
BM_ChannelWithString       46.3 ns         46.2 ns     15880156
BM_ChannelWithStruct       45.9 ns         45.7 ns     16235551

andreiavrammsd · 2025-01-08T18:39:46Z

I'm giving this feature a chance. I will review it after all the jobs have been passed.

dario-loi · 2025-01-08T22:47:14Z

Tried debugging the supposed race condition but it is hard to detect exactly where the double lock is happening. For Clang and MSVC there is no problem, and this bug report https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101978 suggests that in our situation (wait_for usage) GCC < 12.0.0 throws a false positive when using thread sanitizer.

Any chance that the CI/CD could be upgraded to use a more modern version of GCC to check if it's a compiler problem? (that would be the first I witness).

edit: local compilation on my machine (Arch 6.12.8-zen1-1.1-zen, GCC 14.2.1) shows no errors, so a compiler bug in CI/CD is not unlikely.

andreiavrammsd · 2025-01-09T18:00:28Z

Tried debugging the supposed race condition but it is hard to detect exactly where the double lock is happening. For Clang and MSVC there is no problem, and this bug report https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101978 suggests that in our situation (wait_for usage) GCC < 12.0.0 throws a false positive when using thread sanitizer.

Any chance that the CI/CD could be upgraded to use a more modern version of GCC to check if it's a compiler problem? (that would be the first I witness).

edit: local compilation on my machine (Arch 6.12.8-zen1-1.1-zen, GCC 14.2.1) shows no errors, so a compiler bug in CI/CD is not unlikely.

You can upgrade the jobs to Ubuntu 24.04. This version might have GCC 14.

andreiavrammsd · 2025-01-09T18:39:27Z

I will review this soon.

andreiavrammsd · 2025-01-13T15:04:19Z

.github/workflows/cmake.yml

+          sudo apt-get update
+          sudo apt-get install -y gcc-12 g++-12 cmake


"gcc-12 is already the newest version (12.3.0-1ubuntu1~22.04)."

Is update-alternatives enough?

By removing the apt-get call I get:

| update-alternatives: error: alternative path /usr/bin/gcc-12 doesn't exist [build/Ubuntu 22.04 GCC (Debug)-2 ] ❌ Failure - Main Install GCC 12

I am honestly not familiar with Ubuntu so maybe I am not doing this correctly.

If you tried this locally, maybe you don't have GCC 12 already installed. On the GitHub runner, I see 12 is already installed, but not used as default. So just try it here without apt-get update and install.

README.md

andreiavrammsd · 2025-01-13T15:08:10Z

include/msd/channel.hpp

    friend class blocking_iterator<channel>;
 };

 }  // namespace msd

 #include "channel.inl"
-


Please don't resolve conversations. They help to see if we understand one another. The author of the conversation resolves it when the changes are done accordingly.

In this specific case, #include "channel.inl" is needed.

include/msd/channel.inl

examples/timeout.cpp

andreiavrammsd · 2025-01-15T14:24:16Z

include/msd/channel.hpp

+     * @param timeout Duration after which operations will time out.
+     */
+    template <typename Rep, typename Period>
+    void setTimeout(const std::chrono::duration<Rep, Period>& timeout);


Is there a really useful scenario where you want to set a timeout after the channel has been declared, clear the timeout, or change it?

Or would it be enough to have an overloaded constructor that accepts the capacity and the timeout? And then timeout_ does not need to be atomic. Simplicity is usually better.

Good point, this is probably over-engineered since most of the times you do not really need that to change at runtime/past the program's initialization.

The thing is that when I initially programmed this I tried to add time-out functionality on top of the original implementation as much as possible, so I was reluctant to touch the constructors, this is a good idea however and I'll refactor the PR as soon as I have time.

andreiavrammsd · 2025-01-15T14:41:51Z

include/msd/channel.inl

+    }
+
+    return cnd_.wait_for(lock, timeout, pred);
+}


Do you need to verify for if timeout_ is zero()? How about the following?

template <typename T> template <typename Predicate> bool channel<T>::waitWithTimeout(std::unique_lock<std::mutex>& lock, Predicate pred) { auto timeout = timeout_.load(std::memory_order_relaxed); return cnd_.wait_for(lock, timeout, pred); }

From cppreference:

wait_for causes the current thread to block until the condition variable is notified, the given duration has been elapsed, or a spurious wakeup occurs. pred can be optionally provided to detect spurious wakeup.

rel_time - the maximum duration to wait

Which means that if timeout == 0 (so our channel does not have a timeout, if we go with our proposed timeout-at-construction semantics), wait_for will never wait for the predicate and will instantly timeout (problem!).

I think that explicit detection of timeout == 0 is necessary as this is not really a timeout of 0s, but a way for us to have a null timeout without:

Having access to std::optional (C++17 >= only)

Using nullptr

I will have another look at this.

andreiavrammsd · 2025-01-15T14:45:17Z

include/msd/channel.inl

@@ -67,15 +103,15 @@ void channel<T>::close() noexcept
 {
    {
        std::unique_lock<std::mutex> lock{mtx_};
-        is_closed_.store(true);
+        is_closed_.store(true, std::memory_order_relaxed);


The idea is that we do not need explicit memory ordering guarantees (I suppose), so we can give this internal hint in the hope that the compiler can provide the user with more optimized code.

Suppose that in an unlikely scenario the user is handling std::atomic variables close to a channel<T>::close() call, then the compiler could possibly generate better code.

In the case where this is not unsafe, it's basically free to do and it doesn't introduce significant complexity for the user (it is buried in the lib's code).

tests/channel_test.cpp

@andreiavrammsd

Commits according to @andreiavrammsd code review Co-authored-by: Andrei Avram <[email protected]>

Ran clang format on every file. Fixed typos and redundant functions.

andreiavrammsd · 2025-01-17T14:39:12Z

README.md

@@ -29,7 +30,6 @@ see [CMakeLists.txt](./examples/cmake-project/CMakeLists.txt) from the [CMake pr

 ```c++
 #include <cassert>
-


Please revert.

andreiavrammsd · 2025-01-17T14:39:17Z

README.md

@@ -83,7 +83,6 @@ int main() {

 ```c++
 #include <iostream>
-


Please revert.

andreiavrammsd · 2025-01-17T14:43:47Z

include/msd/channel.hpp

    friend class blocking_iterator<channel>;
 };

 }  // namespace msd

 #include "channel.inl"
-


Please don't resolve conversations. They help to see if we understand one another. The author of the conversation resolves it when the changes are done accordingly.

In this specific case, #include "channel.inl" is needed.

andreiavrammsd · 2025-01-17T14:44:28Z

include/msd/channel.inl

        if (!ch.empty()) {
            out = std::move(ch.queue_.front());
            ch.queue_.pop();
            --ch.size_;
        }
    }
-


Please revert.

andreiavrammsd · 2025-01-17T14:44:33Z

include/msd/channel.inl

    ch.cnd_.notify_one();
-


Please revert.

andreiavrammsd · 2025-01-17T14:47:01Z

include/msd/channel.inl

+    }
+
+    return cnd_.wait_for(lock, timeout, pred);
+}


I will have another look at this.

andreiavrammsd · 2025-01-17T14:51:31Z

README.md

+
+
+```
+


Unnecessary.

andreiavrammsd · 2025-01-17T14:51:39Z

README.md

+
+    return 0;
+}
+


Unnecessary.

dario-loi added 5 commits January 8, 2025 18:38

Implemented API-compatible throwing timeout functionality

9b32652

README.md updated

c149418

Atomic timeout variable for lock-free setting

1f89d8d

basic test suite for timeout feature

892ba14

Removed C++20 attributes

3a82b34

Atomic pre-loading and relaxed ordering

aedccfa

CI/CD bump to GCC12

76775cb

dario-loi force-pushed the timeout branch from 9c27201 to 76775cb Compare January 9, 2025 18:32

andreiavrammsd reviewed Jan 15, 2025

View reviewed changes

dario-loi and others added 4 commits January 16, 2025 18:46

Perfect forwarding

cdf5c50

Commits according to @andreiavrammsd code review Co-authored-by: Andrei Avram <[email protected]>

Resolved issues not related to refactoring

af2b030

Ran clang format on every file. Fixed typos and redundant functions.

Fixed universal reference changes not being consistent

949aca1

Remove impl from channel.hpp

9811914

andreiavrammsd reviewed Jan 17, 2025

View reviewed changes

README.md

```

Copy link

Owner

andreiavrammsd Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary.

andreiavrammsd reviewed Jan 17, 2025

View reviewed changes

README.md

return 0;

}

Copy link

Owner

andreiavrammsd Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[New Feature] Timeout-capable channels #51

[New Feature] Timeout-capable channels #51

dario-loi commented Jan 8, 2025

dario-loi commented Jan 8, 2025

andreiavrammsd commented Jan 8, 2025

dario-loi commented Jan 8, 2025 •

edited

Loading

andreiavrammsd commented Jan 9, 2025

andreiavrammsd commented Jan 9, 2025

andreiavrammsd Jan 13, 2025

dario-loi Jan 16, 2025

andreiavrammsd Jan 17, 2025

andreiavrammsd Jan 13, 2025

dario-loi Jan 16, 2025

andreiavrammsd Jan 17, 2025

andreiavrammsd Jan 15, 2025

dario-loi Jan 16, 2025

andreiavrammsd Jan 15, 2025

dario-loi Jan 16, 2025

andreiavrammsd Jan 17, 2025

andreiavrammsd Jan 15, 2025

dario-loi Jan 16, 2025

andreiavrammsd Jan 17, 2025

andreiavrammsd Jan 17, 2025

andreiavrammsd Jan 17, 2025

andreiavrammsd Jan 17, 2025

andreiavrammsd Jan 17, 2025

andreiavrammsd Jan 17, 2025

andreiavrammsd Jan 17, 2025

andreiavrammsd Jan 17, 2025

		sudo apt-get update
		sudo apt-get install -y gcc-12 g++-12 cmake

		@@ -29,7 +30,6 @@ see [CMakeLists.txt](./examples/cmake-project/CMakeLists.txt) from the [CMake pr

		```c++
		#include <cassert>

[New Feature] Timeout-capable channels #51

Are you sure you want to change the base?

[New Feature] Timeout-capable channels #51

Conversation

dario-loi commented Jan 8, 2025

Features

Compatibility with existing code

Benchmarks

dario-loi commented Jan 8, 2025

andreiavrammsd commented Jan 8, 2025

dario-loi commented Jan 8, 2025 • edited Loading

andreiavrammsd commented Jan 9, 2025

andreiavrammsd commented Jan 9, 2025

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

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

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

dario-loi commented Jan 8, 2025 •

edited

Loading