[WinUI] Perf when creating non-trivial grids (and other visual controls) (take 2) #22108

MartyIX · 2024-04-28T14:09:53Z

Description of Change

Alternative to #21818 which attempts to implement @jonathanpeppers's idea:

The part that makes me want to rethink this:

Every control has TranslateX/Y default to 0

Do we actually need to pass 0 to the platform? for every control?

I think there needs to be a concept of a default value
Where if you set TranslateX/Y it would pass to the platform and not otherwise
But I don't know if that breaks some fundamental design
this is just my observation
It would also be nice to get rid of one layer of Dictionary<string, lookup if that were possible

The idea of this PR is to introduce IView.IsPlatformViewNew which is true when there is a new platform view (which is known to have default values by default). So the first assignment of all properties can take into account this property and skip costly interop operations when an element is being added to the UI tree for the first time.

Relevant lines of code:

https://github.com/dotnet/maui/pull/22108/files#diff-fe5090f560d95c536e43dfc5f39442add6ad2d8eb555d675bdb4a0f82a30793dR59 - set IView.IsPlatformViewNew to true.
https://github.com/dotnet/maui/pull/22108/files#diff-fe5090f560d95c536e43dfc5f39442add6ad2d8eb555d675bdb4a0f82a30793dR92 - set IView.IsPlatformViewNew to false.
https://github.com/dotnet/maui/pull/22108/files#diff-6e0bbf3ab78552fc393db84d62ff02aa472a933af77ca0c93362bd0dd3e703a3 - optimizations

Notes:

Clicking "Batch Generate Grid" multiple times lead to ever increasing time to finish the operation. I'm not sure why.
Relevant issue [performance] all PropertyMapper's set the same values over and over #17303

Performance

Main: 424 ms

PR: 391 ms

Old measumerents

Main: 851 ms

PR: 559 ms

-> 34% faster.

Issues Fixed

Contributes to #21787

MartyIX · 2024-04-28T14:15:24Z

src/Controls/src/Core/Element/Element.cs

@@ -91,6 +91,8 @@ public string AutomationId
 			}
 		}

+		public bool IsPlatformViewNew { get; set; }


The name is terrible. It should be internal if possible.

I don't think there is a way to make this internal if it is a property on IElement.

Is there a way this could detect if Handler was null and became non-null for the first time? Then maybe we wouldn't need a new bool property?

Is there a way this could detect if Handler was null and became non-null for the first time? Then maybe we wouldn't need a new bool property?

I don't think I can simulate it that way because I need to set IsPlatformViewNew to true and then back to false:

https://github.com/dotnet/maui/pull/22108/files#diff-fe5090f560d95c536e43dfc5f39442add6ad2d8eb555d675bdb4a0f82a30793dR59 - set IView.IsPlatformViewNew to true.

https://github.com/dotnet/maui/pull/22108/files#diff-fe5090f560d95c536e43dfc5f39442add6ad2d8eb555d675bdb4a0f82a30793dR92 - set IView.IsPlatformViewNew to false.

an alternative which I would consider the best (in an ideal world) would be to pass a flag to:

maui/src/Core/src/Handlers/Element/ElementHandler.cs

Line 79 in e33d176

_mapper.UpdateProperties(this, VirtualView);

but unfortunately then I would need to propagate that flag here:

maui/src/Core/src/PropertyMapper.cs

Line 47 in e33d176

action?.Invoke(viewHandler, virtualView);

and that would mean changing the way mappers are defined:

maui/src/Core/src/PropertyMapper.cs

Line 18 in e33d176

protected readonly Dictionary<string, Action<IElementHandler, IElement>> _mapper = new(StringComparer.Ordinal);

which seems like a very invasive thing to do (with many and many changes required). That's why I went with the IView.IsPlatformViewNew flag to avoid making changes to mappers.

I don't think there is a way to make this internal if it is a property on IElement.

Is there a way this could detect if Handler was null and became non-null for the first time? Then maybe we wouldn't need a new bool property?

I was thinking about this a bit more and one alternative idea would be: Do not add Element.IsPlatformViewNew but rather add two new mapper calls:

InitializingNewElement

NoLongerInitializingNewElement

but I would still need to store the state somewhere and mappers are "state-less" AFAIK.

dotnet-policy-service · 2024-04-28T14:18:08Z

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.

jsuarezruiz · 2024-04-29T05:54:53Z

/azp run

azure-pipelines · 2024-04-29T05:55:15Z

Azure Pipelines successfully started running 3 pipeline(s).

jsuarezruiz

Based on the changes, needs to update also the Stubs used in the testing projects. For example:

D:\a\_work\1\s\src\Core\tests\DeviceTests.Shared\Stubs\WindowStub.cs(6,28): error CS0535: 'WindowStub' does not implement interface member 'IElement.IsPlatformViewNew' [D:\a\_work\1\s\src\Core\tests\DeviceTests.Shared\Core.DeviceTests.Shared.csproj::TargetFramework=net8.0-android]
    16 Error(s)

MartyIX · 2024-04-29T10:39:58Z

Based on the changes, needs to update also the Stubs used in the testing projects. For example:

I tried to fix it in b5d7287.

I believe that this PR requires a conceptual review because it's not clear (to me) whether the approach is OK or not.

jonathanpeppers · 2024-04-29T13:52:36Z

src/Controls/src/Core/Element/Element.cs

@@ -91,6 +91,8 @@ public string AutomationId
 			}
 		}

+		public bool IsPlatformViewNew { get; set; }


I don't think there is a way to make this internal if it is a property on IElement.

Is there a way this could detect if Handler was null and became non-null for the first time? Then maybe we wouldn't need a new bool property?

jonathanpeppers · 2024-04-29T13:54:15Z

src/Core/src/Platform/Windows/ViewExtensions.cs

-			AutomationProperties.SetAutomationId(platformView, view.AutomationId);
+		public static void UpdateAutomationId(this FrameworkElement platformView, IView view)
+		{
+			if (!view.IsPlatformViewNew || view.AutomationId is not null)


Is the default for AutomationId null or string.Empty? I don't know the answer, so just checking.

I saw null when testing it so I used null but I will re-check.

jonathanpeppers · 2024-04-29T13:56:26Z

src/Core/src/Platform/Windows/TransformationExtensions.cs

 			if (rotationX % 360 == 0 && rotationY % 360 == 0 && rotation % 360 == 0 &&
 				translationX == 0 && translationY == 0 && scaleX == 1 && scaleY == 1)


Is there a way this could do slightly better if someone set ScaleX/ScaleY, but not rotation or translation?

Personally, I think it would be a good start to extract changes from this file to a standalone PR because setting

maui/src/Core/src/Platform/Windows/TransformationExtensions.cs

Lines 21 to 22 in e33d176

frameworkElement.RenderTransformOrigin = new global::Windows.Foundation.Point(anchorX, anchorY);

frameworkElement.RenderTransform = new ScaleTransform { ScaleX = scaleX, ScaleY = scaleY };

and then setting

maui/src/Core/src/Platform/Windows/TransformationExtensions.cs

Line 28 in e33d176

frameworkElement.RenderTransform = null;

feels like RenderTransformOrigin does not be set at all in that situation and it also seems to be quite costly. I would need to re-measure it but I think this small change would have noticeable effect.

Is there a way this could do slightly better if someone set ScaleX/ScaleY, but not rotation or translation?

I'll check it.

MartyIX · 2024-07-26T20:36:28Z

First seen sum 200 ms. Haha.

MartyIX · 2025-02-11T09:44:56Z

Closing in favor of #27042

dotnet-policy-service bot added the community ✨ Community Contribution label Apr 28, 2024

MartyIX commented Apr 28, 2024

View reviewed changes

jsuarezruiz added platform/windows 🪟 legacy-area-perf Startup / Runtime performance labels Apr 29, 2024

jsuarezruiz reviewed Apr 29, 2024

View reviewed changes

jonathanpeppers reviewed Apr 29, 2024

View reviewed changes

Eilon added the t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf) label May 10, 2024

MartyIX force-pushed the feature/2024-04-28-WinUI-IsPlatformViewNew branch from b5d7287 to be1e415 Compare May 16, 2024 11:26

PureWeen added the area-layout StackLayout, GridLayout, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter label May 31, 2024

MartyIX force-pushed the feature/2024-04-28-WinUI-IsPlatformViewNew branch 3 times, most recently from 2bbd3a0 to b6f0807 Compare June 28, 2024 12:51

MartyIX force-pushed the feature/2024-04-28-WinUI-IsPlatformViewNew branch 3 times, most recently from 70ca386 to 84ed8e9 Compare July 26, 2024 20:35

MartyIX added 10 commits August 2, 2024 11:52

Add batch generate

4512242

Add batch generate

8272356

WIP: Add IsPlatformViewNew

9d2c947

FontSize

8951247

AccessibilityExtensions

1c29513

Fix stubs

49353da

...

87600f8

Generate grid

5f8f148

Improve

acf2c4f

Improve

e5ac68b

MartyIX added 7 commits August 2, 2024 11:52

improve

e2c79b7

Improve

b9392b8

Improve

10a19fb

Improve test code

b4d9d43

Grid bulk experiment (row level)

d9274a7

AccessibilityExtensions: Improve

a3bc00f

Grid bulk experiment (row level) (2)

f7bfa48

MartyIX force-pushed the feature/2024-04-28-WinUI-IsPlatformViewNew branch from 62e85c0 to f7bfa48 Compare August 2, 2024 09:53

MartyIX mentioned this pull request Aug 2, 2024

[Windows] Skip initial assignments in property mapper (take 3) #23987

Open

MartyIX closed this Feb 11, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WinUI] Perf when creating non-trivial grids (and other visual controls) (take 2) #22108

[WinUI] Perf when creating non-trivial grids (and other visual controls) (take 2) #22108

MartyIX commented Apr 28, 2024 •

edited

Loading

MartyIX Apr 28, 2024

jonathanpeppers Apr 29, 2024

MartyIX Apr 29, 2024

MartyIX May 16, 2024

dotnet-policy-service bot commented Apr 28, 2024

jsuarezruiz commented Apr 29, 2024

azure-pipelines bot commented Apr 29, 2024

jsuarezruiz left a comment

MartyIX commented Apr 29, 2024

jonathanpeppers Apr 29, 2024

jonathanpeppers Apr 29, 2024

MartyIX Apr 29, 2024

jonathanpeppers Apr 29, 2024

MartyIX Apr 29, 2024

MartyIX commented Jul 26, 2024

MartyIX commented Feb 11, 2025

		if (rotationX % 360 == 0 && rotationY % 360 == 0 && rotation % 360 == 0 &&
		translationX == 0 && translationY == 0 && scaleX == 1 && scaleY == 1)

	frameworkElement.RenderTransformOrigin = new global::Windows.Foundation.Point(anchorX, anchorY);
	frameworkElement.RenderTransform = new ScaleTransform { ScaleX = scaleX, ScaleY = scaleY };

[WinUI] Perf when creating non-trivial grids (and other visual controls) (take 2) #22108

[WinUI] Perf when creating non-trivial grids (and other visual controls) (take 2) #22108

Conversation

MartyIX commented Apr 28, 2024 • edited Loading

Description of Change

Performance

Issues Fixed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dotnet-policy-service bot commented Apr 28, 2024

jsuarezruiz commented Apr 29, 2024

azure-pipelines bot commented Apr 29, 2024

jsuarezruiz left a comment

Choose a reason for hiding this comment

MartyIX commented Apr 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartyIX commented Jul 26, 2024

MartyIX commented Feb 11, 2025

MartyIX commented Apr 28, 2024 •

edited

Loading