Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WinUI] Allocate less when updating gestures #21450

Merged
merged 7 commits into from
Apr 6, 2024
Merged

[WinUI] Allocate less when updating gestures #21450

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
acabea4
`EnumerableExtensions`: Enable nullable
MartyIX Mar 26, 2024
19d7b69
Add `EnumerableExtensions.HasAnyGesturesFor`
MartyIX Mar 26, 2024
e984695
Sandbox measure code
MartyIX Mar 26, 2024
1b3612b
Revert "Sandbox measure code"
MartyIX Mar 26, 2024
39b7ec6
Do not create lambdas + apply MSVS suggestion regarding pattern match…
MartyIX Mar 26, 2024
edd66ef
Make `HasAnyGesturesFor` `internal`
MartyIX Mar 26, 2024
acadf48
Reimplement by adding `FirstGestureOrDefault`
MartyIX Mar 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 30 additions & 11 deletions src/Controls/src/Core/EnumerableExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,16 @@
#nullable disable
using System;
using System.Collections.Generic;
using System.ComponentModel;

namespace Microsoft.Maui.Controls.Internals
{
static class EnumerableExtensions
{
public static bool HasChildGesturesFor<T>(this IEnumerable<GestureElement> elements, Func<T, bool> predicate = null) where T : GestureRecognizer
public static bool HasChildGesturesFor<T>(this IEnumerable<GestureElement>? elements, Func<T, bool>? predicate = null) where T : GestureRecognizer
{
if (elements == null)
if (elements is null)
return false;

if (predicate == null)
if (predicate is null)
predicate = x => true;

foreach (var element in elements)
Expand All @@ -26,12 +24,12 @@ public static bool HasChildGesturesFor<T>(this IEnumerable<GestureElement> eleme
return false;
}

public static IEnumerable<T> GetChildGesturesFor<T>(this IEnumerable<GestureElement> elements, Func<T, bool> predicate = null) where T : GestureRecognizer
public static IEnumerable<T> GetChildGesturesFor<T>(this IEnumerable<GestureElement>? elements, Func<T, bool>? predicate = null) where T : GestureRecognizer
{
if (elements == null)
if (elements is null)
yield break;

if (predicate == null)
if (predicate is null)
predicate = x => true;

foreach (var element in elements)
Expand All @@ -43,12 +41,13 @@ public static IEnumerable<T> GetChildGesturesFor<T>(this IEnumerable<GestureElem
}
}

public static IEnumerable<T> GetGesturesFor<T>(this IEnumerable<IGestureRecognizer> gestures, Func<T, bool> predicate = null) where T : GestureRecognizer
/// <remarks>The method makes a defensive copy of the gestures.</remarks>
public static IEnumerable<T> GetGesturesFor<T>(this IEnumerable<IGestureRecognizer>? gestures, Func<T, bool>? predicate = null) where T : GestureRecognizer
{
if (gestures == null)
if (gestures is null)
yield break;

if (predicate == null)
if (predicate is null)
predicate = x => true;

foreach (IGestureRecognizer item in new List<IGestureRecognizer>(gestures))
Expand All @@ -60,5 +59,25 @@ public static IEnumerable<T> GetGesturesFor<T>(this IEnumerable<IGestureRecogniz
}
}
}

public static bool HasAnyGesturesFor<T>(this IEnumerable<IGestureRecognizer>? gestures, Func<T, bool>? predicate = null) where T : GestureRecognizer
{
Copy link
Contributor Author

@MartyIX MartyIX Mar 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to avoid allocation on L53.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make the new method(s) internal?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in edd66ef.

if (gestures is null)
{
return false;
}

predicate ??= x => true;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 39b7ec6.

foreach (IGestureRecognizer item in gestures)
{
if (item is T gesture && predicate(gesture))
{
return true;
}
}

return false;
}
}
}
32 changes: 19 additions & 13 deletions src/Controls/src/Core/Platform/GestureManager/GesturePlatformManager.Windows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -670,28 +670,32 @@ void PinchComplete(bool success)

void UpdateDragAndDropGestureRecognizers()
{
if (_container == null)
if (_container is null)
{
return;
}

var view = Element as View;
IList<IGestureRecognizer>? gestures = view?.GestureRecognizers;

if (gestures == null)
if (gestures is null)
{
return;
}

_container.CanDrag = gestures.GetGesturesFor<DragGestureRecognizer>()
.FirstOrDefault()?.CanDrag ?? false;
bool canDrag = gestures.GetGesturesFor<DragGestureRecognizer>().FirstOrDefault()?.CanDrag ?? false;
Copy link
Contributor Author

@MartyIX MartyIX Mar 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

_container.CanDrag = canDrag;

_container.AllowDrop = gestures.GetGesturesFor<DropGestureRecognizer>()
.FirstOrDefault()?.AllowDrop ?? false;
bool allowDrop = gestures.GetGesturesFor<DropGestureRecognizer>().FirstOrDefault()?.AllowDrop ?? false;
_container.AllowDrop = allowDrop;

if (_container.CanDrag)
if (canDrag)
{
_container.DragStarting += HandleDragStarting;
_container.DropCompleted += HandleDropCompleted;
}

if (_container.AllowDrop)
if (allowDrop)
{
_container.DragOver += HandleDragOver;
_container.Drop += HandleDrop;
Expand All @@ -714,7 +718,7 @@ void UpdatingGestureRecognizers()
IList<TapGestureRecognizer>? childGestures =
children?.GetChildGesturesFor<TapGestureRecognizer>().ToList();

if (gestures.GetGesturesFor<TapGestureRecognizer>(g => g.NumberOfTapsRequired == 1).Any()
if (gestures.HasAnyGesturesFor<TapGestureRecognizer>(g => g.NumberOfTapsRequired == 1)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should save 1 allocation.

|| children?.GetChildGesturesFor<TapGestureRecognizer>(g => g.NumberOfTapsRequired == 1).Any() == true)
{
_container.Tapped += OnTap;
Expand All @@ -728,7 +732,7 @@ void UpdatingGestureRecognizers()
}
}

if (gestures.GetGesturesFor<TapGestureRecognizer>(g => g.NumberOfTapsRequired == 1 || g.NumberOfTapsRequired == 2).Any()
if (gestures.HasAnyGesturesFor<TapGestureRecognizer>(g => g.NumberOfTapsRequired == 1 || g.NumberOfTapsRequired == 2)
|| children?.GetChildGesturesFor<TapGestureRecognizer>(g => g.NumberOfTapsRequired == 1 || g.NumberOfTapsRequired == 2).Any() == true)
{
_container.DoubleTapped += OnTap;
Expand All @@ -747,11 +751,13 @@ void UpdatingGestureRecognizers()
_container.PointerPressed += OnPgrPointerPressed;
_container.PointerReleased += OnPgrPointerReleased;

bool hasSwipeGesture = gestures.GetGesturesFor<SwipeGestureRecognizer>().GetEnumerator().MoveNext();
bool hasPinchGesture = gestures.GetGesturesFor<PinchGestureRecognizer>().GetEnumerator().MoveNext();
bool hasPanGesture = gestures.GetGesturesFor<PanGestureRecognizer>().GetEnumerator().MoveNext();
bool hasSwipeGesture = gestures.HasAnyGesturesFor<SwipeGestureRecognizer>();
bool hasPinchGesture = gestures.HasAnyGesturesFor<PinchGestureRecognizer>();
bool hasPanGesture = gestures.HasAnyGesturesFor<PanGestureRecognizer>();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 allocations here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@MartyIX MartyIX Mar 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (!hasSwipeGesture && !hasPinchGesture && !hasPanGesture)
{
return;
}

//We can't handle ManipulationMode.Scale and System , so we don't support pinch/pan on a scrollview
if (Element is ScrollView)
Expand Down
Loading