rpc: replace `_outstanding` unordered_map with log_indexed_container by emdroid · Pull Request #3450 · scylladb/seastar

emdroid · 2026-06-05T09:16:11Z

std::unordered_map<id_type, unique_ptr<reply_handler_base>> rehashes with a single large contiguous allocation when the number of in-flight requests crosses a bucket threshold. At high concurrency (e.g. 1000 concurrent ALL-consistency reads) this produces >128 KiB allocations, triggering Seastar's large-alloc warning.

Introduce a new generic utility, log_indexed_container<T, IndexType> (include/seastar/util/log_indexed_container.hh), which maps monotonically increasing integer indices to values without hashing or large contiguous allocations:

Backed by boost::container::deque, which releases front blocks on pop_front (unlike std::deque in libstdc++), keeping memory bounded.
Stores values in direct index-offset slots, so lookup is O(1).
Trims empty slots from the front after each removal, keeping the deque short relative to the concurrency window.
Supports any nullable type (raw pointer, unique_ptr) natively, and wraps non-nullable types in std::optional internally.
API: emplace(idx, value), find(idx), take(idx), clear_at(idx), clear(), size(), empty(), base_index().

Replace _outstanding with a log_indexed_container specialised on reply_handler_base unique_ptr. RPC message ids are already monotonically increasing per-connection, making them a natural key. Outstanding reply handlers are removed via take(). The timer and cancellation callbacks preserve the original invariant that the slot always exists when they fire: the receive path disarms both atomically (no yield point) before returning to the reactor, so a missing slot indicates a bug.

Also add tests/unit/log_indexed_container_test.cc with 13 unit tests covering emplace, find, take, clear_at, trim_front, base_index advancement, and a custom index type.

Fixes: SCYLLADB-1541

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR replaces the outstanding-reply handler storage from a hash map to a deque-backed, slot-indexed table to reduce large contiguous allocations during rehash and streamline handler lookup/removal.

Changes:

Introduces reply_table (deque-based) to store in-flight reply handlers by message id.
Updates client cancel/timeout/reply paths to use get()/insert()/remove() instead of unordered_map operations.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File	Description
src/rpc/rpc.cc	Switches outstanding handler operations to the new `reply_table` API.
include/seastar/rpc/rpc.hh	Implements `reply_table` using `std::deque` and replaces the `_outstanding` map.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

gusev-p · 2026-06-05T11:12:11Z

