[WinUI] Allocate less when updating gestures by MartyIX · Pull Request #21450 · dotnet/maui

MartyIX · 2024-03-26T10:53:21Z

Description of Change

This PR adds a few very basic optimizations to improve performance of GesturePlatformManager.UpdateDragAndDropGestureRecognizers and gestures in general.

Measurements

Using commit e984695, I did:

cd src/Controls/samples/Controls.Sample.Sandbox
dotnet publish -f net8.0-windows10.0.19041.0 -c Release -p:PublishReadyToRun=false -p:WindowsPackageType=None
dotnet trace collect --format speedscope -- .\bin\Release\net8.0-windows10.0.19041.0\win10-x64\publish\Maui.Controls.Sample.Sandbox.exe

Speedscope files:
Maui.Controls.Sample.Sandbox.exe_20240326_114219.speedscope.MAIN.json
Maui.Controls.Sample.Sandbox.exe_20240326_114435.speedscope.PR.json

Results

main:

PR:

Unfortunately, the results are not stable. So the results are not meaningful. I think one would have to start the application and then create gestures in code to measure it better. Any other ideas?

Issues Fixed

Contributes to #21449
See also: #21377

This reverts commit e984695.

dotnet-policy-service · 2024-03-26T10:53:32Z

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.

MartyIX · 2024-03-26T10:53:52Z

 			}
 		}
+
+		public static bool HasAnyGesturesFor<T>(this IEnumerable<IGestureRecognizer>? gestures, Func<T, bool>? predicate = null) where T : GestureRecognizer


This is to avoid allocation on L53.

MartyIX · 2024-03-26T10:55:08Z


-			_container.CanDrag = gestures.GetGesturesFor<DragGestureRecognizer>()
-				.FirstOrDefault()?.CanDrag ?? false;
+			bool canDrag = gestures.GetGesturesFor<DragGestureRecognizer>().FirstOrDefault()?.CanDrag ?? false;


Store value in a local variable, so we save 1 interop call on L692 (i.e. if (_container.CanDrag) vs. if (canDrag)) and L698.

Can this use your new HasAnyGesturesFor() method? And check CanDrag inside the predicate?

I have come up with acadf48. Not really sure if that is what you had in mind.

MartyIX · 2024-03-26T10:55:33Z

 				children?.GetChildGesturesFor<TapGestureRecognizer>().ToList();

-			if (gestures.GetGesturesFor<TapGestureRecognizer>(g => g.NumberOfTapsRequired == 1).Any()
+			if (gestures.HasAnyGesturesFor<TapGestureRecognizer>(g => g.NumberOfTapsRequired == 1)


This should save 1 allocation.

MartyIX · 2024-03-26T10:55:44Z

-			bool hasPanGesture = gestures.GetGesturesFor<PanGestureRecognizer>().GetEnumerator().MoveNext();
+			bool hasSwipeGesture = gestures.HasAnyGesturesFor<SwipeGestureRecognizer>();
+			bool hasPinchGesture = gestures.HasAnyGesturesFor<PinchGestureRecognizer>();
+			bool hasPanGesture = gestures.HasAnyGesturesFor<PanGestureRecognizer>();


3 allocations here.

So one thing to consider is this would still iterate over the gestures list three times. How many are normally in this list? If it's usually like 1-5, that might be OK.

I think the list contains only a handful of items as you said.

MartyIX · 2024-03-26T14:44:48Z

cc @jonathanpeppers

jsuarezruiz · 2024-03-26T15:33:47Z

/azp run

azure-pipelines · 2024-03-26T15:34:12Z

Azure Pipelines successfully started running 3 pipeline(s).

jonathanpeppers · 2024-03-26T16:51:50Z

-			bool hasPanGesture = gestures.GetGesturesFor<PanGestureRecognizer>().GetEnumerator().MoveNext();
+			bool hasSwipeGesture = gestures.HasAnyGesturesFor<SwipeGestureRecognizer>();
+			bool hasPinchGesture = gestures.HasAnyGesturesFor<PinchGestureRecognizer>();
+			bool hasPanGesture = gestures.HasAnyGesturesFor<PanGestureRecognizer>();


So one thing to consider is this would still iterate over the gestures list three times. How many are normally in this list? If it's usually like 1-5, that might be OK.

jonathanpeppers · 2024-03-26T16:53:25Z

 			}
 		}
+
+		public static bool HasAnyGesturesFor<T>(this IEnumerable<IGestureRecognizer>? gestures, Func<T, bool>? predicate = null) where T : GestureRecognizer


Can we make the new method(s) internal?

Addressed in edd66ef.

jonathanpeppers · 2024-03-26T16:54:10Z


-			_container.CanDrag = gestures.GetGesturesFor<DragGestureRecognizer>()
-				.FirstOrDefault()?.CanDrag ?? false;
+			bool canDrag = gestures.GetGesturesFor<DragGestureRecognizer>().FirstOrDefault()?.CanDrag ?? false;


Can this use your new HasAnyGesturesFor() method? And check CanDrag inside the predicate?

jonathanpeppers · 2024-03-26T16:55:36Z

+				return false;
+			}
+
+			predicate ??= x => true;


Could we just do a null check inside the foreach?

if (item is T gesture && (predicate is null || predicate(gesture))) { return true; }

This way we aren't creating a lambda like x => true.

Addressed in 39b7ec6.

…ing + add braces

MartyIX · 2024-03-26T19:33:19Z

Main: Maui.Controls.Sample.Sandbox.exe_20240326_202255.speedscope.json

