Skip to content

Modify AvTrace call chain to use params ReadOnlySpan<object> instead of an array #9468

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 15 commits into from
Apr 4, 2025

Conversation

h3xds1nz
Copy link
Member

@h3xds1nz h3xds1nz commented Jul 25, 2024

Fixes #9467 Reference #9463

Description

More of a complex fix, modifies the call chain to make use of the latest .NET features.

Customer Impact

Improved performance when tracing is active, decreased array allocations per each executed trace, smaller assemblies due to the function removal.

Regression

Fixes the ref held by _traceArguments from #6700

Testing

Local build, app run with tracing, comparing outputs of release/PR.

Risk

Low, the actual change is small and was reviewed/tested.

Microsoft Reviewers: Open in CodeFlow

@h3xds1nz h3xds1nz requested review from a team as code owners July 25, 2024 17:21
@dotnet-policy-service dotnet-policy-service bot added PR metadata: Label to tag PRs, to facilitate with triage Community Contribution A label for all community Contributions labels Jul 25, 2024
@h3xds1nz h3xds1nz changed the title Modify EventRoute/TraceData call chain to use params Span<object> instead of an array Modify AvTrace call chain to use params Span<object> instead of an array Jul 25, 2024
@h3xds1nz h3xds1nz changed the title Modify AvTrace call chain to use params Span<object> instead of an array Modify AvTrace call chain to use params ReadOnlySpan<object> instead of an array Jul 26, 2024
@Alexgoon
Copy link

Alexgoon commented Aug 8, 2024

Hi there! Could you please update us on the status of this pull request? It's important for DevExpress components, and we're eager to know if you plan to merge it.

@h3xds1nz
Copy link
Member Author

h3xds1nz commented Aug 8, 2024

Hey @Alexgoon, I'm gonna CC a few guys from the wpf team just to be sure it doesn't get lost.

cc @pchaurasia14 @dipeshmsft

@Alexgoon
Copy link

Hi again,
I see the merging process has started - thanks for that! However, it looks like it’s stalled. Could you let me know if you plan to include this PR in the upcoming .NET 9 release?

@pchaurasia14
Copy link
Member

@Alexgoon - We will be including this PR for upcoming test cycles, and it will go in for .NET 10.

@Alexgoon
Copy link

@pchaurasia14, thanks for the clarification! We were hoping the PR would be merged into .NET 9 since it addresses the issue described in #9467, which surfaced in .NET 9. After migrating to the latest version, a lot of our memory-related tests started failing.

I also noticed another PR (#9463) that might help resolve the memory leak issue. Do you have any plans to merge it into .NET 9, or will the memory issue (#9467) be handled as part of another fix?

@h3xds1nz
Copy link
Member Author

h3xds1nz commented Mar 3, 2025

@siagupta0202 Any news on this one?

@siagupta0202
Copy link
Contributor

@siagupta0202 Any news on this one?

Hey @h3xds1nz, I was involved with some other tasks so couldn't take a look at this PR. I will review this soon.

@h3xds1nz h3xds1nz added the regression status: This issue is a regression from a previous build or release label Mar 29, 2025
@h3xds1nz
Copy link
Member Author

@siagupta0202 Rebased so we can get this done finally.

Copy link

codecov bot commented Mar 31, 2025

Codecov Report

Attention: Patch coverage is 7.69231% with 60 lines in your changes missing coverage. Please review.

Project coverage is 10.96633%. Comparing base (2ded801) to head (a63706b).
Report is 4 commits behind head on main.

Additional details and impacted files
@@                 Coverage Diff                 @@
##                main       #9468         +/-   ##
===================================================
+ Coverage   10.95887%   10.96633%   +0.00746%     
===================================================
  Files           3310        3310                 
  Lines         664667      664388        -279     
  Branches       74667       74666          -1     
===================================================
+ Hits           72840       72859         +19     
+ Misses        590685      590389        -296     
+ Partials        1142        1140          -2     
Flag Coverage Δ
Debug 10.96633% <7.69231%> (+0.00746%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@siagupta0202
Copy link
Contributor

LGTM, @h3xds1nz can you resolve the merge conflicts?

@h3xds1nz h3xds1nz force-pushed the modify-traced-route-event-span branch from 7e26b70 to 9cc4c86 Compare April 3, 2025 12:55
@h3xds1nz
Copy link
Member Author

h3xds1nz commented Apr 3, 2025

@siagupta0202 Done, sorry for the delay :)

@siagupta0202 siagupta0202 merged commit aa023be into dotnet:main Apr 4, 2025
8 checks passed
@siagupta0202
Copy link
Contributor

@h3xds1nz Thank you so much for your contribution!

@h3xds1nz
Copy link
Member Author

h3xds1nz commented Apr 4, 2025

@siagupta0202 So happy to see this one in :) Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community Contribution A label for all community Contributions PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WPF control is not reclaimed on NET9 by GC
9 participants