feat(trace-utils): add span pool for recycling span allocations - #2477
feat(trace-utils): add span pool for recycling span allocations#2477paullegranddc wants to merge 1 commit into
Conversation
# Motivation Allocation is expensive, and spans use a lot of small collections (meta, metrics, span links, events...). Being able to recycle the allocations should be beneficial in terms of perf. # Changes * Add `SpanPool<T>` in `libdd-trace-utils/src/span/span_pool.rs`, backed by a bounded crossbeam channel so the pool never grows without limit. A thread-local chunk cache keeps the single-producer (exporter) path lock-free and gives each thread a local chunk under contention. * Add `PooledChunks<'_, T>`, which wraps `Vec<Vec<Span<T>>>` and returns its spans to a `SpanPool` on drop. `PooledChunks::unpooled()` is a zero-overhead wrapper for callers that do not use the pool. * Add `MaybePool` so call sites can feed spans/chunks back to the pool only when one is attached, and otherwise drop them. * Add `Send` bounds to `SpanText`/`SpanBytes` so pooled `Vec<Span<T>>` can flow through the crossbeam channel and thread-local cache. * Add criterion benchmarks for the pool (recycle vs. allocate) under `libdd-trace-utils/benches/span_pool.rs`. Nothing is wired into the live pipeline yet, so there is no behavior change. The follow-up stacked PR switches the data-pipeline send path over to these types. # Additional Notes Pure addition, no behavior change — the new types are not used anywhere yet, so there is no regression risk.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
📚 Documentation Check Results📦
|
🔒 Cargo Deny Results📦
|
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34891d4de0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let at = remaining.len() - MAX_CHUNK_SIZE; | ||
| Some(remaining.split_off(at)) |
There was a problem hiding this comment.
When an accepted chunk exceeds MAX_CHUNK_SIZE, split_off leaves the original allocation and its full capacity in remaining, so the final ≤20-span piece still owns the potentially much larger backing buffer. That piece can remain in the queue when traffic drops and may later become a thread's empty_chunk, defeating the split's stated purpose of preventing a thread from hoarding a large chunk; rebuild or shrink the final piece while preserving only the spans' internal allocations.
AGENTS.md reference: AGENTS.md:L76-L76
Useful? React with 👍 / 👎.
| let current = self.inner.len.load(Ordering::Relaxed); | ||
| if current + piece_len > self.inner.capacity { | ||
| return; |
There was a problem hiding this comment.
When capacity < MAX_CHUNK_SIZE and a returned chunk has more than 20 spans, the first generated piece contains 20 spans, fails this check, and the return discards both it and every smaller remainder. For example, a capacity-10 pool can never recycle anything from a 21-span chunk even though one span remains after the first split, so callers using a valid small capacity continually allocate; continue examining remainders or size pieces according to the available capacity.
AGENTS.md reference: AGENTS.md:L76-L76
Useful? React with 👍 / 👎.
| fn drop_policy() -> bool { | ||
| const PCT_OF_SPANS_RETURNED_DROPPED: f64 = 0.1; | ||
| thread_local! { | ||
| static RNG: RefCell<rand::rngs::SmallRng> = RefCell::new(rand::rngs::SmallRng::from_entropy()); |
There was a problem hiding this comment.
On a thread whose OS or WASM entropy source is unavailable, SmallRng::from_entropy() panics instead of returning an error. Since PooledChunks::drop reaches this initializer through add_chunks, routine cleanup can unexpectedly panic, and doing so while another panic is already unwinding aborts the host process; this non-cryptographic sampling policy should use an infallible seed or deterministic counter instead.
AGENTS.md reference: AGENTS.md:L72-L72
Useful? React with 👍 / 👎.
| let current = self.inner.len.load(Ordering::Relaxed); | ||
| if current + piece_len > self.inner.capacity { | ||
| return; | ||
| } | ||
| self.inner.len.fetch_add(piece_len, Ordering::Relaxed); |
There was a problem hiding this comment.
When two threads return chunks concurrently, both can load the same len, both pass the capacity check, and then both increment it, leaving the unbounded channel above the configured capacity until consumers happen to drain it. With many concurrent PooledChunks drops the excess scales with the number of producers, so the constructors' “at most capacity” guarantee does not hold for this otherwise thread-safe, cloneable pool; reserve space with a compare-and-update operation rather than a separate load and fetch_add.
AGENTS.md reference: AGENTS.md:L76-L76
Useful? React with 👍 / 👎.
| // No drop-policy control here, but a single span is very likely retained. | ||
| // If we drop 10% of spans, the likelyhood all spans are dropped is 1/10**100 | ||
| // which is basically never happening if we ran this test until the heat death of | ||
| // this universe | ||
| let chunks = pool.wrap_chunks(vec![vec![span("a"); 100]]); |
There was a problem hiding this comment.
drop_policy makes one decision for this entire 100-span input chunk, so the probability that every span is discarded is 10%, not 1/10**100; moreover, whether the chunk is retained or discarded, every subsequent get_span() has the default name and the assertion passes. The test therefore still succeeds if recycling is completely broken, so make the drop policy controllable in tests and assert retained capacity or another property that distinguishes a recycled span from a fresh default.
Useful? React with 👍 / 👎.
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
BenchmarksComparisonBenchmark execution time: 2026-09-07 19:11:21 Comparing candidate commit 34891d4 in PR branch Found 18 performance improvements and 8 performance regressions! Performance is the same for 127 metrics, 0 unstable metrics.
|
1 participant
Motivation
Allocation is expensive, and spans use a lot of small collections (meta, metrics, span links, events...). Being able to recycle the allocation should be beneficial in term of perf.
This PR adds the pool types only. A stacked follow-up PR (#2382) switches the data-pipeline send path over to them.
Changes
SpanPool<T>inlibdd-trace-utils/src/span/span_pool.rsPooledChunks<'_, T>, which wrapsVec<Vec<Span<T>>>and returns its spans to aSpanPoolon drop.PooledChunks::unpooled()is a zero-overhead wrapper for callers that do not use the pool.MaybePoolso call sites can feed spans/chunks back to the pool only when one is attached, and otherwise drop them.Sendbounds toSpanText/SpanBytesso pooledVec<Span<T>>can flow through the crossbeam channel and thread-local cache.libdd-trace-utils/benches/span_pool.rs.Performance
This makes the alloc/populate/dealloc cycle about 20% to 30% faster
Additional Notes
Pure addition, no behavior change. The new types are not used anywhere yet. The stacked PR #2382 wires them into the trace exporter.