Skip to content

fork: make StateGet chunk size runtime-configurable, default 2MB#7

Merged
ddvnguyen merged 3 commits into
hydra-forkfrom
fork/hydra-334-stateget-chunk-pipeline
Jun 22, 2026
Merged

fork: make StateGet chunk size runtime-configurable, default 2MB#7
ddvnguyen merged 3 commits into
hydra-forkfrom
fork/hydra-334-stateget-chunk-pipeline

Conversation

@ddvnguyen

@ddvnguyen ddvnguyen commented Jun 22, 2026

Copy link
Copy Markdown
Owner

What changed

Two commits:

  1. 8189debdc — initial fix: hardcoded CHUNK from 256 KB to 8 MB.
  2. 5f9263508superseding redesign: replaced the hardcoded constant with a runtime-configurable value, since a compile-time constant doesn't fit the engine control-plane pattern this codebase already uses for other tunables (SET_EXPERT_MODE, SWAP_QUANT — Hydra tells the engine what to do via RPC, not via rebuild). Default is now 2 MB, not 8 MB.

Concretely (commit 2):

  • llama_cparams::hydra_state_chunk_size (new field, default 2 * 1024 * 1024), mirroring the existing hydra_expert_mode field.
  • llama_context::hydra_set_state_chunk_size(size_t) setter, clamped to [64 KiB, 64 MiB] since it's reachable from the network.
  • llama_hydra_set_state_chunk_size / llama_hydra_get_state_chunk_size C API (include/llama-hydra.h).
  • llama_io_write_socket constructor now takes chunk_size instead of using a static constexpr CHUNK.
  • CONFIGURE (0x40) — previously received config_json and did nothing with it (pure stub returning {"success":true} unconditionally). Now actually parses it for a "state_chunk_size" field and applies it via the setter above. This is also the first production use of CONFIGURE; it was wired but unused before this PR.

Why

Closes #6 (tracks ddvnguyen/hydra_vortex#334).

