refactor(util): replace lock-free SPSCQueue with mutex-guarded ring buffer#145
Merged
refactor(util): replace lock-free SPSCQueue with mutex-guarded ring buffer#145
Conversation
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan Review Summary by QodoReplace lock-free SPSCQueue with mutex-guarded ring buffer
WalkthroughsDescription• Replace lock-free atomic operations with mutex-based synchronization • Simplify capacity requirements from power-of-2 to any positive integer • Use std::vector<std::optional<T>> for automatic memory management • Update tests to reflect new capacity flexibility and remove power-of-2 constraints Diagramflowchart LR
A["Lock-free atomics<br/>Power-of-2 capacity"] -->|Refactor| B["Mutex-based sync<br/>Any capacity"]
C["Manual memory<br/>management"] -->|Simplify| D["std::vector<br/>std::optional"]
E["Bitwise AND<br/>modulo"] -->|Replace| F["Modulo operator<br/>%"]
File Changes1. src/util/queues.hpp
|
Code Review by Qodo
1. Tests missing required includes
|
📝 WalkthroughWalkthroughThe SPSCQueue implementation was refactored from a lock-free atomic-based ring buffer to a mutex-guarded queue using Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/util/queues.hpp (3)
17-22: Capacity validation occurs after partial initialization.The check
if (capacity == 0)runs afterm_buffer(capacity)is already initialized in the member initializer list. While this works correctly (a zero-size vector is valid and will be cleaned up when the exception propagates), validating before initialization is cleaner.♻️ Optional: Use a helper to validate early
+private: + static size_t validate_capacity(size_t cap) { + if (cap == 0) { + throw std::invalid_argument("SPSCQueue capacity must be > 0"); + } + return cap; + } + +public: explicit SPSCQueue(size_t capacity) - : m_capacity(capacity), m_buffer(capacity), m_head(0), m_tail(0), m_size(0) { - if (capacity == 0) { - throw std::invalid_argument("SPSCQueue capacity must be > 0"); - } + : m_capacity(validate_capacity(capacity)), m_buffer(m_capacity), m_head(0), m_tail(0), m_size(0) { }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util/queues.hpp` around lines 17 - 22, Validate the capacity before any member initializers run in the SPSCQueue constructor to avoid partially initializing members (m_buffer, m_capacity, m_head, m_tail, m_size) when capacity is invalid; move the check for capacity == 0 out of the member initializer list so it executes first (or introduce a small static/constexpr validate_capacity(size_t) helper called in the initializer) and throw std::invalid_argument if zero, then initialize m_buffer(capacity) and the other members normally in the constructor initializer list.
11-15: Class name and documentation no longer reflect implementation.The class is named
SPSCQueue(Single-Producer, Single-Consumer) and documented as such, but the mutex-based implementation is now thread-safe for arbitrary numbers of producers and consumers. This is misleading since:
- Users may expect lock-free SPSC performance characteristics
- The name doesn't convey the actual thread-safety guarantees
Consider renaming to
BoundedQueueorThreadSafeQueue, or updating the documentation to clarify the mutex-based implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util/queues.hpp` around lines 11 - 15, The class name SPSCQueue and its documentation are misleading because the implementation uses a mutex and supports multiple producers/consumers; rename the class to a name that reflects its behavior (e.g., BoundedQueue or ThreadSafeQueue) and update the doc comment to state it is a bounded, mutex-protected queue supporting multiple producers and consumers and that capacity must be > 0 (construction throws std::invalid_argument). Update all references/usages of SPSCQueue in the codebase (constructors, type aliases, tests, and template instantiations) to the new class name and adjust any comments mentioning “single-producer single-consumer” to describe the actual thread-safety and performance characteristics.
65-73: Consider whethersize()andempty()need locking.Both methods acquire the mutex for reading
m_size. This is correct for consistency, but note that the returned value may be stale immediately after the lock is released. If callers use these for decision-making (e.g., "if not empty, pop"), they should usetry_pop()directly instead.If these methods are only used for diagnostics/debugging and don't need strong consistency, making
m_sizeatomic and removing the lock here could reduce contention.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util/queues.hpp` around lines 65 - 73, size() and empty() currently lock m_mutex to read m_size which is safe but can cause contention and still produce stale results after the lock is released; decide whether you need strong consistency for callers and either (A) keep the mutex usage and add documentation on using try_pop() for decision-making (e.g., "if not empty, pop" race), or (B) make m_size an std::atomic<size_t> and remove the locking in size() and empty() to reduce contention for diagnostic-only uses; update the implementation of size(), empty(), and any comments accordingly and reference the members/functions m_size, m_mutex, size(), empty(), and try_pop() when applying the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/util/queues.hpp`:
- Around line 17-22: Validate the capacity before any member initializers run in
the SPSCQueue constructor to avoid partially initializing members (m_buffer,
m_capacity, m_head, m_tail, m_size) when capacity is invalid; move the check for
capacity == 0 out of the member initializer list so it executes first (or
introduce a small static/constexpr validate_capacity(size_t) helper called in
the initializer) and throw std::invalid_argument if zero, then initialize
m_buffer(capacity) and the other members normally in the constructor initializer
list.
- Around line 11-15: The class name SPSCQueue and its documentation are
misleading because the implementation uses a mutex and supports multiple
producers/consumers; rename the class to a name that reflects its behavior
(e.g., BoundedQueue or ThreadSafeQueue) and update the doc comment to state it
is a bounded, mutex-protected queue supporting multiple producers and consumers
and that capacity must be > 0 (construction throws std::invalid_argument).
Update all references/usages of SPSCQueue in the codebase (constructors, type
aliases, tests, and template instantiations) to the new class name and adjust
any comments mentioning “single-producer single-consumer” to describe the actual
thread-safety and performance characteristics.
- Around line 65-73: size() and empty() currently lock m_mutex to read m_size
which is safe but can cause contention and still produce stale results after the
lock is released; decide whether you need strong consistency for callers and
either (A) keep the mutex usage and add documentation on using try_pop() for
decision-making (e.g., "if not empty, pop" race), or (B) make m_size an
std::atomic<size_t> and remove the locking in size() and empty() to reduce
contention for diagnostic-only uses; update the implementation of size(),
empty(), and any comments accordingly and reference the members/functions
m_size, m_mutex, size(), empty(), and try_pop() when applying the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 836afc08-12b5-4c10-a9fb-af78dfa5957f
📒 Files selected for processing (2)
src/util/queues.hpptests/util/test_queues.cpp
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
3
to
+7
| #include <cstddef> | ||
| #include <cstdlib> | ||
| #include <memory> | ||
| #include <mutex> | ||
| #include <optional> | ||
| #include <stdexcept> | ||
| #include <type_traits> | ||
| #include <vector> |
There was a problem hiding this comment.
1. Tests missing required includes 🐞 Bug ✓ Correctness
tests/util/test_queues.cpp uses std::unique_ptr/std::make_unique and std::string but does not include <memory> or <string>; this likely compiled only because queues.hpp previously included <memory>. After this PR removes those transitive includes, the test can fail to compile depending on the standard library/Catch2 headers.
Agent Prompt
### Issue description
`tests/util/test_queues.cpp` uses `std::unique_ptr`, `std::make_unique`, and `std::string` but does not include `<memory>` / `<string>`. This likely became visible after `queues.hpp` stopped including `<memory>`.
### Issue Context
This is a compile portability issue caused by relying on transitive includes.
### Fix Focus Areas
- tests/util/test_queues.cpp[1-10]
- Add `#include <memory>` and `#include <string>` (and any other directly-used standard headers not already included).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
42
to
+49
| auto try_push(T&& item) -> bool { | ||
| const size_t current_head = m_head.load(std::memory_order_relaxed); | ||
| const size_t current_tail = m_tail.load(std::memory_order_acquire); | ||
| const size_t current_size = (current_head - current_tail) & m_capacity_mask; | ||
| if (current_size >= m_capacity) { | ||
| std::lock_guard lock(m_mutex); | ||
| if (m_size >= m_capacity) { | ||
| return false; | ||
| } | ||
|
|
||
| new (&m_buffer[current_head]) T(std::move(item)); | ||
| const size_t next_head = (current_head + 1) & m_capacity_mask; | ||
| m_head.store(next_head, std::memory_order_release); | ||
| m_buffer[m_head].emplace(std::move(item)); | ||
| m_head = (m_head + 1) % m_capacity; | ||
| ++m_size; |
There was a problem hiding this comment.
2. Missing include 🐞 Bug ✓ Correctness
src/util/queues.hpp uses std::move but does not include <utility>, making the header non-self-contained and potentially failing to compile on some toolchains/stdlib implementations.
Agent Prompt
### Issue description
`src/util/queues.hpp` uses `std::move` but does not include `<utility>`, so the header is not self-contained.
### Issue Context
Relying on indirect includes (e.g., through `<optional>` or `<vector>`) is not guaranteed by the standard and can break builds across platforms/stdlibs.
### Fix Focus Areas
- src/util/queues.hpp[1-10]
- Add `#include <utility>`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Refactor