SIGN IN SIGN UP

telemetry: reuse the drained staging buffer (#2086)

## What?

`nixlTelemetryStagingQueue` allocated and freed one capacity-sized
vector on every drain. It now keeps two capacity-reserved buffers and
alternates them, so no allocation happens after construction.

The drain contract changes from an owning `std::vector` to a borrowed
`std::span<const nixlTelemetryEvent>` valid until the next drain, and
`takePending()` is renamed to `drainPending()`.

Capacity, drop-newest overflow, all-or-none batches, drop accounting,
drain order and shutdown behaviour are unchanged.

## Why?

NIX-1641, from review feedback on the staging-queue extraction: the
drain was the last allocation left in the telemetry event path,
recurring once per flush interval for the life of the process. The drain
is now constant work; it previously contained a fixed-size malloc/free
pair whose latency depends on heap state rather than on how many events
were staged.

It is also a prerequisite for NIX-1544 (low-lock queue): with a
borrowed-view drain contract, that change can return a view straight
over its retired buffer instead of copying into an owning vector,
leaving it to be purely about removing the producer mutex.

<details>
<summary>How? — design, contract change, and validation</summary>

### What the drain used to cost

`takePending()` called `reserve(capacity_)` unconditionally, so every
flush allocated a fixed 64 KiB (4096 events × 16 B at the default
`NIXL_TELEMETRY_BUFFER_SIZE`) regardless of occupancy, and freed it when
the returned vector died at the end of `flushPendingEvents()`. Both ends
sat outside the critical section, so this never widened the window
producers block on — the win is bounded, which is why the NIX-1544
prerequisite leads.

### Double buffering

The queue holds `live_` (producers append here) and `drained_` (handed
to the consumer). The drain discards the previous drain's contents,
swaps the two buffers, and returns a view over `drained_`:

    std::span<const nixlTelemetryEvent>
    nixlTelemetryStagingQueue::drainPending() {
        const std::lock_guard<std::mutex> lock(mutex_);
        drained_.clear();
        live_.swap(drained_);
        return drained_;
    }

Both vectors are `reserve()`d to capacity at construction. `clear()`
keeps capacity, and `swap()` is a constant-time exchange of the two
vectors' internals — no element is copied or moved — so steady state is
allocation-free. Clearing before the swap (not after) is what keeps the
previous drain's storage available for reuse as the next `live_`. The
lock is held across the `return` so the span's pointer and size are
captured in the same critical section as the swap.

Colin's original note suggested moving off `std::vector` to a raw buffer
plus a `used` index. Kept as vectors deliberately: `nixlTelemetryEvent`
is trivially destructible, so `clear()` is already just a size reset and
`swap()` is already a constant-time pointer exchange — the raw buffer
would add hand-managed storage for no measurable gain.

Residency grows from one to two capacity-sized buffers — 128 KiB at the
default 4096-event capacity — in exchange for zero per-flush allocator
traffic.

### Why the rename

The single call site is `nixlTelemetry::flushPendingEvents()`, which
held the result in `auto`. A `std::span` binds to `auto` just as happily
as a `vector` does, so keeping the name would have let the call site
compile unchanged while silently acquiring the new lifetime rule.
Renaming to `drainPending()` forces every call site to be looked at.

### Thread safety

Unchanged in structure: producers only ever mutate `live_`, always under
the mutex, and the swap happens under the same mutex, so a producer
never observes a buffer mid-swap and the unlock publishes their writes
to the consumer.

The borrowed view is valid until the next drain, which is safe under the
queue's documented single-consumer contract: the flush runs on a
one-thread pool that re-arms only after the callback returns, so two
drains cannot overlap; the destructor joins that pool before members are
destroyed; and every exporter copies out of the `const &` rather than
retaining a pointer into the span.

### Tests

The 13 existing queue unit tests are adapted to the new name and view
return. Three were added for the properties that make this correct
rather than merely working:

- `DrainAlternatesBetweenTwoBuffersWithoutReallocating` — exactly two
distinct storage addresses across 200 drain cycles, which is the
allocation-free proof;
- `CapacitySurvivesAlternatingDrains` — the logical capacity still holds
after alternation (this is what fails if the `clear()` is dropped);
- `DrainViewCarriesOnlyTheCurrentDrain` — the second view is a different
buffer carrying only the new events.

### Validation

- Staging-queue unit tests: 16/16 pass in the normal, TSan and UBSan
builds, stable over `--gtest_repeat=5`.
- Telemetry gtest suites: 61/61 pass in the normal build, and 61/61
under both TSan and UBSan with no ThreadSanitizer warning and no UBSan
runtime error.
- DOCA: `doca_test`, `doca_nixl_test`, `histogram_parity_test` and
`telemetry_benchmark` pass against a local DOCA 3.3.

</details>

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Performance**
* Improved telemetry event draining to reduce memory allocations and
preserve buffer capacity.
  * Enhanced handling of concurrent telemetry collection and processing.

* **Reliability**
* Ensured drained telemetry batches remain isolated while new events
continue being collected.
* Improved zero-capacity queue handling and predictable batch lifetimes.

* **Tests**
* Added coverage for repeated draining, buffer reuse, capacity
preservation, isolation, and concurrent operations.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Efraim Eygin <eeygin@nvidia.com>
E
e-eygin committed
21afd554cc4d94cee56d6b21720fe7b2bf502e2f
Parent: 2301000
Committed by GitHub <noreply@github.com> on 8/14/2026, 1:18:05 PM