Skip to content

deps: update perfview (removes the .il suffix from profile module names) - #5502

Open
jamescrosswell wants to merge 6 commits into
mainfrom
deps/update-perfview
Open

deps: update perfview (removes the .il suffix from profile module names)#5502
jamescrosswell wants to merge 6 commits into
mainfrom
deps/update-perfview

Conversation

@jamescrosswell

@jamescrosswell jamescrosswell commented Aug 23, 2026

Copy link
Copy Markdown
Collaborator

Bumps the perfview submodule from e343a0cf (v3.1.15-5) to 9c4f637c (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.etlx

TraceLogProcessorTests loads a committed sample.etlx, whose ETLX format version is tied to the
TraceEvent build — the new FastSerialization accepts >= 78 and the committed copy was 74, so it
failed 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.nettrace
on demand, since it goes stale on every submodule bump. That turned out to be the wrong trade:
_resourcesPath resolves to the shared source Resources directory for every target framework,
and dotnet test runs one host per TFM, so they raced on both sample.etlx and the
sample.etlx.new temp TraceEvent writes beside it. That failed on .NET (win-arm64) with
the 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.md
covers 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 EventPipeEventSource is disposed
(upstream's own CreateFromEventPipeDataFile wraps it the same way).

Snapshot change

Both verified snapshots change in one way only: module names lose a spurious .il suffix (System.Private.CoreLib.ilSystem.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.6 landed upstream before our fix, so the fix is not in the 3.2.6
release — it will be in the next one. Until then the submodule remains necessary; switching
Sentry.Profiling back to a PackageReference is something we could consider in the future.

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

codecov Bot commented Aug 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.78%. Comparing base (2be688c) to head (a9f09c4).
⚠️ Report is 2 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jamescrosswell
jamescrosswell marked this pull request as ready for review August 24, 2026 05:01
@github-actions github-actions Bot added the risk: medium PR risk score: medium label Aug 24, 2026
Comment thread test/Sentry.Profiling.Tests/Resources/.gitignore Outdated
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>
Comment thread test/Sentry.Profiling.Tests/TraceLogProcessorTests.verify.cs Outdated
Comment thread test/Sentry.Profiling.Tests/TraceLogProcessorTests.verify.cs Outdated
jamescrosswell and others added 2 commits August 24, 2026 21:42
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>
Comment thread test/Sentry.Profiling.Tests/TraceLogProcessorTests.verify.cs
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>
Comment on lines +58 to +59
?? throw new InvalidOperationException(
"TraceLog.CreateFromEventPipeEventSources was not found. Has the perfview submodule changed its signature?");

@jamescrosswell jamescrosswell Aug 24, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the point of this particular change in the PR - gives a meaningful error if the signature ever changes.

@jamescrosswell
jamescrosswell requested a review from vaind August 24, 2026 22:03

@vaind vaind left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDK the current state but previously some of the tests have been disabled (at least in CI?) as flaky. How is this change tested?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk: medium PR risk score: medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants