Skip to content

Try fix EventRoute weak memory leak #9463

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

Conversation

lindexi
Copy link
Member

@lindexi lindexi commented Jul 25, 2024

Fixes #9467

Reference #6700 and #9460

Description

See #9467

After #6700 , to reduce allocations when tracing routed events, we'll use _traceArguments field to store the trace arguments which cause #9467 issues.

I try clear the reference from _traceArguments after call the TraceRoutedEvent.Trace.

And this pr is a patch, and I think #9460 will be better. Thank you @kasperk81

Customer Impact

See #9467

The memory will still be freed, albeit at a slower pace. Even if this issue is not addressed, it won't have a significant impact. There are two reasons for this. Firstly, the memory will still be freed, it just can't be released immediately. Secondly, this issue only affects those who have enabled Trace, which normal users typically do not do.

Regression

Reference #6700

And Cc @bgrainger

Testing

CI Only.

Risk

Middling

Microsoft Reviewers: Open in CodeFlow

@lindexi lindexi requested review from a team as code owners July 25, 2024 11:24
@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
@kasperk81
Copy link
Contributor

duplicate of dotnet/runtime#9460

@lindexi
Copy link
Member Author

lindexi commented Jul 25, 2024

And I think the #9460 is better than this. This pr is the patch.

@kasperk81
Copy link
Contributor

kasperk81 commented Jul 25, 2024

i've closed mine.

your fix still has chance of race, when multiple threads are updating the variable. make the variable local? correctness > performance

@lindexi lindexi force-pushed the t/lindexi/FixEventRouteMemoryLeak branch from 03d93e2 to b6b976a Compare July 26, 2024 01:30
@dipeshmsft
Copy link
Member

dipeshmsft commented Apr 1, 2025

@lindexi , @h3xds1nz this PR and #9468 are doing the same thing right, the difference being that here we are clearing out the array, whereas in the other PR we have replaced the array with ReadOnlySpan. Is there something more to it that I have missed ?

@lindexi
Copy link
Member Author

lindexi commented Apr 1, 2025

@dipeshmsft You are right. My PR #9463 just simple fix. @h3xds1nz 's #9468 can do more improve. I expected this PR to be incorporated into the official release as a way to fix the patch, so I decided to make minimal changes.

@dipeshmsft
Copy link
Member

I expected this PR to be incorporated into the official release as a way to fix the patch, so I decided to make minimal changes.

I completely agree with this. I was thinking of taking this for .NET 9 as this is minimal and solves the problem and will be easier to get approved for servicing.

@h3xds1nz
Copy link
Member

h3xds1nz commented Apr 1, 2025

I completely agree with this. I was thinking of taking this for .NET 9 as this is minimal and solves the problem and will be easier to get approved for servicing.

@dipeshmsft That was basically "our" intention with @lindexi.

@dipeshmsft
Copy link
Member

In that case, @lindexi can you retarget this PR to release/9.0 branch.

@lindexi lindexi changed the base branch from main to release/9.0 April 1, 2025 08:53
@lindexi
Copy link
Member Author

lindexi commented Apr 1, 2025

@dipeshmsft Done.

@dipeshmsft dipeshmsft merged commit 98634f6 into dotnet:release/9.0 Apr 9, 2025
8 checks passed
@dipeshmsft
Copy link
Member

Thanks a lot @lindexi for taking a stab at this issue.

@github-actions github-actions bot locked and limited conversation to collaborators May 9, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Community Contribution A label for all community Contributions Included in test pass PR metadata: Label to tag PRs, to facilitate with triage Servicing-approved Status:Completed Status:Proposed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WPF control is not reclaimed on NET9 by GC
7 participants