fix: bound profiler memory growth by trimming interned call stacks - #5503
Open
jamescrosswell wants to merge 18 commits into
Open
fix: bound profiler memory growth by trimming interned call stacks#5503jamescrosswell wants to merge 18 commits into
jamescrosswell wants to merge 18 commits into
Conversation
Bumps the perfview submodule from e343a0cf (v3.1.15-5) to 9c4f637c (v3.2.6-3), 227 commits. That includes microsoft/perfview#2452, which adds TraceLog.TrimLiveSessionState() - the API needed to bound the call stack interning growth reported in #5469. Wiring it up is a separate change. Also removes the committed sample.etlx test fixture. Its ETLX format version is tied to the TraceEvent build (the new FastSerialization accepts >= 78, the committed copy was 74), so it goes stale on every submodule bump. TraceLogProcessorTests regenerates it from sample.nettrace when absent, so it is gitignored instead of committed. The verified snapshots change accordingly: module names lose a spurious ".il" suffix (System.Private.CoreLib.il -> System.Private.CoreLib). That is the only difference across both snapshots apart from a trailing newline. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
TraceLog interns every distinct call stack it observes and never releases them, so the continuous EventPipe session behind transaction profiling grew without bound - roughly 0.6 GB/day to OOM under production traffic on Linux. Uses TraceLog.TrimLiveSessionState() (microsoft/perfview#2452) to discard those tables once they pass a budget. The trim runs from an always-on ThreadSample handler on the factory, which matters for two reasons: it is the session's event processing thread, which is the only thread TrimLiveSessionState permits, and the tables grow whether or not a profile is running, so trimming only at profile boundaries would leave the growth unchecked whenever traffic is sparse. Trimming is gated on no profile being in progress AND the previous profile having stopped consuming samples. The second condition is not redundant: _inProgress is cleared when a transaction finishes, but that profiler stays subscribed and keeps resolving samples up to its end timestamp - roughly two seconds later, because TraceLog dispatches on a delay. Trimming inside that window would leave SampleProfileBuilder's CallStackIndex-keyed cache resolving to unrelated stacks. Passing the end timestamp to OnFinish lets the handler tell when that drain has finished. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5503 +/- ##
=======================================
Coverage 74.74% 74.75%
=======================================
Files 513 513
Lines 18829 18852 +23
Branches 3682 3684 +2
=======================================
+ Hits 14074 14093 +19
- Misses 3875 3879 +4
Partials 880 880 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jamescrosswell
commented
Aug 24, 2026
jamescrosswell
commented
Aug 24, 2026
jamescrosswell
commented
Aug 24, 2026
jamescrosswell
commented
Aug 24, 2026
jamescrosswell
commented
Aug 24, 2026
Co-authored-by: James Crosswell <jamescrosswell@users.noreply.github.com>
Removing the committed sample.etlx puts this reflection path on the critical path for every clean checkout, where before it never ran. CreateFromEventPipeEventSources is non-public API in the perfview submodule, so a bump can move it; the null-conditional call would then no-op and surface as a confusing file-not-found on the TraceLog constructor instead of naming the real cause. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CreateFromEventPipeEventSources does not take ownership of the source - upstream's own TraceLog.CreateFromEventPipeDataFile wraps it in a using for the same reason. Removing the committed sample.etlx means this path now runs on every clean checkout, so the handle was actually being leaked rather than sitting in dead code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Generating it on demand raced across target frameworks: _resourcesPath resolves to the shared source Resources directory for every TFM, and dotnet test runs one host per TFM, so they collided on both sample.etlx and the sample.etlx.new temp that TraceEvent writes alongside it. That already failed on .NET (win-arm64) with "the process cannot access the file ... because it is being used by another process". Committing it regenerated at the current format version keeps the tests deterministic and takes that whole class of failure off the table. README documents how to regenerate after a future submodule bump, which was the original reason for not committing it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jamescrosswell
commented
Aug 25, 2026
Gating the trim on "no profile running, and the previous one has drained" could starve indefinitely. Profiles run back to back under sustained load - a new one starts as soon as the previous clears _inProgress - so the window where trimming was permitted could simply never coincide with a dispatched sample. The tables then grow unbounded, in exactly the workload where that matters most. The guards were conservative because a trim reissues CallStackIndex from zero and SampleProfileBuilder caches against it. Rather than avoid that, invalidate it: SampleProfilerSession carries a generation that increments on every trim, and the builder drops _stackIndexes when it sees the generation move. Its other caches are unaffected, since a trim touches neither the code address and method tables nor TraceLog.Threads. That makes trimming safe at any moment, so both guards, _lastProfileEndTimeMs and the timestamp on OnFinish all go away. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jamescrosswell
commented
Aug 25, 2026
jamescrosswell
commented
Aug 25, 2026
jamescrosswell
commented
Aug 25, 2026
Co-authored-by: James Crosswell <jamescrosswell@users.noreply.github.com>
jamescrosswell
marked this pull request as ready for review
August 25, 2026 05:12
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 21be80c. Configure here.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both trim tests exited their work loop at the first trim, which made them timing-dependent in opposite ways. The idle test then compared a high-water mark sampled on the test thread against a count read later, while the dispatch thread kept interning - so with a tiny budget the "after" reading could exceed the "peak". It now runs for a fixed duration and asserts the property that actually matters: the table stays bounded. The in-profile test finished the profile milliseconds after starting it, so no samples had been dispatched yet and ValidateProfile found none. It now runs for a fixed duration too, which is also what makes collecting and validating the profile meaningful as a check that a mid-profile trim doesn't corrupt output. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
#5502 was squash merged, so the bump content now arrives from main rather than from this branch's commits. #5470 also landed, which reworked the factory - it introduced its own lock-guarded _session, so the volatile field added here is dropped in favour of it, and the trim handler captures the session from the startup closure rather than reading that field off the dispatch thread.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

Stacked on #5502 to keep the mechanical changes for the submodule bump independent of the fix itself. This PR targets
deps/update-perfviewand is just the fix.Fixes #5469
What was leaking
TraceCallStacks.InternCallStackIndexinterns every distinct call stack it observes and neverreleases any of them. That is correct for a trace file, which is finite, but the profiler's
EventPipe session runs for the life of the process — so the tables grow forever. Two properties made
it worse than it sounds: growth tracks the diversity of stack shapes rather than sample volume,
and it happens whether or not a profile is running, which is why lowering
ProfilesSampleRatenever helped and only setting it to
0did.The fix
TraceLog.TrimLiveSessionState()(microsoft/perfview#2452, contributed for this) discards those tables. This wires it up from an always-onThreadSamplehandler on the factory.A trim reissues
CallStackIndexfrom zero, andSampleProfileBuildercaches against it, so a stale entry would resolve a sample to an unrelated stack — wrong frames, no exception.So the cache must be invalidated.
SampleProfilerSessioncarries a generation that increments on every trim;SampleProfileBuilderrecords what it saw and drops_stackIndexeswhen the generation increments. Affected stacks get walked again, at worst appearing twice inProfile.Stacks, which samples reference by the builder's own ids. Potentially we spend a bit more CPU then - but we avoid the memory leak.Both run on the dispatch thread, so ordering can't bite: trim-then-
AddSamplesees the bump andclears,
AddSample-then-trim resolved against tables that were valid at that instant and sees thebump next time.
The builder's other caches need no handling:
_frameIndexesByCodeAddressIndexand_frameIndexesByMethodIndexkey off tables a trim doesn't touch, and_threadIndexeskeys offTraceLog.Threads, which is a different table from the per-thread roots insideTraceCallStacks.Trim Frequency
MaxCallStackCountis 100,000 (~10 MB of tables) - roughly every 7 minutes in the reporting customer's scenarion, and a trim measured at ~0.012 MiB of unreturned RSS. It is an internal static rather than aSentryOption- we could expose this if there was a compelling reason but I think best to leave internal initially.Why not just restart the session
That was the obvious alternative and it was measured and rejected: it bounds managed memory but costs
3.1–3.3 MiB of RSS per recycle that is never returned, making RSS grow roughly 3x faster than the
leak. Trimming in place costs ~0.012 MiB, because no new EventPipe session is created.
Effect
Same workload, 600 s, real time session on .NET 9 in a Linux container:
Throughput went slightly up, since the baseline spends real time growing and collecting those
tables. That workload is deliberately far more stack-diverse than a real service so the effect is
measurable in minutes — the shape matches the field report, the rate is exaggerated.
Test
Profiler_WhenNoProfileRunning_TrimsInternedCallStackscovers the idle path — no profile is everstarted, so the trim has to come from ordinary samples.
Profiler_WhileProfileRunning_StillTrimsInternedCallStackscovers the starvation case: it holds aprofile open for the whole test, asserts trimming still happens, then collects and validates the
profile to show a mid-profile trim doesn't corrupt its output. Both were mutation-tested — restoring
the old
_inProgressguard makes the second fail with "No trim occurred while a profile was running".Sentry.Profiling.Testson net10.0: 19 passed, 0 failed, 3 skipped. net8.0/net9.0 hosts can't belaunched locally (no arm64 runtimes for those on this machine), so CI covers those.