[Windows] Optimize `LayoutHandler.Windows.cs` (take 2) by MartyIX · Pull Request #26460 · dotnet/maui

MartyIX · 2024-12-08T10:05:05Z

Description of Change

I've been scratching my head how to improve performance of LayoutHandler and it seems that storing Children of PlatformView leads to a significant performance gain in my test scenario (#21787). This PR is a replacement for #26161 that implements @mattleibow's suggestion from the previous PR. Interestingly, the suggestion allows to easily use CachedChildren in more places and thus improve performance a bit more.

Explanation from @Sergio0694:

"Do we have an idea what the property [i.e. Panel.Children] 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).

Speedscope

main: 1733654186..speedscope.MAIN.json
PR: 1733653984..speedscope.PR.json

-> 97% improvement for IPanelMethods.get_Children.

Issues Fixed

Contributes to #21787

mattleibow

This looks very nice and I do like the fact that you pointed out: we can use the better api in other places.

One thing I think we can do to force usage is to add the old children api to the banned apis file. Then no one can accidentally use it.

src/Core/src/Platform/Windows/MauiPanel.cs

mattleibow · 2024-12-09T03:22:07Z

src/Core/src/Platform/Windows/MauiPanel.cs

+		{
+			get
+			{
+				_cachedChildren ??= Children;


Can we add the Children api to the banned apis?

Modified.

However, thanks to this change, the PR exploded a bit in size. I tried to split the work in multiple commits so if necessary, it can be split in multiple PRs¹.

Footnotes

One variant I like in case the PR is too big: Modify src/Core/src/Handlers/Layout/LayoutHandler.Windows.cs to use CachedChildren. Ban Children and suppress errors. Successive PRs would remove suppressions and use CachedChildren. ↩

MartyIX · 2024-12-09T09:54:04Z

src/Controls/src/Core/Compatibility/Handlers/Windows/VisualElementRenderer.cs

-			{
-				Control.Measure(availableSize);
-			}
+			Control?.Measure(availableSize);


Modified because MSVS shows the original if as an error.

MartyIX · 2024-12-09T09:54:47Z

src/Controls/src/Core/Compatibility/Handlers/Windows/VisualElementRenderer.cs


 		public UIElement? GeTPlatformElement() => Control;

+		UIElementCollection? _cachedChildren;


The parent type VisualElementHandler is in Microsoft.Maui.Controls.Handlers.Compatibility namespace. So I'm not sure if this type is to be touched or not.

MartyIX · 2024-12-09T09:58:03Z

src/Core/src/Handlers/View/ViewHandlerOfT.Windows.cs

+			}
+
+#pragma warning disable RS0030 // Do not use banned APIs; Panel.Children is banned for performance reasons. MauiPanel might not be used everywhere though.
+			var oldParentChildren = PlatformView.Parent is MauiPanel mauiPanel


The thing here is that I don't really believe that PlatformView.Parent is MauiPanel, it appears that MauiPanel is not used everytime.

So we can either

Leave it as is

Just suppress the error here instead of modifying code.

Have a new type CachedPanel that would extend Panel and would have CachedChildren implementation.

I didn't want to add excessive changes.

MartyIX · 2024-12-09T09:58:23Z

src/Core/src/Handlers/Window/WindowHandler.Windows.cs


-			if (window.VisualDiagnosticsOverlay != null)
-				window.VisualDiagnosticsOverlay.Initialize();
+			window.VisualDiagnosticsOverlay?.Initialize();


MSVS reported an error here

MartyIX · 2024-12-09T09:59:16Z

src/Core/src/Platform/Windows/WrapperView.cs

 			}
+
+			_shadowCanvasCachedChildren = null;
+			_shadowCanvas = null;


It appears that this was missing? I checked WrapperView in Android code and it has that.

MartyIX · 2024-12-09T09:59:28Z

