Skip to content

[Store] Remove redundant alignment headroom from DFS shard allocation - #4753

Merged
ykwd merged 2 commits into
kvcache-ai:mainfrom
2xdevv:perf/dfs-shard-alignment-headroom
Oct 20, 2026
Merged

ykwd merged 2 commits into
kvcache-ai:mainfrom
2xdevv:perf/dfs-shard-alignment-headroom

Conversation

@2xdevv

@2xdevv 2xdevv commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Description

ShardAllocator::Allocate requests aligned_size + alignment - 1 bytes from the shard's OffsetAllocator and then rounds the returned offset up to the alignment. That slack is only needed when the allocator can return unaligned offsets, and here it can't:

  • every shard allocator is created at base 0;
  • every request is a multiple of the power-of-two alignment;
  • size-class rounding of a multiple of the alignment stays a multiple of it, so each split leaves the remainder at an aligned offset, and merging on free only joins aligned boundaries.

So the returned offset is always aligned, and the extra alignment - 1 bytes are never used. They do cost space, though: when the aligned size is exactly a size class, the extra bytes push the request into the next class.

This PR requests aligned_size directly. Since there is no longer slack to round into, the offset rounding is replaced by a check that fails the allocation with INTERNAL_ERROR if the offset is ever unaligned. Normalized reservation accounting (reserved_bytes, used bytes, deferred frees, eviction) is unchanged. Only the shard (POSIX/HF3FS) path is affected; ImmutableBucketAllocator and DSB object mode are not.

Objects per shard, 4 KiB alignment, measured by filling the unchanged OffsetAllocator:

Payload Shard Before After Reserved before → after
4 KiB 64 MiB 8192 16384 8 KiB → 4 KiB
192 KiB 64 MiB 315 341 208 KiB → 192 KiB
1 MiB 32 MiB 28 32 1.125 MiB → 1 MiB
2 MiB 32 MiB 14 16 2.25 MiB → 2 MiB
16 MiB 1 GiB 56 64 18 MiB → 16 MiB
32 MiB 1 GiB 28 32 36 MiB → 32 MiB
16.5 MiB (off-class) 1 GiB 56 56 18 MiB → 18 MiB

Payloads whose aligned size is exactly a size class, which includes powers of two, save about 11–12%. Other sizes reserve the same as before. An object whose aligned size equals the shard capacity now fits; it was previously rejected. This is a capacity gain only; I have not measured serving latency or hit rate.

Interaction with #4018 (draft): its recovery validation requires requested_size == aligned_size + alignment - 1, so it would reject allocations made after this change. That check would need to accept requested_size == aligned_size. Layouts produced by the old code are also aligned, so recovering a mix of old and new records is safe. #3787 (finer size classes) is independent: it narrows the class steps, but on-class sizes still cross into the next class with the old headroom.

Suggested Merge Priority

  • P0 — Urgent: Critical security vulnerability, data loss/corruption, or widespread production outage requiring immediate action
  • P1 — High: Significant correctness or stability issue in existing functionality, or a regression blocking users without a practical workaround
  • P2 — Normal: Routine bug fix, validated performance/compatibility improvement, or new feature for a confirmed use case
  • P3 — Low: Experimental feature, optional enhancement, refactoring, or cleanup with no urgent user impact

Module

  • Transfer Engine (mooncake-transfer-engine)
  • Mooncake Store (mooncake-store)
  • Mooncake Conductor (mooncake-conductor)
  • Reshard (mooncake-reshard)
  • Mooncake EP (mooncake-ep)
  • Mooncake PG (mooncake-pg)
  • Integration (mooncake-integration)
  • P2P Store (mooncake-p2p-store)
  • Python Wheel (mooncake-wheel)
  • Common (mooncake-common)
  • Mooncake RL (mooncake-rl)
  • CI/CD
  • Docs
  • Other

Type of Change

  • Bug fix
  • New feature
  • Refactor
  • Breaking change
  • Documentation update
  • Performance improvement
  • Other

How Has This Been Tested?

