[Windows] Optimize LayoutHandler.Windows.cs#26161
[Windows] Optimize LayoutHandler.Windows.cs#26161MartyIX wants to merge 1 commit intodotnet:mainfrom
LayoutHandler.Windows.cs#26161Conversation
|
Hey there @MartyIX! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
| protected override void ConnectHandler(LayoutPanel platformView) | ||
| { | ||
| base.ConnectHandler(platformView); | ||
| _platformViewChildren = platformView.Children; |
There was a problem hiding this comment.
The main question is if this is safe to cache here. Is it?
There was a problem hiding this comment.
So, the entire benefit here is this avoids calling the FrameworkElement.Children property over and over?
Do we have an idea what the property does underneath? Can we decompile it? Are they creating a copy of a native list -> managed on purpose?
I wonder if the native list will move and the cached one is out of date later.
/cc @PureWeen
There was a problem hiding this comment.
So, the entire benefit here is this avoids calling the
FrameworkElement.Childrenproperty over and over?
Yes.
Do we have an idea what the property does underneath? Can we decompile it? Are they creating a copy of a native list -> managed on purpose?
I'll try to dig into it. However, I believe @Sergio0694 @codendone might know off the bat.
I wonder if the native list will move and the cached one is out of date later.
/cc @PureWeen
Yes, that's the hard question.
There was a problem hiding this comment.
"Do we have an idea what the property does underneath?"
Assuming that's some WinUI Panel control, accessing that property will invoke the get_Children method on the native WinRT object, then marshal the returned IInspectable* pointer to a C# object (ie. it'll go find the existing RCW for it, or create a new one if it doesn't exist already).
There was a problem hiding this comment.
"Do we have an idea what the property does underneath?"
Assuming that's some WinUI
Panelcontrol, accessing that property will invoke theget_Childrenmethod on the native WinRT object, then marshal the returnedIInspectable*pointer to a C# object (ie. it'll go find the existing RCW for it, or create a new one if it doesn't exist already).
That sounds like it's OK for the purposes of this PR.
I wonder if the native list will move and the cached one is out of date later.
I don't know the answer but if it were the case, then I believe that many optimizations I did would lead to crashes. So I don't think so.
There was a problem hiding this comment.
I mean it's not a bug, it's just the expected behavior 😅
Yeah all WinRT issues go in the CsWinRT repo, you can always post there if you have any specific problems and we can help!
There was a problem hiding this comment.
Yeah all WinRT issues go in the CsWinRT repo, you can always post there if you have any specific problems and we can help!
Is there anything else that we can do here to improve performance? Or is caching the best one can achieve?
I noticed that there is microsoft/CsWinRT#1843 but I'm not sure if it might be relevant for MAUI.
There was a problem hiding this comment.
That is more about supporting some additional scenarios and fixing some warnings, but it doesn't impact performance at all. If your code is working today, that PR will basically not do much at all for you (which is not a bad thing, just expected).
There was a problem hiding this comment.
Sounds like a bug! Slow code is no go code! Is there an issue currently tracking performance on the interop?
I don't know how this would be a bug. Doing interop vs no-interop that is simply reading a field is going to be faster.
There was a problem hiding this comment.
Sounds like a bug! Slow code is no go code! Is there an issue currently tracking performance on the interop?
I don't know how this would be a bug. Doing interop vs no-interop that is simply reading a field is going to be faster.
It could be a bug if it were much slower than expected. I guess it would be great to write a benchmark unless there is one already. But I fear it works as fast as it was designed and there is nothing we can do.
| { | ||
| /// <summary>Children of the platform view.</summary> | ||
| /// <remarks>Stored for performance reasons.</remarks> | ||
| UIElementCollection? _platformViewChildren; |
There was a problem hiding this comment.
I know this makes it better, but putting view specific data on handlers forces people to derived from the handler to customize sometimes.
What happens if you use an attached property on the platform view?
There was a problem hiding this comment.
I created a new PR #26460 to make it easy to compare this and the new PR. The new PR looks better to me.
You mentioned attached property, I used a normal property. Not sure if you really meant attached for some reason.
There was a problem hiding this comment.
I meant attached because I forgot we owned the type. But since it is our type, your PR is much better and we can even add the old children api to the banned apis
|
Closing in favor of #26460 |
Description of Change
I've been scratching my head how to improve performance of
LayoutHandlerand it seems that storingChildrenofPlatformViewleads to a significant performance gain in my test scenario (#21787).Hopefully, there is not a logical error. Please review carefully.
Speedscope
-> 98 % improvement for
IPanelMethods.get_Children, and ~2% improvement for the app itself.Issues Fixed
Contributes to #21787