From 8d002659987cd45cb77896fbb173159398138e73 Mon Sep 17 00:00:00 2001 From: MerryMage Date: Sat, 6 Feb 2021 19:13:03 +0000 Subject: [PATCH] ring_buffer: Remove granularity template argument Non-obvious bug in RingBuffer::Push(std::vector&) when granularity != 1 Just remove it altogether because we do not have a use for granularity != 1 --- src/common/ring_buffer.h | 21 ++++++++++----------- src/tests/common/ring_buffer.cpp | 10 +++++----- 2 files changed, 15 insertions(+), 16 deletions(-) diff --git a/src/common/ring_buffer.h b/src/common/ring_buffer.h index 138fa0131..4a8d09806 100644 --- a/src/common/ring_buffer.h +++ b/src/common/ring_buffer.h @@ -19,15 +19,14 @@ namespace Common { /// SPSC ring buffer /// @tparam T Element type /// @tparam capacity Number of slots in ring buffer -/// @tparam granularity Slot size in terms of number of elements -template +template class RingBuffer { - /// A "slot" is made of `granularity` elements of `T`. - static constexpr std::size_t slot_size = granularity * sizeof(T); + /// A "slot" is made of a single `T`. + static constexpr std::size_t slot_size = sizeof(T); // T must be safely memcpy-able and have a trivial default constructor. static_assert(std::is_trivial_v); // Ensure capacity is sensible. - static_assert(capacity < std::numeric_limits::max() / 2 / granularity); + static_assert(capacity < std::numeric_limits::max() / 2); static_assert((capacity & (capacity - 1)) == 0, "capacity must be a power of two"); // Ensure lock-free. static_assert(std::atomic_size_t::is_always_lock_free); @@ -47,7 +46,7 @@ public: const std::size_t second_copy = push_count - first_copy; const char* in = static_cast(new_slots); - std::memcpy(m_data.data() + pos * granularity, in, first_copy * slot_size); + std::memcpy(m_data.data() + pos, in, first_copy * slot_size); in += first_copy * slot_size; std::memcpy(m_data.data(), in, second_copy * slot_size); @@ -74,7 +73,7 @@ public: const std::size_t second_copy = pop_count - first_copy; char* out = static_cast(output); - std::memcpy(out, m_data.data() + pos * granularity, first_copy * slot_size); + std::memcpy(out, m_data.data() + pos, first_copy * slot_size); out += first_copy * slot_size; std::memcpy(out, m_data.data(), second_copy * slot_size); @@ -84,9 +83,9 @@ public: } std::vector Pop(std::size_t max_slots = ~std::size_t(0)) { - std::vector out(std::min(max_slots, capacity) * granularity); - const std::size_t count = Pop(out.data(), out.size() / granularity); - out.resize(count * granularity); + std::vector out(std::min(max_slots, capacity)); + const std::size_t count = Pop(out.data(), out.size()); + out.resize(count); return out; } @@ -113,7 +112,7 @@ private: alignas(128) std::atomic_size_t m_write_index{0}; #endif - std::array m_data; + std::array m_data; }; } // namespace Common diff --git a/src/tests/common/ring_buffer.cpp b/src/tests/common/ring_buffer.cpp index 54def22da..903626e4b 100644 --- a/src/tests/common/ring_buffer.cpp +++ b/src/tests/common/ring_buffer.cpp @@ -14,7 +14,7 @@ namespace Common { TEST_CASE("RingBuffer: Basic Tests", "[common]") { - RingBuffer buf; + RingBuffer buf; // Pushing values into a ring buffer with space should succeed. for (std::size_t i = 0; i < 4; i++) { @@ -77,7 +77,7 @@ TEST_CASE("RingBuffer: Basic Tests", "[common]") { } TEST_CASE("RingBuffer: Threaded Test", "[common]") { - RingBuffer buf; + RingBuffer buf; const char seed = 42; const std::size_t count = 1000000; std::size_t full = 0; @@ -92,8 +92,8 @@ TEST_CASE("RingBuffer: Threaded Test", "[common]") { std::array value = {seed, seed}; std::size_t i = 0; while (i < count) { - if (const std::size_t c = buf.Push(&value[0], 1); c > 0) { - REQUIRE(c == 1U); + if (const std::size_t c = buf.Push(&value[0], 2); c > 0) { + REQUIRE(c == 2U); i++; next_value(value); } else { @@ -107,7 +107,7 @@ TEST_CASE("RingBuffer: Threaded Test", "[common]") { std::array value = {seed, seed}; std::size_t i = 0; while (i < count) { - if (const std::vector v = buf.Pop(1); v.size() > 0) { + if (const std::vector v = buf.Pop(2); v.size() > 0) { REQUIRE(v.size() == 2U); REQUIRE(v[0] == value[0]); REQUIRE(v[1] == value[1]);