+    // ever exceeds a page regardless of in-flight count.
+    // Note: no_wait verbs consume a message id without registering a slot;
+    // those ids produce an out-of-range lookup on reply, handled gracefully.
+    struct reply_table {


We used the same idea recently here scylladb/scylladb#29794. Maybe extract a template data structure for reuse? The log_indexed_container already has unit tests and also handles the case when insert index is smaller than base_index.

Right, that would make sense, only we'd need to move (copy first) the log_indexed_container to Seastar. Btw. there is also the idea of "chunked map" #2133 - which this might eventually evolve into.

Is this PR urgent? There are some comments in scylladb/scylladb#29794 which I need to address, and IIUC I must do it before we merge this PR. I'm not sure when I'll do it, though, as I have some higher-priority work (mostly reviews) and I'm going on vacation on Thursday.

It's P3 and not marked as CI stability, so not really urgent.

gusev-p · 2026-06-05T11:37:29Z

+
+    private:
+        std::deque<std::unique_ptr<reply_handler_base>> _slots;
+        id_type _base_id = 1; // id corresponding to _slots[0]


id_type _base_id = 1; This seems strange, why not zero?

_message_id starts at 1 in client, so the first id inserted is 1, meaning slot 0 would always be empty if _base_id = 0. Starting at 1 avoids that wasted slot. Which is what Claude inferred when working at the fix - but starting at 0 doesn't make a big difference I guess, or at least comment could be added with the explanation. But especially if we'd create a shared implementation, starting at 0 would be better indeed

gusev-p · 2026-06-05T11:46:24Z

+            // ids are monotonically increasing and always >= _base_id;
+            // a negative index means the caller is violating this invariant.
+            SEASTAR_ASSERT(idx >= 0);
+            while (static_cast<id_type>(_slots.size()) <= idx) {


batch-insert at the end is likely more efficient _slots.insert(_slots.end(), count, nullptr)

gusev-p · 2026-06-05T11:46:47Z

+        }
+
+        // Pop null entries from the front, advancing _base_id.
+        void trim() noexcept {


This can be made private? There can't be null slots at the front after remove-s

gusev-p · 2026-06-05T11:53:51Z

        cancel->cancel_wait = [this, id] {
-            _outstanding[id]->cancel();
-            _outstanding.erase(id);
+            if (auto* h = _outstanding.get(id)) {


why relax the assumptions? previously it was guaranteed that id exists

gusev-p · 2026-06-05T11:54:14Z

    _stats.timeout++;
-    _outstanding[id]->timeout();
-    _outstanding.erase(id);
+    if (auto* h = _outstanding.get(id)) {


why relax the assumptions? previously it was guaranteed that id exists

emdroid · 2026-06-05T18:07:51Z

New version:

copied over the existing log_indexec_container from raft: use log_indexed_container for _awaited_commits/_awaited_applies scylladb#29794, that is to be used in similar case in the scylladb.

std::unordered_map<id_type, unique_ptr<reply_handler_base>> rehashes with a single large contiguous allocation when the number of in-flight requests crosses a bucket threshold. At high concurrency (e.g. 1000 concurrent ALL-consistency reads) this produces >128 KiB allocations, triggering Seastar's large-alloc warning. Introduce a new generic utility, log_indexed_container<T, IndexType> (include/seastar/util/log_indexed_container.hh), which maps monotonically increasing integer indices to values without hashing or large contiguous allocations: - Backed by boost::container::deque, which releases front blocks on pop_front (unlike std::deque in libstdc++), keeping memory bounded. - Stores values in direct index-offset slots, so lookup is O(1). - Trims empty slots from the front after each removal, keeping the deque short relative to the concurrency window. - Supports any nullable type (raw pointer, unique_ptr) natively, and wraps non-nullable types in std::optional internally. - API: emplace(idx, value), find(idx), take(idx), clear_at(idx), clear(), size(), empty(), base_index(). Replace _outstanding with a log_indexed_container specialised on reply_handler_base unique_ptr. RPC message ids are already monotonically increasing per-connection, making them a natural key. Outstanding reply handlers are removed via take(). The timer and cancellation callbacks preserve the original invariant that the slot always exists when they fire: the receive path disarms both atomically (no yield point) before returning to the reactor, so a missing slot indicates a bug. Also add tests/unit/log_indexed_container_test.cc with 13 unit tests covering emplace, find, take, clear_at, trim_front, base_index advancement, and a custom index type. Fixes: SCYLLADB-1541

bitpathfinder · 2026-06-08T10:47:11Z

IMO the changes should be separated. 1st commit should introduce the new container, the second replace the unordered_map.

emdroid requested a review from Copilot June 5, 2026 09:16

emdroid self-assigned this Jun 5, 2026

Copilot AI reviewed Jun 5, 2026

View reviewed changes

emdroid force-pushed the fix/SCYLLADB-1541-rpc-outstanding-deque branch from 372f9f8 to d600495 Compare June 5, 2026 09:55

emdroid requested review from avikivity, bitpathfinder, gleb-cloudius, gusev-p and xemul June 5, 2026 10:05

emdroid added the ai-assisted label Jun 5, 2026

gusev-p reviewed Jun 5, 2026

View reviewed changes

emdroid force-pushed the fix/SCYLLADB-1541-rpc-outstanding-deque branch from d600495 to 23ea09d Compare June 5, 2026 18:04

emdroid changed the title ~~rpc: replace _outstanding unordered_map with slot-indexed deque~~ rpc: replace _outstanding unordered_map with log_indexed_container Jun 5, 2026

emdroid force-pushed the fix/SCYLLADB-1541-rpc-outstanding-deque branch from 23ea09d to 0ffa7da Compare June 5, 2026 18:19

emdroid requested a review from patjed41 June 8, 2026 07:49

bitpathfinder reviewed Jun 8, 2026

View reviewed changes

Conversation

emdroid commented Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emdroid commented Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

emdroid commented Jun 5, 2026 •

edited

Loading

emdroid commented Jun 5, 2026 •

edited

Loading