perf: reuse counter snapshot buffer in CounterGroup.OnTimer#127886
perf: reuse counter snapshot buffer in CounterGroup.OnTimer#127886unsafePtr wants to merge 2 commits intodotnet:mainfrom
Conversation
|
Tagging subscribers to this area: @steveisok, @dotnet/area-system-diagnostics-tracing |
| _counters.CopyTo(counters); | ||
|
|
||
| counterCount = _counters.Count; | ||
| if (_onTimerCounters.Length < counterCount) |
There was a problem hiding this comment.
race condition can be raised? The _onTimerCounters is allocated/resized and then filled using _counters.CopyTo inside the s_counterGroupLock but later it is used outside the lock. This means theoretically _onTimerCounters or its contents can be changed before executing the next foreach loop.
I would suggest instead of doing that, we can use ArrayPool?
There was a problem hiding this comment.
There is only a single thread executing PollForValues
OnTimer itself is called sequentially in a foreach within PollForValues
There is no parrallel accessed pattern, and if _counters are modified, we are protected under s_counterGroupLock lock. _onTimerCounters is only ever read or written inside OnTimer.
In practice the counter set is fixed at first Enable and never mutated afterward. ArrayPool would pay Rent/Return overhead on every tick for a buffer that, in practice, never needs to be returned.
There was a problem hiding this comment.
When it would be possible to do stackalloc DiagnosticCounter[_counters.Count] it would be an easy change here. Actually now thinking about it again, we can do [InlineArray] for small counts (maybe below 32?). As it seems the max count of RuntimeEventSource is 27 counters. And use ValueListBuilder which switches to ArrayPool when there is no enough space.
i.e.
[InlineArray(32)]
private struct CounterScratch
{
private DiagnosticCounter? _element0;
}
CounterScratch scratch = default;
var builder = new ValueListBuilder<DiagnosticCounter>(scratch);
// instead of _onTimerCounters = new DiagnosticCounter[counterCount];
counterCount = _counters.Count;
Span<DiagnosticCounter> dst = builder.AppendSpan(counterCount);
CollectionsMarshal.AsSpan(_counters).CopyTo(dst);Seems to be an elegant solution, and this way we avoid holding an array. Will wait for your judgment before doing the change.
Match the GC behavior of the previous local-snapshot version: drop counter references the moment the foreach exits, instead of holding them until the next tick.
| counter.WritePayload((float)elapsed.TotalSeconds, (int)pollingInterval.TotalMilliseconds); | ||
| } | ||
|
|
||
| Array.Clear(_onTimerCounters); |
There was a problem hiding this comment.
Cleaning so we don't hold any references, and GC can collect them in case if _counters is updated
CounterGroup.OnTimerallocates a freshDiagnosticCounter[]snapshot every poll to copy the counter list. In practice the counter set is fixed atEventSourceconstruction and rarely changes. Reuse a per-instance buffer instead, grow on demand, and clear trailing slots for disposed counters.