New tests:

  • ShardAllocatorTest.MegabytePayloadsPackWithoutAlignmentHeadroom fills a 32 MiB shard with 1–8 MiB payloads, checks count, back-to-back offsets and used bytes, then writes a distinct pattern into every object through the POSIX backend and reads each one back.
  • ShardAllocatorTest.MixedMegabytePayloadsStayAlignedAcrossReuse mixes odd MB sizes at 4 KiB, 64 KiB and 1 MiB alignment over repeated free/refill rounds, checking alignment and no overlap.
  • ShardAllocatorTest.AllocateReservesOnlyAlignedSize replaces AllocateReservesAlignmentPadding, which asserted the old padding.
  • Hf3fsAdapterTest.ShardAllocatorPacksMegabytePayloads does the same MB packing and readback through the native 3FS I/O path.

The first three fail on the original code; for example, 1 MiB objects land 1.125 MiB apart.

Tests that relied on a 100-byte object reserving 8 KiB to fill a small shard or cross eviction watermarks now use 8 KiB objects, so they check the same behavior as before.

Test commands:

cmake -B build -DUSE_3FS=ON -DUSE_HTTP=ON -DBUILD_UNIT_TESTS=ON
make -C build -j dfs_posix_test dfs_hf3fs_test offset_allocator_test \
  master_service_dfs_scenario_test master_service_test master_admin_server_test \
  dfs_sync_client_test distributed_storage_config_test pybind_client_test
./build/mooncake-store/tests/dfs_posix_test
MOONCAKE_DFS_ROOT_DIR=/mnt/3fs/<writable-dir> ./build/mooncake-store/tests/dfs_hf3fs_test
# ...and the other targets above

Test results:

  • Unit tests pass: dfs_posix_test (55), offset_allocator_test (56), master_service_dfs_scenario_test (6), master_service_test (69), master_admin_server_test (63), dfs_sync_client_test (12), distributed_storage_config_test (10), pybind_client_test DFS case (1)
  • Integration tests pass (if applicable): dfs_hf3fs_test (5) on a local single-node 3FS cluster
  • Manual testing done (describe below)

Checklist

  • I have performed a self-review of my own code
  • I have formatted my code using ./scripts/code_format.sh
  • I have run pre-commit on the files changed in this PR and all hooks pass
  • I have updated the documentation (if applicable)
  • I have added tests to prove my changes are effective
  • For changes >500 LOC: I have filed an RFC issue

AI Assistance Disclosure

  • No AI tools were used
  • AI tools were used (specify below)

Claude Code helped verify the alignment invariant against the allocator code, implement the change and tests, and run the test suites locally and on 3FS. I have reviewed every changed line.

Shard allocators start at offset 0 and only get alignment-multiple
requests, so returned offsets are already aligned. The extra
alignment - 1 bytes only pushed on-class sizes into the next size
class (e.g. 1 MiB reserved 1.125 MiB). Request the aligned size and
check the offset instead of rounding it.

@ykwd ykwd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I accidentally clicked Approve by mistake. Will initiate the review process as soon as possible.

@ykwd
ykwd requested a review from fcczzz October 19, 2026 12:08

@fcczzz fcczzz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you update the DFS deployment guide to match this change? It still says shard allocations require an additional MOONCAKE_DFS_ALIGNMENT - 1 bytes of padding, including in the limitations section.

Please clarify that shard allocations no longer need leading alignment padding because they start at offset 0 and preserve alignment through size-class rounding, splitting, and coalescing. Size-class rounding can still reserve more than aligned_size.

@github-actions github-actions Bot added the documentation Improvements or additions to documentation label Oct 20, 2026
@2xdevv

2xdevv commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

Hello @fcczzz. Just updated the DFS deployment guide with the things you've mentioned.

@fcczzz fcczzz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ykwd
ykwd merged commit 7d07515 into kvcache-ai:main Oct 20, 2026
27 checks passed
@2xdevv
2xdevv deleted the perf/dfs-shard-alignment-headroom branch October 20, 2026 11:35
zxpdemonio added a commit to zxpdemonio/Mooncake that referenced this pull request Oct 21, 2026
…20261021

