dotnet
Try fix EventRoute weak memory leak
Fixes https://github.com/dotnet/wpf/issues/9467
Reference https://github.com/dotnet/wpf/pull/6700 and https://github.com/dotnet/wpf/pull/9460
Description
See https://github.com/dotnet/wpf/issues/9467
After https://github.com/dotnet/wpf/pull/6700 , to reduce allocations when tracing routed events, we'll use _traceArguments field to store the trace arguments which cause https://github.com/dotnet/wpf/issues/9467 issues.
I try clear the reference from _traceArguments after call the TraceRoutedEvent.Trace.
And this pr is a patch, and I think https://github.com/dotnet/wpf/pull/9460 will be better. Thank you @kasperk81
Customer Impact
See https://github.com/dotnet/wpf/issues/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 https://github.com/dotnet/wpf/pull/6700
And Cc @bgrainger
Testing
CI Only.
Risk
Middling
Microsoft Reviewers: Open in CodeFlow
And I think the https://github.com/dotnet/wpf/pull/9460 is better than this. This pr is the patch.
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 , @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 ?
@dipeshmsft You are right. My PR #9463 just simple fix. @h3xds1nz 's https://github.com/dotnet/wpf/pull/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.
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.
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.