Production StateGet traffic (the Hydra RPC wire protocol's engine→core save path) routes through llama_io_write_socket::write_tensor() — confirmed by reading the call path (hydra_handle_state_get()state_seq_get_data_to_fd() → this class), not assumed. For every chunk of every K/V tensor, this loop does, strictly serially with no overlap:

  1. ggml_backend_tensor_get() → on CUDA, ggml_backend_cuda_buffer_get_tensor() issues cudaMemcpyAsync immediately followed by cudaStreamSynchronize — effectively synchronous, fixed per-call driver overhead.
  2. A blocking send() syscall.

At the original 256 KB, a 1.21 GB KV state meant ~4,800 of these serialized round trips on a single thread — measured production throughput was a flat ~200 MB/s regardless of state size, consistent with fixed per-call overhead dominating over actual data movement (256 KB over PCIe/loopback should take microseconds, not ~1.3ms). This also explains the "100% user CPU" observation in the Hydra issue: the thread is busy through syscall/driver overhead, not idle on I/O.

Note: the original Hydra-side issue suspected realloc-per-layer buffer growth as the root cause. That doesn't apply here — this is the zero-copy M2 streaming path (no buffer growth, just a fixed-size reused staging buffer), and is the path all real RPC traffic uses.

What this does NOT do (scoped out, see #6 for the rest)

  • Double-buffer copy/send pipelining — only worth adding if the chunk-size change alone doesn't reach the ~600-800 MB/s target. Needs a live before/after measurement first; not implemented in this PR.
  • Parallelizing the layer loop across threads — the original Hydra issue suggested this, but concurrent cudaMemcpy calls from multiple host threads against the same CUDA context add synchronization risk for unclear gain once fixed-call overhead is amortized, and the wire format has no per-layer framing for out-of-order reassembly. Not planned.

How to test

Build: verified clean against both sm_120 (RTX) and sm_60 (P100) locally.

cmake --build build_sm120 --config Release -j$(nproc) --target llama-engine
cmake --build build_sm60  --config Release -j$(nproc) --target llama-engine

Correctness (round-trip) — not yet done, needed before merge:
Round-trip a large (500 MB–1.2 GB) sequence's KV state through STATE_GETSTATE_PUT and confirm identical generation output / cache_n after restore. No existing automated test covers /slots/{id}/state in tools/server/tests/, so this needs either a manual check against a running llama-engine instance, or a small ad-hoc script issuing the Hydra RPC StateGet/StatePut opcodes directly. Also worth a basic CONFIGURE round-trip check: send {"state_chunk_size":N} and confirm llama_hydra_get_state_chunk_size reflects it (e.g. via a log line — SRV_INF("hydra: CONFIGURE state_chunk_size=...")).

Throughput — not yet done, needed before merge:
On the Hydra side (paired with the corresponding ddvnguyen/hydra_vortex PR, which sends CONFIGURE {"state_chunk_size": ...} once per worker connection — see ConfigureStateChunkSizeAsync in WorkerSchedulerService.cs), send a 100K+ token request through the coordinator in P/D split (cold_concurrency) mode and watch the hydra_save_kv_rpc_seconds Prometheus metric / save_kv_rpc_ms phase on the Grafana dashboard. Target: ~6.2s/1.21GB (199 MB/s) baseline → ~2s/1.21GB (600-800 MB/s) or better.

(This wasn't measured yet on my end because the RTX GPU was fully loaded — 14.4/16.3 GB — by an already-running hydra-system stack at the time of this change, and swapping the binary would have required restarting that live stack. Deferring the live measurement + merge decision to whoever picks this up next.)

Links

llama_io_write_socket::write_tensor() chunks GPU->host copy + socket
send in lockstep; at 256KB each chunk pays a fixed
cudaMemcpyAsync+cudaStreamSynchronize round trip plus a send()
syscall, and that per-call overhead dominates for large states
(~4800 chunks for a 1.2GB transfer, matching the observed 6.2s/199MB/s
ceiling). 8MB cuts the call count ~32x.

Implements the C++ side of ddvnguyen/hydra_vortex#334.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace the hardcoded CHUNK constant with a runtime setting threaded
through llama_cparams (mirroring the existing hydra_expert_mode
pattern): llama_io_write_socket now takes chunk_size as a constructor
arg, sourced from cparams.hydra_state_chunk_size, defaulting to 2 MiB
(up from the original 256 KiB).

Wires CONFIGURE (0x40), previously a no-op stub, to actually parse and
apply a "state_chunk_size" field — Hydra can now tune this without a
rebuild, matching the rest of the engine control plane (SET_EXPERT_MODE,
SWAP_QUANT) rather than hardcoding a build-time constant. Clamped to
[64 KiB, 64 MiB] in the setter since it's reachable from the network.

New API: llama_hydra_set_state_chunk_size / llama_hydra_get_state_chunk_size
(include/llama-hydra.h).

Implements the C++ side of ddvnguyen/hydra_vortex#334.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@ddvnguyen ddvnguyen changed the title fork: bump StateGet socket-stream chunk size 256KB -> 8MB fork: make StateGet chunk size runtime-configurable, default 2MB Jun 22, 2026
…ize, doc race

Per code review on PR #7:

- Extract the chunk-size clamp into a pure llama_hydra_clamp_state_chunk_size()
  function exposed via llama-hydra.h, and add tests/test-hydra-state-chunk-size.cpp
  pinning its bounds (no llama_context/model needed — registered via
  llama_build_and_test so it runs on every platform).
- CONFIGURE (0x40) now echoes the post-clamp value back as
  "state_chunk_size_applied" in both the wire response (hydra_handle_configure)
  and to_json(), so the Coordinator can detect a silent clamp instead of
  trusting an unconditional success response.
- Document the benign cparams.hydra_state_chunk_size data race (written by
  the CONFIGURE task thread, read by state_seq_get_data_to_fd) — same
  pattern as the pre-existing hydra_expert_mode field, not synchronized
  because CONFIGURE is a one-time startup call.
- Confirmed (not a code change): all slots share one llama_context
  (server-context.cpp:1127, slot.ctx_tgt = ctx_tgt for every slot in the
  n_parallel loop), so CONFIGURE on slot "0" applies engine-wide — same
  scope as the existing SET_EXPERT_MODE precedent.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@ddvnguyen

Copy link
Copy Markdown
Owner Author

Pushed a follow-up commit (358955300) addressing review feedback:

  • Extracted the clamp into a pure llama_hydra_clamp_state_chunk_size(), exposed via llama-hydra.h, with a dedicated unit test (tests/test-hydra-state-chunk-size.cpp, registered via llama_build_and_test — no model/context needed, runs on every platform). Confirmed green locally.
  • CONFIGURE now echoes state_chunk_size_applied (post-clamp) in both the wire response (hydra_handle_configure) and to_json(), so a silent clamp is no longer invisible to the caller.
  • Documented the benign cparams.hydra_state_chunk_size data race inline (same pattern/rationale as the pre-existing hydra_expert_mode field).
  • Confirmed (no code change needed): all slots share one llama_context per engine process (server-context.cpp:1127), so CONFIGURE on slot "0" is correctly engine-wide.

Builds clean on sm_120 + sm_60. Still open: live throughput measurement + STATE_GET/PUT round-trip correctness (same as before — blocked on GPU availability).

@ddvnguyen ddvnguyen merged commit 6e397fb into hydra-fork Jun 22, 2026
ddvnguyen pushed a commit to ddvnguyen/hydra_vortex that referenced this pull request Jun 22, 2026
Re-bump from the unmerged branch-tip SHA (5f9263508 -> ...3589553) to
the actual merge commit 6e397fb325ae94f5ef9452d5405b478c4f1bbd3a on
hydra-fork, now that ddvnguyen/llama.cpp#7 is merged. Same pattern as
fork PR #5 -> Hydra PR #320.

Per the documented sequence: next is build sm_120/sm_60 + deploy, then
the live hydra_save_kv_rpc_seconds measurement + STATE_GET/PUT
round-trip correctness check.

Fork PR: ddvnguyen/llama.cpp#7
Hydra issue: #334

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ddvnguyen pushed a commit to ddvnguyen/hydra_vortex that referenced this pull request Jun 24, 2026
…armup hang; PR #5 NOT the culprit)

The previous attempt (revert of PR #5's is_rec term in the checkpoint
gate, commit 7fa82b18) was the wrong bisect direction. Live verification
on P100 shows :7fa82b18 still hangs in warmup at the exact same
futex_do_wait state as :6e397fb32, with the same warmup log
'common_init_from_params: warming up the model ...'. Removing the
checkpoint-creation gate term is not enough — the warmup hang is in
PR #7, not PR #5.

This reverts the parent-side config to :f518cff16 (PR #5 only, the
last tag where model load returns within ~30s on RTX, ~3 min on P100).
PR #7 (StateGet chunk size) is what's wedging the warmup; the
revert was a 'slips to next milestone' per issue's suggested next steps.

### What
- submodule: :7fa82b18 -> :f518cff16 (fork PR #5 only)
- node-rtx.yaml: :7fa82b18 -> :f518cff16
- node-p100.yaml: :7fa82b18 -> :f518cff16

### Cross-repo links
- Hydra issue: #346
- Fork PR (closed, was wrong): ddvnguyen/llama.cpp#9 (reverted)
- Bisect: f518cff16 works, 6e397fb32 hangs, 7fa82b18 hangs => PR #7 is the culprit

### Follow-up (out of scope for this PR)
- The fork-side fix is to revert PR #7 in ddvnguyen/llama.cpp#7 and
  re-introduce it once the warmup hang is root-caused upstream
  (likely interaction between llama_decode warmup and the
  hydra_state_chunk_size field's 2 MiB default in llama_cparams).
  Filed as a separate issue once the root cause is known.
- Hydra PR #338 (ConfigureStateChunkSizeAsync in WorkerSchedulerService.cs)
  becomes a no-op against the old engine; safe to keep on main.

### Verification plan
- [ ] curl http://localhost:8080/health -> 200 (RTX, ~30s)
- [ ] curl http://192.168.122.21:8086/health -> 200 (P100, ~3 min)
- [ ] E2E: pytest tests/system -k cold_concurrency (P/D split)

Closes #346

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[fork/hydra-334] StateGet: 256KB chunk size + serial copy/send caps throughput at ~200 MB/s

1 participant