deps: update perfview (removes the .il suffix from profile module names) - #5502
Open
jamescrosswell wants to merge 6 commits into
Open
deps: update perfview (removes the .il suffix from profile module names)#5502jamescrosswell wants to merge 6 commits into
jamescrosswell wants to merge 6 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>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5502 +/- ##
==========================================
- Coverage 74.87% 74.78% -0.09%
==========================================
Files 513 513
Lines 18659 18759 +100
Branches 3636 3669 +33
==========================================
+ Hits 13970 14029 +59
- Misses 3819 3856 +37
- Partials 870 874 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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>
jamescrosswell
commented
Aug 24, 2026
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 24, 2026
Comment on lines
+58
to
+59
| ?? throw new InvalidOperationException( | ||
| "TraceLog.CreateFromEventPipeEventSources was not found. Has the perfview submodule changed its signature?"); |
Collaborator
Author
There was a problem hiding this comment.
This is the point of this particular change in the PR - gives a meaningful error if the signature ever changes.
vaind
approved these changes
Aug 25, 2026
vaind
left a comment
Contributor
There was a problem hiding this comment.
IDK the current state but previously some of the tests have been disabled (at least in CI?) as flaky. How is this change tested?
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.
Bumps the
perfviewsubmodule frome343a0cf(v3.1.15-5) to9c4f637c(v3.2.6-3) — 227 commits.The reason for doing it now is microsoft/perfview#2452, which adds
TraceLog.TrimLiveSessionState(): the API needed to bound the call stack interning growth behind#5469. This PR does not fix that leak — it only makes the API available by bumping to the appropriate version of perfview.
Regenerating
sample.etlxTraceLogProcessorTestsloads a committedsample.etlx, whose ETLX format version is tied to theTraceEvent build — the new FastSerialization accepts
>= 78and the committed copy was74, so itfailed to load after the bump. It has been regenerated at the current version and re-committed.
An earlier revision of this PR deleted it instead and let the test rebuild it from
sample.nettraceon demand, since it goes stale on every submodule bump. That turned out to be the wrong trade:
_resourcesPathresolves to the shared sourceResourcesdirectory for every target framework,and
dotnet testruns one host per TFM, so they raced on bothsample.etlxand thesample.etlx.newtemp TraceEvent writes beside it. That failed on.NET (win-arm64)withthe process cannot access the file ... because it is being used by another process.Committing it removes the writes entirely rather than trying to synchronise them. The staleness
problem that motivated the removal is now handled by documentation instead:
Resources/README.mdcovers how to regenerate, including the error that signals it is needed.
Two hardening fixes from review remain, since they protect whoever next regenerates the fixture and
runs that path deliberately: the reflection lookup now throws if the target moves, rather than
silently no-opping into a confusing file-not-found, and the
EventPipeEventSourceis disposed(upstream's own
CreateFromEventPipeDataFilewraps it the same way).Snapshot change
Both verified snapshots change in one way only: module names lose a spurious
.ilsuffix (System.Private.CoreLib.il→System.Private.CoreLib). 68 of the 70 changed lines are that rename and the other two are a trailing newline.These names surface in profile frames, so this is a small user-visible improvement.
Note on the NuGet package
4cfd9f57 Bump version to 3.2.6landed upstream before our fix, so the fix is not in the 3.2.6release — it will be in the next one. Until then the submodule remains necessary; switching
Sentry.Profilingback to aPackageReferenceis something we could consider in the future.