src: add batched support callback in AsyncTSQueue #312
Conversation
WalkthroughThe AsyncTSQueue class was refactored to support both batch and single-item processing callbacks using compile-time detection, replacing the previous single-item-only callback mechanism. The constructor and process method were updated accordingly, and new tests were added to verify batch callback behavior with various signatures and argument types. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AsyncTSQueue
participant Callback
User->>AsyncTSQueue: enqueue(items)
User->>AsyncTSQueue: process()
alt Batch callback detected
AsyncTSQueue->>Callback: operator()(std::vector<T>&& batch[, extra_args])
else Single-item callback detected
loop for each item
AsyncTSQueue->>Callback: operator()(T&& item[, extra_args])
end
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
9455b8d to
7ca4eed
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/nsolid/async_ts_queue.h (1)
87-99: Trait set missesconst std::vector<T>&&andconst T&&casesAlthough unlikely in practice, users could legitimately provide callbacks that take
const std::vector<T>&&orconst T&&. Those signatures currently fail SFINAE detection and trigger the static-assert even though they are perfectly sound.- std::is_invocable<Cb, std::vector<T>&&, Extra...>, - std::is_invocable<Cb, const std::vector<T>&, Extra...> + std::is_invocable<Cb, std::vector<T>&&, Extra...>, + std::is_invocable<Cb, const std::vector<T>&, Extra...>, + std::is_invocable<Cb, const std::vector<T>&&, Extra...>and likewise for the
is_single_callbackbranch.Adding the missing qualifiers costs virtually nothing and avoids surprising compile-time errors for users that choose to keep immutability even when moving.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/nsolid/async_ts_queue.h(3 hunks)test/cctest/test_nsolid_async_ts_queue.cc(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/cctest/test_nsolid_async_ts_queue.cc (1)
src/nsolid/async_ts_queue.h (6)
create(39-44)create(39-39)item(59-63)item(59-59)item(71-75)item(71-71)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: test-macOS
- GitHub Check: coverage-linux
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: coverage-linux-without-intl
- GitHub Check: build-tarball
- GitHub Check: coverage-windows
- GitHub Check: lint-js-and-md
- GitHub Check: build-docs
🔇 Additional comments (3)
test/cctest/test_nsolid_async_ts_queue.cc (3)
237-256: Great coverage forstd::vector<T>&&batch pathThe test validates rvalue batch handling and ensures the callback is invoked only once—exactly the edge that could regress.
258-276: 👍 Extra-arg batch test adds real-world scenarioVery useful to demonstrate that forwarding of additional bound parameters still works with batch mode.
279-295: Const-reference batch variant coveredThe const-ref path is less common but important for read-only consumers. Test looks solid.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/nsolid/async_ts_queue.h (2)
109-112: Perfect-forward the extra arguments inside the bound lambda
argsare captured by value; when they are later passed tostd::invokethey are always forwarded as l-values, which can:
- Prevent move-only parameters from being moved into the callback.
- Produce unnecessary copies for heavy objects.
Forward them again to preserve their value category:
- std::invoke(cb, std::forward<decltype(first)>(first), args...); + std::invoke(cb, + std::forward<decltype(first)>(first), + std::forward<Args>(args)...);(You’ll need to add
Argsto the lambda’s template parameter list, e.g. turn it into a generic lambda withtemplate<typename... Args2>or wrap the extras in a tuple andstd::apply.)
115-125: Reserve vector capacity to avoid repeated reallocationsWhen batching, the vector grows one element at a time. If thousands of items are pending, that leads to O(n) reallocations.
After the first successful dequeue you know the queue’s approximate size; pre-allocate to at least that:
- std::vector<T> batch; - batch.push_back(std::move(item)); + std::vector<T> batch; + batch.reserve(queue_.approx_size() + 1); // or a sensible heuristic + batch.push_back(std::move(item));(Assumes
TSQueueexposes anapprox_size()/size()getter; if not, consider adding one.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/nsolid/async_ts_queue.h(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: lint-js-and-md
- GitHub Check: coverage-linux
- GitHub Check: build-docs
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: coverage-windows
- GitHub Check: test-macOS
- GitHub Check: coverage-linux-without-intl
- GitHub Check: build-tarball
Enable AsyncTSQueue to accept both single-element (T&&, const T&) and batch (std::vector<T>&&, const std::vector<T>&) callbacks, using if constexpr with type traits for compile-time dispatch. Ensure correct argument forwarding and update tests to cover all supported callback signatures.
02c9003 to
9cc7a5d
Compare
Enable AsyncTSQueue to accept both single-element (T&&, const T&) and batch (std::vector<T>&&, const std::vector<T>&) callbacks, using if constexpr with type traits for compile-time dispatch. Ensure correct argument forwarding and update tests to cover all supported callback signatures. PR-URL: #312 Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
|
Landed in 4ba885f |
Enable AsyncTSQueue to accept both single-element (T&&, const T&) and batch (std::vector<T>&&, const std::vector<T>&) callbacks, using if constexpr with type traits for compile-time dispatch. Ensure correct argument forwarding and update tests to cover all supported callback signatures. PR-URL: #312 Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Enable AsyncTSQueue to accept both single-element (T&&, const T&) and batch (std::vector<T>&&, const std::vector<T>&) callbacks, using if constexpr with type traits for compile-time dispatch. Ensure correct argument forwarding and update tests to cover all supported callback signatures. PR-URL: #312 Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> PR-URL: #359 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Enable AsyncTSQueue to accept both single-element (T&&, const T&) and batch (std::vector&&, const std::vector&) callbacks, using SFINAE traits for compile-time detection and dispatch. Ensure correct argument forwarding and update tests to cover all supported callback signatures.
Summary by CodeRabbit