Reduce EloqKV cold-read handoff overhead

[thweetkomputer]

Context

EloqKV cold reads copied the completed serialized value from ReadClosure into
FetchRecordCc::rec_str_ before BackFill deserialized it into the CCMap
payload. Each new single-flight fetch also constructed a FetchRecordCc inside
an std::unordered_map node and destroyed it when the fetch completed.

For 1–4 KiB values, the value copy adds one full-value memcpy and temporarily
keeps two serialized copies alive. The short-lived fetch object and map node
also add allocator traffic, including the first allocation of its requester
vector.

Behavior before and after

Before this change, a successful EloqKV read copied the serialized value into
FetchRecordCc, and every new active fetch allocated map/object state that was
freed at completion.

After this change:

• the callback transfers ownership of the serialized value into
  FetchRecordCc;
• each shard reuses FetchRecordCc objects from CcRequestPool;
• an absl::flat_hash_map<LruEntry *, FetchRecordCc *> indexes active fetches
  without allocating one node per fetch after the table reaches capacity; and
• FetchRecordCc member reordering reduces sizeof(FetchRecordCc) from 384 to
  368 bytes on the current 64-bit build.

The Redis-visible value, TTL semantics, record timestamp, coalescing behavior,
error handling, range-partition decoding, durability, and recovery behavior are
unchanged.

Allocation accounting

This change does reduce allocations after pool/map warm-up: it reuses the
FetchRecordCc object, its requesters_ capacity, and the active-fetch hash
table storage instead of allocating an unordered_map node containing a new
object for each cold fetch.

It does not eliminate the local serialized-value allocation. Moving
ReadClosure::value_ into rec_str_ gives up the closure's reusable buffer, so
a later local EloqStore read allocates a new buffer when filling that closure.
The local-path improvement is one fewer full-value memcpy and one fewer
simultaneously live serialized copy, not one fewer steady-state value-buffer
allocation. BackFill also still deserializes into the final CCMap payload, so
that final allocation/copy remains.

For remote reads, cleanup already releases the protobuf response storage.
Moving its string also avoids the separate destination allocation previously
performed by rec_str_.assign(...).

Implementation

• Add ReadClosure::TakeValue() as the explicit consuming API for local and
  RPC-backed reads.
• Use it only in the EloqKV hash-partition success path; range-partition decoding
  retains the existing string_view flow.
• Make FetchRecordCc resettable and pool it per CcShard.
• Keep pooled object addresses stable while asynchronous data-store callbacks
  are in flight; the flat map is only a non-owning active-fetch index.
• Reset request state between uses while retaining reusable requester/string
  capacity. Archive result storage is released to avoid retaining an
  unexpectedly large vector indefinitely.
• Read the clock only when a record has a non-zero TTL.
• Group naturally aligned members to remove 16 bytes of padding per object.

Concurrency and lifetime

The pool and active-fetch map are owned by the same CcShard. A request never
migrates between shards, and Reset() asserts that invariant. Removing a fetch
erases the map entry before returning the request to the pool. Although Free()
can run while FetchRecordCc::Execute() is unwinding, new fetches and resumed
requesters execute serially on the owning shard, so NextRequest() cannot reuse
that object until control returns to the shard loop.

Reopen operations retain the same active request and do not return it to the
pool. The data-store callback has completed before the request is removed, so no
callback can retain a pointer to a freed/reused request.

Test plan

• Unit/CTest coverage
• Standalone data_substrate build
• Parent-project integration and manual validation
• Formatting/build checks
• Performance validation
• Documentation reviewed; no externally visible behavior changed

Commands and results:

clang-format-18 --dry-run --Werror <changed C++ files>
PASS

git diff --check origin/main...HEAD
PASS

cmake -S . -B bld-pr -DCMAKE_BUILD_TYPE=Release \
  -DELOQ_THIRD_PARTY_PREFIX=/mnt/dev/eloqkv/data_substrate/third_party/install \
  -DWITH_DATA_STORE=ELOQDSS_ELOQSTORE -DWITH_LOG_SERVICE=ON
cmake --build bld-pr --parallel 16
PASS (data_substrate target and dependencies)

cmake --build /mnt/dev/eloqkv/bld --parallel 16
cmake --install /mnt/dev/eloqkv/bld
PASS (parent EloqKV Release build/install containing the change)

ctest --test-dir bld-pr --output-on-failure
No tests were found in this build configuration.

Cold-read comparison with the same 16-core EloqKV / 16-core memtier setup and
1–4 KiB values:

before pool: 248,744.50 QPS, 0.32160 ms average,
             p99.9 0.671 ms, p99.99 3.119 ms
after pool:  248,845.50 QPS, 0.32144 ms average,
             p99.9 0.679 ms, p99.99 3.295 ms

The approximately +0.04% QPS difference is within run-to-run noise; this test
does not demonstrate a measurable performance gain from request pooling. It
does show that these control-object allocations are not the dominant bottleneck
in the tested workload. The ownership move remains useful because it removes a
full-value copy.

No standalone TCL suite or dedicated remote/range/reopen/failover test was run.

Risk assessment

The main regression surface is pooled request lifetime and reset completeness.
The pool keeps a small baseline of eight FetchRecordCc objects per shard and
can retain requester/string capacities reached by prior requests. Large archive
vectors are explicitly released. The flat map reserves 64 active-fetch slots
per shard to avoid hot-path rehashing.

