Add TraceLog.ResetCallStacks to bound real time call stack growth - #2452
Add TraceLog.ResetCallStacks to bound real time call stack growth#2452James Crosswell (jamescrosswell) wants to merge 6 commits into
Conversation
In a real time session the call stack interning tables grow for the lifetime of the session: TraceCallStacks.InternCallStackIndex appends to callStacks and callees for every distinct stack observed, and nothing is ever released. Trace files are finite so this never mattered there, but a long running process with diverse stacks grows without bound. FlushRealTimeEvents already trims eventsToStacks, eventsToCodeAddresses and cswitchBlockingEventsToStacks, but not the interning tables themselves, which are by far the largest of the structures involved. Add TraceLog.ResetCallStacks(), which discards the interned call stacks and returns those tables to their initial state. It is opt in, restricted to real time sessions, and invalidates any CallStackIndex handed out earlier, so the caller decides when no such index is live. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@microsoft-github-policy-service agree company="Functional Software, Inc." |
|
James Crosswell (@jamescrosswell) thank you for contributing this PR and for the detailed description of the scenario and change. I agree that something along these lines is useful and necessary. In fact, it's something that should exist for both ETW and EventPipe live sessions. Ideally, we implement the same pattern for both, though it's possible that the underlying implementation may need to be slightly different. Thinking about what you've written here and the code diff, I think it makes sense to have an opt-in API that users can call when they're ready to destroy existing state. I had hoped that we could do something more automatic, but this just presents more complex lifetime trade-offs since we wouldn't have any way to know when it's truly safe to toss this state. Question on your real scenario: Do you have a situation where you hold onto existing state (e.g. stack indices) and use them later to recall stacks, or would it technically be safe for you to call this on every callback? I'm also thinking a bit about the API name - I think I'd like to name it something that can be more generic (e.g. |
There was this (now shelved) PR building task/async profiling on ActivityComputer So having that historic data might have made it easier to solve some problems... but resolving a memory leak that is bringing down customer's servers whenever they have profiling enabled is obviously way more important, so we'll have to find another solution to that problem.
Not quite safe, in our scenario. Our sample handler resolves each stack synchronously inside the callback. It walks Caller() to the root, resolves, method names, and builds our own frame objects. However our builder keeps a Dictionary<int,int> keyed by CallStackIndex so a repeated stack isn't re-walked, and that cache lives for the duration of one profile (up to 30 s). After a reset, indexes are reissued starting from zero for different stacks, so a surviving cache entry would silently resolve to the wrong stack. Our plan was to reset between profiles instead, where we'd also discard that cache. So we don't retain indexes to recall stacks later, but we do use them as cache keys (same lifetime requirement).
Happy to rename to something more generic, in case this grows to cover more structures. No strong opinions there. Maybe |
Background
The .NET runtime includes EventPipe which creates a nettrace stream for runtime events that are used for profiling.
TraceLog, from the TraceEvent library in PerfView, turns those raw events into something symbolic — it tracks processes, threads, modules, methods and code addresses so thatTraceEvent.CallStack()returns frames rather than hex addresses.Call stacks are the bulk of that data and the most redundant: consecutive samples usually share almost every frame. So
TraceCallStacksdoes not store stacks, it interns them into a prefix tree. Each node isCallStackInfo { codeAddressIndex, callerIndex }, a whole stack is identified by a singleCallStackIndex. This structure is maintained in three arrays:callStacks(nodes),callees(each node's children, searched when interning) andthreads(per-thread roots).For a trace file this works really well. The input is finite, and while you are analysing it you want every stack it contains to stay resolvable, so the tree is deliberately a cache with no eviction.
Real time use case
#1867 added streaming / in-memory EventPipe support, and #2169 added the rundown-provider API that lets frames resolve mid-session. Those made
TraceLogusable against a live session rather than a file — which is what continuous profilers need, and what the Sentry .NET SDK is built on.The data model came along unchanged.
InternCallStackIndexstill appends, and nothing is ever released. If you point that at a server process that runs for weeks then the tree grows for the life of the session (i.e. indefinitely).Additionally:
This is already known
[TraceEvent] Microsoft.Diagnostics.Tracing.Etlx.TraceCallStacks+CallStackInfo[] - High memory usage #1199 reported the same
CallStackInfo[]growth in 2022. It was closed as not planned because the original reporter stopped seeing it, not because it was resolved or ruled out.[TraceEvent] TraceCallStacks interning tables grow without bound on the real-time/EventPipe path in long-lived processes (~0.6 GB/day to OOM) #2451 was opened recently by a Sentry customer running into this issue.
FlushRealTimeEventsalready carries deliberate memory management for real time sessions (TraceLog.cs#L952-L955):The three it trims are the small event-to-stack maps. This PR adds a fourth — the largest one, roughly fourteen times the size of the next biggest structure in the dump above.
The change
The addition of a new
TraceLog.ResetCallStacks()method that discards the interned call stacks and returns the tree to its initial state.It is inert for every existing consumer. It is opt in, nothing calls it, no existing behaviour changes, and it throws outside a real time session.
Two implementation details worth review attention:
GrowableArrays rather than callingGrowableArray.Clear().Clear()only resets the length and keeps the backing store — and forcalleesandthreadsthat store holds a reference to every childList<CallStackIndex>ever created, which is the bulk of the memory being released.eventsToStacksandcswitchBlockingEventsToStacks, which map events to the indexes just invalidated. Those hold plain structs, so a length reset genuinely is enough there — they are cleared for correctness, not for memory.Measured
Synthetic workload, 600 s, real time EventPipe session, .NET 9, Linux container:
After warm up the reset run moved +2.1 MiB over 9 minutes and 174 resets — about 0.012 MiB per reset. Sample throughput went up, because the baseline spends real time growing and collecting those tables. The workload is deliberately far more stack-diverse than a real service, to make the effect measurable in minutes; the shape matches the field report below, the rate is exaggerated.
From the field — getsentry/sentry-dotnet#5469, an ASP.NET Core service on Linux growing ~0.6 GB/day until the kernel OOM-killed it at ~8 days of uptime.
dotnet-gcdumpshowed the two halves of this tree at exactly the same size:Setting the profile sample rate to 0 — the only thing that stops a session being created at all — cured it completely, with every TraceEvent type disappearing from the heap.
Why the other tables are left alone
codeAddresses,methodsandILToNativeMapalso grow, and are deliberately untouched.Clearing them would be irreversible. The rundown that populates method names for code JIT'd before the session started runs once, as a separate short-lived session at
TraceLogcreation, and cannot be re-run against a live session.They are not strictly bounded — tiered compilation, expression trees,
RegexOptions.Compiledand reflection emit all mint new methods over time — so a residual, much smaller growth remains for apps doing continuous runtime codegen.Addressing this needs a different mechanism, and is left as separate work.
The contract
ResetCallStacksasks of callersUniversal preconditions:
CallStackIndexobtained earlier may still be in useWhat is not universal is when a given consumer can know its indexes are dead — a consumer that resolves synchronously in the callback can reset between any two events, while one that caches by index can only do so between cache lifetimes. That depends entirely on its own retention pattern, so the API states the invariant and leaves the timing to the caller.
This is also why we've added an explicit method rather than, say, a
MaxCallStackCountproperty that resets itself inside the event pump.Test
EventPipeParsing.ResetCallStacksDiscardsInternedStacksAndKeepsInterning, modelled on the existingSessionStreamingtest. It starts a real time session, lets stacks accumulate, resets from inside the sample callback, then asserts both halves of the contract: the tables are emptied, and stacks interned afterwards still resolve through to a method name.It also walks the entire post-reset caller chain, range checking every link. That is deliberate —
GrowableArray's indexer only guards withDebug.Assert, so in Release a stale index reads the backing store and silently resolves to the wrong frame instead of throwing. Asserting one frame resolves is not enough to catch that; walking the chain is.Runs in ~350 ms, and is skipped below .NET Core 3.0 like its neighbour.
A known gap, flagged deliberately. The test does not fail if
threadsis left uncleared — verified this by mutation. Stale per-thread roots resolve todefault(CallStackInfo), whose code address index is 0, which is almost never a valid/real address, so they sit inert and get appended to. The consequence is retained memory plus a rare correctness risk (a code-address-0 collision, orIndexOutOfRangeExceptiononce a stale index exceeds the new capacity) rather than deterministic corruption. A memory-based test would cover it but seems a poor trade for CI time. Suggestions welcome.