* upstream/main: (507 commits)
  [Reshard] Plan Store KV reuse across TP, PP and host layouts (kvcache-ai#4762)
  [Store] Walk tenants through shard-backed cursors and build the removes on them (kvcache-ai#4796)
  [Store-RS][Bugfix] Match TENT Request Layout to the C ABI (kvcache-ai#4732)
  [TransferEngine] Recover auto GID after data-path retry exhaustion (kvcache-ai#4290)
  [Store] Add the namespace layer over the object route (kvcache-ai#4721)
  [Store] Fix silent loss of offload keys deferred to the ungrouped pool (kvcache-ai#3006) (kvcache-ai#3009)
  [Store] Remove redundant alignment headroom from DFS shard allocation (kvcache-ai#4753)
  [CI/Build] Gate ROCm hardware tests and bound nightly Docker builds (kvcache-ai#4750)
  [Bugfix][Store] Fix double-counting of segment OpLog metrics in OpLogApplier (kvcache-ai#4771)
  [Store] Skip hot-cache writeback for device-resident read destinations (kvcache-ai#4764)
  [Reshard] Add bounded in-flight execution for weight transfers (kvcache-ai#4628)
  [Store] Avoid racing with detached hot-cache fills during key scans (kvcache-ai#4334)
  [Store] HA: drain the master on SIGTERM/SIGINT before exit (part 1 of kvcache-ai#4710) (kvcache-ai#4745)
  TENT: report a notification the receiver could not take (kvcache-ai#4754)
  [Store] Reach object metadata through a per-key accessor contract (kvcache-ai#4717)
  [Store][Bugfix] Fix DFS sync client test retry failure (kvcache-ai#4781)
  [Wheel] Keep integer dtype for typed_ragged rows when a row is an empty list (kvcache-ai#4611)
  [Doc] Sync README news onto the docs homepage (kvcache-ai#4767)
  [CI] remove stale Torch version binding after PG JIT (kvcache-ai#4760)
  [TransferEngine] Add buffer-protocol and page-gathered batch transfer APIs (kvcache-ai#4763)
  ...
zxpdemonio added a commit to zxpdemonio/Mooncake that referenced this pull request Oct 21, 2026
…-ranges-20261021

* upstream/main: (394 commits)
  [Reshard] Plan Store KV reuse across TP, PP and host layouts (kvcache-ai#4762)
  [Store] Walk tenants through shard-backed cursors and build the removes on them (kvcache-ai#4796)
  [Store-RS][Bugfix] Match TENT Request Layout to the C ABI (kvcache-ai#4732)
  [TransferEngine] Recover auto GID after data-path retry exhaustion (kvcache-ai#4290)
  [Store] Add the namespace layer over the object route (kvcache-ai#4721)
  [Store] Fix silent loss of offload keys deferred to the ungrouped pool (kvcache-ai#3006) (kvcache-ai#3009)
  [Store] Remove redundant alignment headroom from DFS shard allocation (kvcache-ai#4753)
  [CI/Build] Gate ROCm hardware tests and bound nightly Docker builds (kvcache-ai#4750)
  [Bugfix][Store] Fix double-counting of segment OpLog metrics in OpLogApplier (kvcache-ai#4771)
  [Store] Skip hot-cache writeback for device-resident read destinations (kvcache-ai#4764)
  [Reshard] Add bounded in-flight execution for weight transfers (kvcache-ai#4628)
  [Store] Avoid racing with detached hot-cache fills during key scans (kvcache-ai#4334)
  [Store] HA: drain the master on SIGTERM/SIGINT before exit (part 1 of kvcache-ai#4710) (kvcache-ai#4745)
  TENT: report a notification the receiver could not take (kvcache-ai#4754)
  [Store] Reach object metadata through a per-key accessor contract (kvcache-ai#4717)
  [Store][Bugfix] Fix DFS sync client test retry failure (kvcache-ai#4781)
  [Wheel] Keep integer dtype for typed_ragged rows when a row is an empty list (kvcache-ai#4611)
  [Doc] Sync README news onto the docs homepage (kvcache-ai#4767)
  [CI] remove stale Torch version binding after PG JIT (kvcache-ai#4760)
  [TransferEngine] Add buffer-protocol and page-gathered batch transfer APIs (kvcache-ai#4763)
  ...
daegyu94 pushed a commit to daegyu94/Mooncake that referenced this pull request Oct 23, 2026
Since kvcache-ai#4753, 100-byte allocations reserve 4 KiB instead of 8 KiB, so
four of them only fill half of the 32 KiB shard and never trigger
eviction. Use 8192-byte payloads to keep the shard full and assert the
initial preparation returns two candidates so the restore paths run.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation run-ci Store

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants