Skip to content

Conversation

@hazemawadalla
Copy link

This is a major architectural upgrade to the core benchmark logic. Replacing the original "Spillover" memory management strategy with the new "Waterfall LRU" implementation to accurately simulate enterprise storage hierarchies.

Key Changes:

  • Waterfall Eviction: Implemented recursive eviction (GPU -> CPU -> NVMe). New data now correctly lands in the fastest available tier, pushing cold data down, rather than the old behavior where new data skipped directly to NVMe if RAM was full.
  • Static Buffer Optimization: Replaced the CPU-bound np.random generation with a pre-allocated static noise buffer. This removes the CPU bottleneck that was masking true storage latency, allowing us to fully saturate high-performance NVMe drives.
  • Concurrency Hardening: Added semaphore-based concurrency limits (max_concurrent_allocs) and atomic memory reservations to prevent OOM crashes under heavy load.
  • Storage Metrics: Added explicit tracking for nvme_tokens_processed to calculate true storage throughput separate from system throughput.
  • Stress Test Validation: Verified that this new architecture correctly exposes storage latency limits (e.g., pushing P95 write latency >1000ms) where the old script artificially throttled the load.

@hazemawadalla hazemawadalla requested a review from a team December 9, 2025 16:04
@hazemawadalla hazemawadalla requested a review from a team as a code owner December 9, 2025 16:04
@github-actions
Copy link

github-actions bot commented Dec 9, 2025

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

This is a major architectural upgrade to the core benchmark logic. Replacing
the original "Spillover" memory management strategy with the new "Waterfall
LRU" implementation to accurately simulate enterprise storage hierarchies.

Key Changes:
- Waterfall Eviction: Implemented recursive eviction (GPU -> CPU -> NVMe).
  New data now correctly lands in the fastest available tier, pushing cold
  data down, rather than the old behavior where new data skipped directly
  to NVMe if RAM was full.
- Static Buffer Optimization: Replaced the CPU-bound np.random generation
  with a pre-allocated static noise buffer. This removes the CPU bottleneck
  that was masking true storage latency, allowing us to fully saturate
  high-performance NVMe drives.
- Concurrency Hardening: Added semaphore-based concurrency limits
  (max_concurrent_allocs) and atomic memory reservations to prevent OOM
  crashes under heavy load.
- Storage Metrics: Added explicit tracking for nvme_tokens_processed to
  calculate true storage throughput separate from system throughput.
- Stress Test Validation: Verified that this new architecture correctly
  exposes storage latency limits (e.g., pushing P95 write latency >1000ms)
  where the old script artificially throttled the load.
Copy link
Contributor

@wvaske wvaske left a comment

Choose a reason for hiding this comment

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

approved

This patch addresses two bugs that surface when running the benchmark
with --enable-rag:

1. Race condition in process_requests (line 2693)

   Worker threads begin processing requests immediately upon benchmark
   start, while RAG document ingestion runs in a separate daemon thread.
   When a worker hits the 10% RAG query path before any documents have
   been ingested, random.choice() is called on an empty list, raising
   IndexError.

   Fixed by adding a truthiness check on self.rag_manager.documents
   before entering the RAG code path. An empty dict evaluates to False,
   so RAG queries are safely skipped until ingestion populates at least
   one document.

2. Division by zero in KVCacheGenerator.generate (line 1097)

   The buffer slicing logic uses modulo to compute a pseudo-random start
   index: seed % (buffer_size - total_elements). When total_elements
   exactly equals buffer_size (an edge case permitted by the <= guard),
   the divisor becomes zero, raising ZeroDivisionError.

   Fixed by computing the divisor separately and defaulting start_idx
   to 0 when the divisor is zero.
… 4G of DRAM to reduce Queue contention and unrealistic read amplification
@johnugeorge johnugeorge merged commit 92d5e89 into mlcommons:TF_KVCache Dec 22, 2025
1 check passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants