[HiCache][Observability] Add fine-grained L1<->L2 KV transfer metrics (events / blocks / bytes / duration / bandwidth)#26974
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
ishandhanani
left a comment
There was a problem hiding this comment.
Tested with HiCache + write_through, forcing offload (write-back) and onboard (load-back) by evicting a 1408-token prefix to host and re-requesting it. Both the Prometheus series and the DEBUG logs work and line up with each other:
# /metrics
sglang:hicache_l1_l2_transfer_events_total{direction="offload",...} 9
sglang:hicache_l1_l2_transfer_blocks_total{direction="offload",...} 198
sglang:hicache_l1_l2_transfer_bytes_total{direction="offload",...} 1.453e9
sglang:hicache_l1_l2_transfer_events_total{direction="onboard",...} 1
sglang:hicache_l1_l2_transfer_blocks_total{direction="onboard",...} 22 # 1408 tokens // page_size 64 = 22
# DEBUG log
Onboard transfer complete blocks=22 bytes=161480704 xfer_us=24254 bandwidth=6.66GB/s src="sglang_hicache::L2" dst="sglang_hicache::L1"
Onboard transfer cumulative direction="onboard" total_events=1 total_blocks=22 total_bytes=161480704 ...
block_count matches token_count // page_size exactly and the cumulative log totals match the counters. Nice addition. A few things inline — the first is a hard break for a config this PR doesn't cover.
| @@ -143,6 +146,18 @@ class HiCacheAck(NamedTuple): | |||
| finish_event: device_module.Event | |||
| node_ids: List[int] | |||
|
|
|||
| # Number of KV token slots moved by this merged operation. | |||
| token_count: int | |||
There was a problem hiding this comment.
Adding these 4 non-default fields to HiCacheAck means every consumer that unpacks it positionally has to be updated. The radix-cache controllers here are all converted, but two sites that still do the old 3-tuple unpack were missed:
python/sglang/srt/disaggregation/decode_kvcache_offload_manager.py:205—_, finish_event, ack_list = self.cache_controller.ack_write_queue.pop(0)will raiseValueError: too many values to unpack (expected 3)once these acks have 7 fields (decode-side KV offload path).test/registered/unit/mem_cache/test_unified_radix_cache_unittest.py:1350— same 3-tuple unpack ofack_load_queue.
(Defaults wouldn't help — unpacking a 7-tuple into 3 names fails regardless — so those two consumers need the ack.finish_event / ack.node_ids treatment too.)
There was a problem hiding this comment.
Good catch, thanks. The main branch evolves, so I did a rebase. After the rebase there are a few more stale consumers too, not just the two originally listed. I have updated all ack_write_queue / ack_load_queue consumers to use ack.finish_event and ack.node_ids, and update the tests that still mock the old 3-tuple shape.
| else: | ||
| raise ValueError(f"Unknown HiCache L1/L2 transfer direction: {direction}") | ||
|
|
||
| xfer_us = self._transfer_elapsed_us(ack) |
There was a problem hiding this comment.
record_l1_l2_transfer_complete runs on every transfer regardless of --enable-metrics: _transfer_elapsed_us calls start_event.elapsed_time(finish_event) (a CUDA call) and the hicache_l1_l2_transfer_totals dict is updated each time; only the Prometheus .inc() is gated on the collector. So it's not quite 'zero overhead otherwise' as the description says — there's a per-transfer elapsed_time() even with metrics and debug logging both off. Could early-return / skip the elapsed_time when not self.enable_l1_l2_transfer_metrics and not logger.isEnabledFor(logging.DEBUG).
There was a problem hiding this comment.
Agreed. I added an early return in record_l1_l2_transfer_complete when both metrics are disabled and DEBUG logging is off, before calling _transfer_elapsed_us. I also gated the cumulative debug totals behind the same debug check so the normal path stays effectively no-op.
There was a problem hiding this comment.
Related to this elapsed_time() path, I think there may also be a timing-semantics issue: these ACK events appear to be created via device_module.Event() without enable_timing=True.
In PyTorch, torch.cuda.Event defaults to enable_timing=False, so the event is usable for readiness/query/sync but is not a timing event. If elapsed_time() fails and falls back to host perf_counter_ns(), this metric can mix device-side DMA timing with host wall-clock timing, including scheduling/polling/ACK-processing delay.
This is related to the issue addressed in #26411. It may be worth separating readiness events from timing events here too, especially since bandwidth is derived from the duration.
| for _, finish_event, ack_list in cc.ack_write_queue: | ||
| finish_event.synchronize() | ||
| for ack_id in ack_list: | ||
| for ack in cc.ack_write_queue: |
There was a problem hiding this comment.
This write_back=True (blocking drain) branch doesn't call record_l1_l2_transfer_complete, while the non-blocking branch just below does — and hiradix_cache.py does record in its equivalent blocking branch (hi_mamba_radix_cache.py also omits it). So flush-path offloads are undercounted inconsistently across the three caches. Minor, but worth making uniform.
There was a problem hiding this comment.
Makes sense. I made the blocking drain paths consistent by calling record_l1_l2_transfer_complete(direction="offload", ack=ack) after synchronizing each ack in both unified_radix_cache.py and hi_mamba_radix_cache.py, matching hiradix_cache.py.
| labelnames=labelnames, | ||
| ) | ||
|
|
||
| self.transfer_time_us_total = Counter( |
There was a problem hiding this comment.
transfer_time_us_total (and transfer_events_total above) duplicate the histogram's _sum / _count — I confirmed they match exactly in my run (events 9 == duration_us_count 9; time_us 94853 == duration_us_sum 94853). Could drop the two counters and read _sum/_count off transfer_duration_us. The blocks/bytes counters are the useful cumulative additions. Minor.
There was a problem hiding this comment.
Agreed, the histogram already gives _count and _sum, so keeping separate event/time counters is redundant. I dropped hicache_l1_l2_transfer_events_total and hicache_l1_l2_transfer_time_us_total, while keeping the blocks/bytes counters and duration histogram.
6451dfe to
c366119
Compare
|
Re-tested the updated branch (
Two remaining issues:
Fix + before/after evidence: hxieustc#2. With it applied, the same idle-gap onboard reports |
8bd3424 to
076755e
Compare
aecb383 to
9d91092
Compare
|
/tag-and-rerun-ci |
f9fea5b to
be53ff3
Compare
…edTreeNode The fix commit added write_through_pending_id earlier in __init__ (after priority, grouped with other node state), but main already had the same assignment at the end of __init__. Remove the trailing duplicate.
…d disagg offload path HybridCacheController and the disagg offload HiCacheController were created without enable_metrics / extra_metric_labels, so HiCacheL1L2TransferMetrics Collector was never initialised on those paths even when --enable-metrics is set. Thread enable_metrics and extra_metric_labels through: - hybrid_pool_assembler: all 5 build_*_stack functions, all 6 StackStrategy build() methods (including the base), and the top-level strategy.build() dispatch call -- following the exact same pattern as enable_storage_metrics. - decode_kvcache_offload_manager: pass server_args.enable_metrics and server_args.extra_metric_labels when constructing HiCacheController.
…nsfers The old _host_pool_bytes_per_token summed size_per_token across every HostPoolGroup entry and multiplied by the anchor KV token count. This over-counts sidecar pools (Mamba, SWA, DSA indexer) that use a different index cardinality: MambaPoolHost uses page_size=1 (one slot per node) while the KV pool uses page_size=N, so Mamba bytes were inflated by N. Replace with _host_pool_size_per_token(pool_name) for named per-pool lookup, _transfer_index_count(transfer) for actual sidecar index counts, and update _estimate_l1_l2_transfer_bytes / _make_hicache_ack to accept pool_transfers so each sidecar contributes its own (actual_index_count * size_per_token).
… to attach_hybrid_pool_to_mamba_cache The function body already forwarded these two names to build_hybrid_mamba_stack, but they were never declared in the signature, causing a NameError at startup for any Mamba model with HiCache enabled.
… DSA attach path The DSA (MLATokenToKVPool / DeepSeek V4) branch called attach_hybrid_dsa_pool_to_hiradix_cache without these args, so the HybridCacheController was always constructed with enable_metrics=False. The non-DSA else-branch correctly passed both params; this brings the DSA branch into parity.
…ck in hybrid write/load paths Completes the byte-estimation fix in cache_controller.py: without forwarding resolved_pool_transfers here, sidecar pools (Mamba, SWA, DSA indexer) would still contribute zero bytes to the ack, leaving only KV bytes accounted for.
… to attach_hybrid_pool_to_mamba_cache The function body already forwarded these two names to build_hybrid_mamba_stack, but they were never declared in the signature, causing a NameError at startup for any Mamba model with HiCache enabled.
Two issues left the transfer metrics non-functional on GPU after merging main: 1. Durations were never recorded. record_l1_l2_transfer_complete feeds ack.start_event.elapsed_time(finish_event) into the duration histogram, but the events were created without enable_timing=True, so elapsed_time() always raises on CUDA and _transfer_elapsed_us returns None -> the duration_us histogram and bandwidth got no data (xfer_us=unavailable, bandwidth=0). Create the event pairs used for timing with enable_timing=True (LayerLoadingEvent.start_event + last layer event; write-path start/finish in cache_controller and hybrid_cache_controller). The Optional[int] host fallback is preserved for backends without elapsed_time(). 2. The Prometheus collector was never created without L3 storage. main moved self.pp_rank/pp_size assignment into _generate_storage_config, which only runs when a storage backend is attached; _init_l1_l2_transfer_metrics (needed for the collector) reads self.pp_rank and crashed (AttributeError) or was skipped. Derive pp_rank/pp_size in __init__ before calling _init so the collector is built for plain HiCache too. Verified on 1xL40S: offload/onboard duration histogram now populates (count == debug cumulative events), ~26 GB/s device timing, gating intact (no --enable-metrics -> 0 series).
Add test/registered/unit/mem_cache/test_hicache_l1l2_metrics.py. CPU-only via _ControllerStub; covers collector call-chain and record_l1_l2_transfer_complete with DI recording doubles.
Add isolated-registry tests verifying metric names, label values, and counter/histogram sample values in generate_latest() output.
Drop helper/edge-case tests; keep 2 DI-double, 3 controller-stub, and 4 Prometheus exposition tests.
be53ff3 to
169c7ad
Compare
Motivation / Problem statement
HiCache constantly moves KV between L1 (GPU) and L2 (CPU host) via write-back (offload, L1→L2) and load-back (onboard, L2→L1), but there is no per-operation accounting of data volume, transfer time, or bandwidth. This makes it hard to understand transfer cost, diagnose load-back stalls, or validate device↔host DMA performance.
Modifications
Instrument the device↔host path in HiCacheController, exposed in logs and Prometheus:
HiCacheAckextended withtoken_count,block_count,byte_count, host-sidestart_time_nsfallback timer;_make_hicache_ack()computes block/byte estimates (incl. draft KV pool).record_l1_l2_transfer_complete(direction=...)called at each ack-completion point; logs per-transfer and cumulative stats and feeds the collector. Duration prefers CUDA event elapsed_time, falling back to host wall-clock.HiCacheL1L2TransferMetricsCollectorexposes counters + a duration histogram, labeled by direction/src/dst plus TP/DP/PP/attn-CP rank and io_backend.hiradix_cache,unified_radix_cache,hi_mamba_radix_cache,hybrid_cache_controller;ack-queueconsumers refactored from tuple-unpacking to attribute access.enable_metrics; the in-process cumulative totals dict is always kept for logging.Benefits
How to enable
The transfer log lines are emitted whenever HiCache is active (DEBUG level).
The Prometheus metrics are additionally gated by
--enable-metrics:--enable-metrics constructstheHiCacheL1L2TransferMetricsCollectorand exposes thesglang:hicache_l1_l2_transfer_* series; without it, no Prometheus objects are created but the cumulative log lines still appear.Expected output
Logs.
Prometheus /metrics.
Derived bandwidth in Grafana:
rate(...bytes_total[1m]) / rate(...time_us_total[1m])Correctness
_make_hicache_ack() helper.HostKVCache,HostPoolGroupwrappers, and the draft pool. Duration degrades to host wall-clock whenevent.elapsed_time()is unavailable.Caveats
byte_countis an estimate (token_count × bytes_per_token); bandwidth is derived from it, not from measured DMA bytes.finish_event.synchronize()inloading_checkbefore recording onboard completion — small sync cost on the load-back path.Speed Tests and Profiling
Instrumentation is lightweight (counter increments + one log line per completed merged op). The only added synchronization is the load-back synchronize(), on acks that already complete before tree-state updates — no measurable serving impact.
Checklist
CI States
Latest PR Test (Base): ❌ Run #28265436149
Latest PR Test (Extra): ❌ Run #28265435914