src/Core/src/Platform/Windows/WrapperView.cs

 	public partial class WrapperView : Grid, IDisposable
 	{
 		Canvas? _shadowCanvas;
+		UIElementCollection? _shadowCanvasCachedChildren;


Not sure if this is not "too much".

I don't think it's too much, if anything it's a simple example of how to use caching

Redth · 2024-12-10T13:44:28Z

/azp run

azure-pipelines · 2024-12-10T13:44:52Z

Azure Pipelines successfully started running 3 pipeline(s).

PureWeen · 2024-12-10T19:59:19Z

/azp run

azure-pipelines · 2024-12-10T19:59:39Z

Azure Pipelines successfully started running 3 pipeline(s).

MartyIX · 2024-12-10T21:16:05Z

CI uncovered a few more places requiring changes. Force-pushed.

PureWeen · 2024-12-10T22:12:56Z

/azp run

azure-pipelines · 2024-12-10T22:13:17Z

Azure Pipelines successfully started running 3 pipeline(s).

…ing a container

PureWeen · 2024-12-10T23:20:04Z

/azp run

azure-pipelines · 2024-12-10T23:20:24Z

Azure Pipelines successfully started running 3 pipeline(s).

MartyIX · 2024-12-11T13:19:07Z

It appears there is no test failure on Windows.

jonathanpeppers · 2024-12-18T18:41:15Z

src/Compatibility/Core/src/Windows/ButtonRenderer.cs

+#pragma warning disable RS0030 // Do not use banned APIs; Panel.Children is banned for performance reasons.
 					container.Children.Add(image);
 					container.Children.Add(textBlock);
+#pragma warning restore RS0030 // Do not use banned APIs


@mattleibow should we instead just not use BannedApiAnalyzer in src/Compatibility? Maybe we don't care as much about this project going forward?

jonathanpeppers · 2024-12-18T18:43:12Z

src/Controls/src/Core/Platform/AlertManager/PromptDialog.Windows.cs

+#pragma warning disable RS0030 // Do not use banned APIs; Panel.Children is banned for performance reasons. Here we allow it as a part of initialization.
 		layout.Children.Add(TextBlockMessage);
 		layout.Children.Add(TextBoxInput);
+#pragma warning restore RS0030 // Do not use banned APIs


This one, you could have:

Suggested change

#pragma warning disable RS0030 // Do not use banned APIs; Panel.Children is banned for performance reasons. Here we allow it as a part of initialization.

layout.Children.Add(TextBlockMessage);

layout.Children.Add(TextBoxInput);

#pragma warning restore RS0030 // Do not use banned APIs

#pragma warning disable RS0030 // Do not use banned APIs; Panel.Children is banned for performance reasons. Here we can just cache it.

var children = layout.Children;

#pragma warning restore RS0030 // Do not use banned APIs

children.Add(TextBlockMessage);

children.Add(TextBoxInput);

Addressed in the new PR here: fa2d649#diff-fe91d0630fdf7c96daa6fc8ece1b59f70527d8160730d4bfe8666a94583b038a

jonathanpeppers · 2024-12-18T18:44:13Z

src/Controls/tests/DeviceTests/Elements/Modal/ModalTests.Windows.cs

+#pragma warning disable RS0030 // Do not use banned APIs; Panel.Children is banned for performance reasons. In tests, we don't mind.
+					var windowRootViewContainerChildren = windowRootViewContainer.Children;
+#pragma warning restore RS0030 // Do not use banned APIs


Wait, do the device tests use BannedApiAnalyzers?!? Seems like tests should not.

This is what controls it:

maui/Directory.Build.targets

Line 17 in ed53c15

<Import Condition="'$(SampleProject)' != 'true' and '$(IsTestProject)' != 'true'" Project="eng\BannedApis.targets" />

Seems like $(IsTestProject) should be true for the DeviceTests. Is it?

I created a new PR #26722 addressing this suggestion¹. See commit 1db1088.

Footnotes

It feels good for comparison purposes. ↩

MartyIX · 2024-12-20T20:45:52Z

Closing in favor of #26722

MartyIX requested a review from a team as a code owner December 8, 2024 10:05

MartyIX requested review from jfversluis and jsuarezruiz December 8, 2024 10:05

dotnet-policy-service bot added the community ✨ Community Contribution label Dec 8, 2024

MartyIX mentioned this pull request Dec 8, 2024

[Windows] Optimize LayoutHandler.Windows.cs #26161

Closed

MartyIX requested review from Foda and mattleibow and removed request for jfversluis and jsuarezruiz December 8, 2024 11:15

mattleibow reviewed Dec 9, 2024

View reviewed changes

MartyIX force-pushed the feature/2024-12-08-Optimize-LayoutHandler-for-Windows-2 branch from 6f2f471 to 7d53052 Compare December 9, 2024 09:44

MartyIX commented Dec 9, 2024

View reviewed changes

jfversluis added the area-layout StackLayout, GridLayout, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter label Dec 9, 2024

MartyIX added perf/general The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf) platform/windows labels Dec 10, 2024