-> UNMANAGED_CODE_TIME: 1:26 (1 minute 26 seconds)

PR: Maui.Controls.Sample.Sandbox.exe_20240326_202543.speedscope.json

-> UNMANAGED_CODE_TIME: 54.21s

On sample code:

MainPage.xaml

<ContentPage
    xmlns="http://schemas.microsoft.com/dotnet/2021/maui"
    xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml"
    x:Class="Maui.Controls.Sample.MainPage"
    x:DataType="local:MainPage"
    xmlns:local="clr-namespace:Maui.Controls.Sample">
    <VerticalStackLayout x:Name="myLayout">

        <Button Text="Add" Clicked="Button_Clicked"/>

        <Label Text="Drag me">
            <Label.GestureRecognizers>
                <DragGestureRecognizer CanDrag="True" />
            </Label.GestureRecognizers>
        </Label>

    </VerticalStackLayout>
</ContentPage>

MainPage.xaml.cs

using System;
using System.Collections.ObjectModel;
using System.Diagnostics;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Maui;
using Microsoft.Maui.Controls;
using Microsoft.Maui.Graphics;

namespace Maui.Controls.Sample
{
	public partial class MainPage : ContentPage
	{
		public MainPage()
		{
			InitializeComponent();
		}

		private void DragGestureRecognizer_DragStarting(object? sender, DragStartingEventArgs e)
		{

		}

		private void Button_Clicked(object sender, EventArgs e)
		{
			for (int i = 0; i < 10000; i++)
			{
				// Add the following XAML.
				/*
					<Rectangle Stroke="Red" Fill="DarkBlue" StrokeThickness="4" HeightRequest="200" WidthRequest="200">
						<Rectangle.GestureRecognizers>
							<DragGestureRecognizer DragStarting="DragGestureRecognizer_DragStarting" />
						</Rectangle.GestureRecognizers>
					</Rectangle>
				 */

				Microsoft.Maui.Controls.Shapes.Rectangle rectangle = new()
				{
					Fill = Colors.DarkBlue,
					Stroke = Colors.Red,
					StrokeThickness = 4,
					WidthRequest = 200,
					HeightRequest = 200,
				};

				DragGestureRecognizer recognizer = new();
				recognizer.DragStarting += DragGestureRecognizer_DragStarting;

				rectangle.GestureRecognizers.Add(recognizer);

				myLayout.Add(rectangle);
			}
		}
	}
}

jonathanpeppers · 2024-03-26T20:52:56Z

/azp run

azure-pipelines · 2024-03-26T20:53:17Z

Azure Pipelines successfully started running 3 pipeline(s).

jonathanpeppers

The changes make sense to me. 👍

But I think someone most familiar with Windows gestures on the MAUI team should also review.

MartyIX · 2024-03-29T17:24:49Z

@mattleibow Could you please take a look?

MartyIX added 4 commits March 26, 2024 11:20

EnumerableExtensions: Enable nullable

acabea4

Add EnumerableExtensions.HasAnyGesturesFor

19d7b69

Sandbox measure code

e984695

Revert "Sandbox measure code"

1b3612b

This reverts commit e984695.

MartyIX requested a review from a team as a code owner March 26, 2024 10:53

MartyIX requested review from StephaneDelcroix and mattleibow March 26, 2024 10:53

dotnet-policy-service Bot added the community ✨ Community Contribution label Mar 26, 2024

MartyIX commented Mar 26, 2024

View reviewed changes

jsuarezruiz added legacy-area-perf Startup / Runtime performance area-gestures Gesture types labels Mar 26, 2024

jonathanpeppers reviewed Mar 26, 2024

View reviewed changes

MartyIX added 3 commits March 26, 2024 19:38

Do not create lambdas + apply MSVS suggestion regarding pattern match…

39b7ec6

…ing + add braces

Make HasAnyGesturesFor internal

edd66ef

Reimplement by adding FirstGestureOrDefault

acadf48

jonathanpeppers approved these changes Mar 27, 2024

View reviewed changes

rmarinho approved these changes Apr 6, 2024

View reviewed changes

rmarinho merged commit 0c02e4b into dotnet:main Apr 6, 2024

github-actions Bot locked and limited conversation to collaborators May 9, 2024

Eilon added perf/general The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf) and removed legacy-area-perf Startup / Runtime performance labels May 10, 2024

MartyIX deleted the feature/2024-03-26-gestures-less-allocations-final branch May 24, 2024 08:43

samhouts added fixed-in-8.0.40 fixed-in-9.0.0-preview.4.10690 labels Aug 2, 2024

Conversation

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

Uh oh!

Description of Change

Measurements

Results

Issues Fixed

Uh oh!

dotnet-policy-service Bot commented Mar 26, 2024

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartyIX commented Mar 26, 2024

Uh oh!

jsuarezruiz commented Mar 26, 2024

Uh oh!

azure-pipelines Bot commented Mar 26, 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!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

jonathanpeppers commented Mar 26, 2024

Uh oh!

azure-pipelines Bot commented Mar 26, 2024

Uh oh!

jonathanpeppers left a comment

Choose a reason for hiding this comment

Uh oh!

MartyIX commented Mar 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

MartyIX commented Mar 26, 2024 •

edited

Loading

MartyIX Mar 26, 2024 •

edited

Loading

MartyIX Mar 26, 2024 •

edited

Loading

MartyIX Mar 26, 2024 •

edited

Loading

MartyIX commented Mar 26, 2024 •

edited

Loading