perf(brush): Don't use reflection to invoke brush updates #18899
Conversation
github-actions
bot
added
platform/wasm 🌐
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-18899/index.html |
|
🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-18899/index.html |
|
|
|
|
src/Uno.UI/UI/Xaml/Media/Brush.cs
Outdated
| #endif | ||
|
|
||
| namespace Microsoft.UI.Xaml.Media | ||
| { | ||
| [TypeConverter(typeof(BrushConverter))] | ||
| public partial class Brush : DependencyObject | ||
| { | ||
| private readonly WeakEventManager _weakEventManager = new(); | ||
| private List<WeakEventHelper.GenericEventHandler>? _invalidateRenderHandlers; |
There was a problem hiding this comment.
From my previous testing, I recall this was leaking.
I never seen perf issues from the current approach when profiling, and even if there is a perf issue, I feel @dr1rrb's approach is safer.
There was a problem hiding this comment.
Either previous approaches have issues, the original one is using reflection and is very slow on wasm (it uses the interpreter), and the used by David uses CWT which is costly as well. I agree that it might on some instances, particularly if instances are not collected, but we can improve that with GCHandles and gen2 GC detection now that we're in net8+.
There was a problem hiding this comment.
If it's problematic on Wasm specifically I think you might consider going with conditional code to use different approach on different platforms?
The main issue with WeakEventHelper is that the WeakReference instances are leaked (which is not detected by When_Add_Remove, still it's a memory leak).
On desktop, I never seen a perf issue for the existing approach, and it doesn't leak. I can't speak of other platforms though as I haven't profiled on them.
The real issue is that there is no good solution for weak events in C#. Everyone is inventing their own solution, and all the solutions have problems.
There was a problem hiding this comment.
I understand you concerns, but yes, it's sufficient, and using reflection for such a critical path is not a good way to handle this for any platform. I agree that the origin of the problem has not been addressed, but every version of the runtime provides its features that provide a new way for handling it.
I'll try with the GCHandle approach, we'll see.
jeromelaban
force-pushed
the
dev/jela/brush-weak
branch
from
7d5f7eb
to
92335c3
Compare
|
🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-18899/index.html |
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-18899/index.html |
github-actions
bot
added
area/automation
|
|
|
|
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-18899/index.html |
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-18899/index.html |
|
🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-18899/index.html |
|
|
|
🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-18899/index.html |
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-18899/index.html |
No description provided.