MartyIX requested a review from mattleibow December 10, 2024 09:47

MartyIX force-pushed the feature/2024-12-08-Optimize-LayoutHandler-for-Windows-2 branch from 7d53052 to cfb3772 Compare December 10, 2024 19:57

MartyIX force-pushed the feature/2024-12-08-Optimize-LayoutHandler-for-Windows-2 branch from cfb3772 to 3c7c24c Compare December 10, 2024 21:13

MartyIX added 13 commits December 10, 2024 23:26

Optimize LayoutHandler.Windows.cs

0126555

Ban use of Microsoft.UI.Xaml.Controls.Panel.Children

1f8c123

Fix additional occurrences found by the Banned API analyzer

7c399ce

ButtonExtensions: Check if we use MauiPanel for children or not

1dd98c2

FlyoutPanel: Add CachedChildren implementation

a5f6678

RootPanel: Add CachedChildren implementation

8f9b580

WindowRootViewContainer: Add CachedChildren implementation

34050b5

WrapperView: Add CachedChildren implementation (even for shadows)

01d5f33

ViewHandler: Use CachedChildren if possible when setting up/remov…

73329f8

…ing a container

VisualElementRenderer: Add CachedChildren implementation

93881d9

Ignore accessing Panel.Children when benefit seems small

73d83dd

Fixes for device tests

e0d2141

Fixes for "Compatibility/Core" project

c76d0c5

MartyIX force-pushed the feature/2024-12-08-Optimize-LayoutHandler-for-Windows-2 branch from 3c7c24c to c76d0c5 Compare December 10, 2024 23:03

Foda approved these changes Dec 17, 2024

View reviewed changes

jonathanpeppers reviewed Dec 18, 2024

View reviewed changes

MartyIX mentioned this pull request Dec 19, 2024

[Windows] Optimize LayoutHandler.Windows.cs (take 3) #26722

Merged

MartyIX closed this Dec 20, 2024

MartyIX deleted the feature/2024-12-08-Optimize-LayoutHandler-for-Windows-2 branch December 20, 2024 20:46

github-actions bot locked and limited conversation to collaborators Jan 20, 2025


		public UIElement? GeTPlatformElement() => Control;

		UIElementCollection? _cachedChildren;

Conversation

MartyIX commented Dec 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of Change

Speedscope

Issues Fixed

Uh oh!

mattleibow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Redth commented Dec 10, 2024

Uh oh!

azure-pipelines bot commented Dec 10, 2024

Uh oh!

PureWeen commented Dec 10, 2024

Uh oh!

azure-pipelines bot commented Dec 10, 2024

Uh oh!

MartyIX commented Dec 10, 2024

Uh oh!

PureWeen commented Dec 10, 2024

Uh oh!

azure-pipelines bot commented Dec 10, 2024

Uh oh!

PureWeen commented Dec 10, 2024

Uh oh!

azure-pipelines bot commented Dec 10, 2024

Uh oh!

MartyIX commented Dec 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartyIX Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Footnotes

Uh oh!

MartyIX commented Dec 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

MartyIX commented Dec 8, 2024 •

edited

Loading

MartyIX Dec 19, 2024 •

edited

Loading