FetchCc now stores its shard as a pointer so pooled instances can be default
constructed; existing non-pooled subclasses continue to initialize it through
the same constructor. This is an internal in-process layout change with no wire,
storage-format, locking, or durability impact.

Rollback plan

Revert this PR. The first restores copying the read result;
the second restores per-fetch construction in std::unordered_map.

Reviewer guide

1. Review FetchRecordCallback and ReadClosure::TakeValue() for the ownership
   transfer and local/remote allocation distinction.
2. Review CcShard::FetchRecord() and RemoveFetchRecordRequest() for
   single-flight indexing, pool lifetime, and same-shard serialization.
3. Review FetchRecordCc::Reset() for complete state reset across retries,
   reopen, archive reads, and errors.

Follow-up work

Reducing the remaining 1–4 KiB local value-buffer allocation requires a broader
handoff design, such as filling a reusable consumer-owned buffer or pooling the
serialized buffer separately across the asynchronous fetch/backfill lifetime.

Summary by CodeRabbit

• Performance
  • Improved record retrieval efficiency by reducing unnecessary value copying.
  • Optimized fetch request allocation and cleanup for smoother processing under load.
• Reliability
  • Improved handling of record expiration and fetch request state resets.
  • Added safer initialization and lifecycle management for fetch operations.

[coderabbitai]

Walkthrough

FetchRecordCc requests now use pooled allocation and reset-based reuse. CcShard stores stable request pointers and releases them explicitly. ReadClosure transfers fetched values by move, and EloqKV TTL checks remain branch-local.

Changes

Fetch request pooling and value transfer

Layer / File(s)	Summary
Reusable fetch request contract tx_service/include/cc/cc_req_misc.h, tx_service/src/cc/cc_req_misc.cpp	FetchRecordCc gains default initialization and Reset. The reset restores request state and validates shard affinity. Fetch completion handlers use nullable shard pointers.
Pooled request lifecycle tx_service/include/cc/cc_shard.h, tx_service/src/cc/cc_shard.cpp	CcShard stores FetchRecordCc pointers in the request index, obtains pooled requests for new fetches, reuses existing entries, and returns removed requests to the pool.
Move-based read value transfer store_handler/data_store_service_client_closure.h, store_handler/data_store_service_client_closure.cpp	ReadClosure::TakeValue() moves local or remote values. FetchRecordCallback uses the moved value for non-range records and keeps range decoding separate. EloqKV TTL evaluation uses branch-local values.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• eloqdata/tx_service#484: Modifies the same fetch request and read closure classes for reopen propagation and buffered-command partition reopening.

Suggested reviewers: liunyl, liangjchen

Poem

A rabbit resets each pooled request,
Then sends old buffers back to rest.
Read values move without delay,
TTL checks guide the record’s way.
Stable pointers keep the path clear.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly identifies the primary change: reducing EloqKV cold-read handoff overhead.
Description check	✅ Passed	The description covers the required context, behavior, implementation, testing, risks, rollback, review, and follow-up; only the design-decisions heading is implicit.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch agent/move-fetch-record-value

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

tx_service/src/cc/cc_req_misc.cpp (1)

793-864: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consolidate FetchRecordCc initialization into Reset().

The parameterized constructor duplicates the field-assignment path that pooled requests now use through FetchRecordCc() + Reset(). Remove the constructor if it has no call sites, or delegate it to default construction plus Reset() so field initialization has a single source of truth.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tx_service/src/cc/cc_req_misc.cpp` around lines 793 - 864, Consolidate
FetchRecordCc initialization by removing the parameterized constructor if it has
no call sites; otherwise make it delegate to default construction and Reset().
Keep Reset() as the single source of truth for all field assignments, including
shard ownership and request state.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tx_service/include/cc/cc_req_misc.h`:
- Around line 88-94: Document the default-construction and reset invariants in
the FetchCc and FetchRecordCc declarations: state that default-constructed
instances have a null ccs_, must not invoke methods that dereference it until
Reset() completes, and that Reset() binds the request to the resetting shard
without migration. Add concise comments at both default constructors and the
Reset() declaration, covering the pool-reuse contract without changing behavior.

---

Nitpick comments:
In `@tx_service/src/cc/cc_req_misc.cpp`:
- Around line 793-864: Consolidate FetchRecordCc initialization by removing the
parameterized constructor if it has no call sites; otherwise make it delegate to
default construction and Reset(). Keep Reset() as the single source of truth for
all field assignments, including shard ownership and request state.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 334b56e7-ee64-4f2b-8fc3-4834a802fdc3

📥 Commits

Reviewing files that changed from the base of the PR and between bd47861 and 58be39c.

📒 Files selected for processing (5)

• store_handler/data_store_service_client_closure.cpp
• tx_service/include/cc/cc_req_misc.h
• tx_service/include/cc/cc_shard.h
• tx_service/src/cc/cc_req_misc.cpp
• tx_service/src/cc/cc_shard.cpp

🚧 Files skipped from review as they are similar to previous changes (1)

• store_handler/data_store_service_client_closure.cpp

[liangjchen]

Any reason you made this change? the comment itself does not justify the reason. std::unordered_map provides pointer stability.

[thweetkomputer]

to reuse FetchRecordCc.