kvcache-ai/Mooncake

Fork 0

mirror of https://github.com/kvcache-ai/Mooncake.git synced 2026-11-05 01:17:55 +00:00

[GH-ISSUE #2491] [Store] Add shard-grouped BatchPutStart fast path and benchmark shard traversal order #661

New issue

Closed

opened 2026-10-29 21:17:47 +00:00 by bsakaguchi · 1 comment

bsakaguchi commented

2026-10-29 21:17:47 +00:00

Owner

Originally created by @bitborne on GitHub (Jun 16, 2026).
Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/2491

Originally assigned to: @bitborne on GitHub.

Describe your performance question

Background

#2221, #2272, and #2405 showed that Mooncake Store batch metadata paths should avoid behaving like N independent single-key operations.

#2405 optimized BatchExistKey by grouping keys by metadata shard, batching group-route lookup, routing the RPC wrapper to the real batch path, and scanning lookup shards in the opposite direction from BatchEvict.

BatchPutStart still does not have a comparable master-side batch fast path. WrappedMasterService::BatchPutStart currently loops over keys and calls single-key MasterService::PutStart.

Problem

For a batch of N puts, the current implementation effectively performs N independent PutStart operations.

This repeats:

object-operation lock acquisition;
snapshot_mutex_ shared-lock acquisition;
group-route lookup through getMetadataShardIndex(...);
metadata shard write-lock acquisition;
metadata lookup and insertion setup.

When many keys in a batch touch the same shard, this creates avoidable lock churn. It can also contend with lookup, remove, and eviction paths during refill/put-heavy workloads.

Implementation Plan

Add a real MasterService::BatchPutStart path and route WrappedMasterService::BatchPutStart to it.

The batch path should:

validate batch inputs once;
derive per-key config and group id;
compute lookup shard and target shard for each key;
acquire group_routing_mutex_ once while routing the batch, following the #2405 pattern;
group keys by shard and acquire each touched metadata shard write lock once where semantics allow;
acquire object-operation lock stripes in a stable order to avoid deadlocks;
preserve result ordering by writing results back to the original input index;
preserve tenant isolation, group routing, partial success, duplicate-key behavior, and existing error semantics;
reuse AllocateAndInsertMetadata(...) instead of duplicating allocation and insertion logic.

For grouped keys where lookup_shard != target_shard, keep the existing single-key behavior: check and clean up the lookup shard first, then insert/check again under the target shard.

Shard traversal order should be benchmarked, not assumed. #2405 uses reverse shard traversal for lookup because it interacts with BatchEvict's forward scan. BatchPutStart takes write locks, so natural touched-shard order and randomized reverse shard order should be compared before choosing one.

Tests

Follow the test style used in #2405:

./scripts/code_format.sh
cmake --build build --target master_service_test mooncake_master -j$(nproc)
ctest --test-dir build -R '^master_service_test$' --output-on-failure

Suggested unit coverage:

batch result ordering and partial failure, using existing-key failures such as OBJECT_ALREADY_EXISTS;
grouped keys remain queryable/removable through group routing;
tenant-aware isolation, tested by running the same batch under different tenants;
duplicate keys in the same batch do not deadlock or double-create metadata;
wrapper behavior and metrics remain correct after routing to MasterService::BatchPutStart;
existing prefer_alloc_in_same_node behavior remains compatible.

Benchmark

The PR should add or extend a master-side benchmark for BatchPutStart.

Existing benchmarks are not enough to isolate this path:

master_bench measures BatchPutStart + BatchPutEnd and only reports ops/s;
store_kv_bench.py measures end-to-end put, including transfer and PutEnd.

The benchmark should measure BatchPutStart start-only latency and throughput.

Suggested benchmark dimensions:

large batch size;
concurrent batch put/refill workers;
optional concurrent batch lookup workers;
optional eviction-active run;
natural touched-shard order vs randomized reverse shard order;
avg, p50, p90, p99, max latency;
keys/s and requests/s;
error-code counts.

Acceptance Criteria

WrappedMasterService::BatchPutStart uses a real master-side batch path.
Batch routing avoids per-key group_routing_mutex_ acquisition.
Metadata shard write-lock acquisition is reduced from per key to per touched shard where semantics allow.
Object-operation locks are acquired in a deadlock-safe order.
Existing API behavior and result ordering are preserved.
Unit tests cover ordering, partial failure, grouped keys, tenant isolation, duplicate keys, and compatibility behavior.
The PR includes a benchmark that isolates BatchPutStart and compares shard traversal order.

Before submitting a new issue...

Make sure you already searched for relevant issues and read the documentation

Originally created by @bitborne on GitHub (Jun 16, 2026). Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/2491 Originally assigned to: @bitborne on GitHub. ### Describe your performance question ## Background #2221, #2272, and #2405 showed that Mooncake Store batch metadata paths should avoid behaving like N independent single-key operations. #2405 optimized `BatchExistKey` by grouping keys by metadata shard, batching group-route lookup, routing the RPC wrapper to the real batch path, and scanning lookup shards in the opposite direction from `BatchEvict`. `BatchPutStart` still does not have a comparable master-side batch fast path. `WrappedMasterService::BatchPutStart` currently loops over keys and calls single-key `MasterService::PutStart`. ## Problem For a batch of N puts, the current implementation effectively performs N independent `PutStart` operations. This repeats: - object-operation lock acquisition; - `snapshot_mutex_` shared-lock acquisition; - group-route lookup through `getMetadataShardIndex(...)`; - metadata shard write-lock acquisition; - metadata lookup and insertion setup. When many keys in a batch touch the same shard, this creates avoidable lock churn. It can also contend with lookup, remove, and eviction paths during refill/put-heavy workloads. ## Implementation Plan Add a real `MasterService::BatchPutStart` path and route `WrappedMasterService::BatchPutStart` to it. The batch path should: - validate batch inputs once; - derive per-key config and group id; - compute lookup shard and target shard for each key; - acquire `group_routing_mutex_` once while routing the batch, following the #2405 pattern; - group keys by shard and acquire each touched metadata shard write lock once where semantics allow; - acquire object-operation lock stripes in a stable order to avoid deadlocks; - preserve result ordering by writing results back to the original input index; - preserve tenant isolation, group routing, partial success, duplicate-key behavior, and existing error semantics; - reuse `AllocateAndInsertMetadata(...)` instead of duplicating allocation and insertion logic. For grouped keys where `lookup_shard != target_shard`, keep the existing single-key behavior: check and clean up the lookup shard first, then insert/check again under the target shard. Shard traversal order should be benchmarked, not assumed. #2405 uses reverse shard traversal for lookup because it interacts with `BatchEvict`'s forward scan. `BatchPutStart` takes write locks, so natural touched-shard order and randomized reverse shard order should be compared before choosing one. ## Tests Follow the test style used in #2405: - `./scripts/code_format.sh` - `cmake --build build --target master_service_test mooncake_master -j$(nproc)` - `ctest --test-dir build -R '^master_service_test$' --output-on-failure` Suggested unit coverage: - batch result ordering and partial failure, using existing-key failures such as `OBJECT_ALREADY_EXISTS`; - grouped keys remain queryable/removable through group routing; - tenant-aware isolation, tested by running the same batch under different tenants; - duplicate keys in the same batch do not deadlock or double-create metadata; - wrapper behavior and metrics remain correct after routing to `MasterService::BatchPutStart`; - existing `prefer_alloc_in_same_node` behavior remains compatible. ## Benchmark The PR should add or extend a master-side benchmark for `BatchPutStart`. Existing benchmarks are not enough to isolate this path: - `master_bench` measures `BatchPutStart + BatchPutEnd` and only reports ops/s; - `store_kv_bench.py` measures end-to-end put, including transfer and PutEnd. The benchmark should measure `BatchPutStart` start-only latency and throughput. Suggested benchmark dimensions: - large batch size; - concurrent batch put/refill workers; - optional concurrent batch lookup workers; - optional eviction-active run; - natural touched-shard order vs randomized reverse shard order; - avg, p50, p90, p99, max latency; - keys/s and requests/s; - error-code counts. ## Acceptance Criteria - `WrappedMasterService::BatchPutStart` uses a real master-side batch path. - Batch routing avoids per-key `group_routing_mutex_` acquisition. - Metadata shard write-lock acquisition is reduced from per key to per touched shard where semantics allow. - Object-operation locks are acquired in a deadlock-safe order. - Existing API behavior and result ordering are preserved. - Unit tests cover ordering, partial failure, grouped keys, tenant isolation, duplicate keys, and compatibility behavior. - The PR includes a benchmark that isolates `BatchPutStart` and compares shard traversal order. ### Before submitting a new issue... - [ ] Make sure you already searched for relevant issues and read the [documentation](https://kvcache-ai.github.io/Mooncake/)

bsakaguchi closed this issue

2026-10-29 21:17:47 +00:00

bsakaguchi commented

2026-10-29 21:17:48 +00:00

Author

Owner

@github-actions[bot] commented on GitHub (Jun 16, 2026):

Thanks for opening this issue, @bitborne!

Field	Value
Issue	#2491
GitHub user ID	`177341921`
Reporter	@bitborne

A maintainer will triage this when possible. To help us respond faster, please include:

Mooncake version or commit SHA
Environment (OS, CUDA/driver, RDMA stack if relevant)
Steps to reproduce and expected vs. actual behavior

Useful links: Documentation · Contributing guide

This message was posted automatically by the issue bot.

@github-actions[bot] commented on GitHub (Jun 16, 2026): Thanks for opening this issue, @bitborne! | Field | Value | |-------|-------| | **Issue** | #2491 | | **GitHub user ID** | `177341921` | | **Reporter** | @bitborne | A maintainer will triage this when possible. To help us respond faster, please include: - Mooncake version or commit SHA - Environment (OS, CUDA/driver, RDMA stack if relevant) - Steps to reproduce and expected vs. actual behavior Useful links: [Documentation](https://kvcache-ai.github.io/Mooncake/) · [Contributing guide](https://github.com/kvcache-ai/Mooncake/blob/main/CONTRIBUTING.md) > This message was posted automatically by the issue bot.

bsakaguchi referenced this issue

2026-10-29 21:29:50 +00:00

[PR #661] [MERGED] fix(bench): use correct GPU ID in memory registration #1725