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

Jump to bottom

Improves performance of GetLayoutHandlerIndex #18499

Merged
merged 1 commit into from
Nov 6, 2023
Merged

Improves performance of GetLayoutHandlerIndex #18499

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 22 additions & 2 deletions src/Core/src/Handlers/Layout/LayoutExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,34 @@ internal static class LayoutExtensions

public static int GetLayoutHandlerIndex(this ILayout layout, IView view)
mattleibow marked this conversation as resolved.
Show resolved Hide resolved
{
switch (layout.Count)
var count = layout.Count;
switch (count)
{
case 0:
return -1;
case 1:
return view == layout[0] ? 0 : -1;
default:
return layout.OrderByZIndex().IndexOf(view);
var found = false;
Copy link
Contributor

@espenrl espenrl Nov 4, 2023 •
edited

Choose a reason for hiding this comment

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

A suggestion to improve code readability and optimum performance is to use MemoryExtensions.Sort and MemoryExtensions.BinarySearch - with a custom comparer to sort by ZIndex. If one can't sort layout in place one could stackalloc a buffer array to avoid using the heap.

Another idea would be for the layout itself to keep a sorted list of the children.

Copy link
Contributor Author

@albyrock87 albyrock87 Nov 4, 2023 •
edited

Choose a reason for hiding this comment

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

In this case any kind of sort is still less performant than O(N).

A SortedList though would be beneficial for both methods in this class, but the problem is that both zIndex and childIndex might change in time (i.e. adding or removing children).

I'm thinking about a more general solution for the long term, but I felt this was a low hanging fruit for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's true that the total thing would be O(N log N) which really isn't much more than O(N) for the low numbers found in layout children. My guess it's probably the allocations of the original code that makes it slower. The sorting itself could in fact be negligible.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do think your code is optimal, I was mainly commenting on readability.

var zIndex = view.ZIndex;
var lowerViews = 0;

for (int i = 0; i < count; i++)
{
var child = layout[i];
var childZIndex = child.ZIndex;

if (child == view) {
found = true;
mattleibow marked this conversation as resolved.
Show resolved Hide resolved
}

if (childZIndex < zIndex || !found && childZIndex == zIndex)
{
++lowerViews;
}
}

return found ? lowerViews : -1;
}
}
}
Expand Down
1 change: 1 addition & 0 deletions src/Core/src/Properties/AssemblyInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
[assembly: InternalsVisibleTo("Microsoft.Maui.Controls.DeviceTests")]
[assembly: InternalsVisibleTo("Microsoft.Maui.Controls.Core.UnitTests")]
[assembly: InternalsVisibleTo("Microsoft.Maui.Benchmarks.Droid")]
[assembly: InternalsVisibleTo("Core.Benchmarks")]
[assembly: InternalsVisibleTo("Comet")]
[assembly: InternalsVisibleTo("Comet.Tests")]
[assembly: InternalsVisibleTo("CommunityToolkit.Maui")]
Expand Down
31 changes: 31 additions & 0 deletions src/Core/tests/Benchmarks/Benchmarks/LayoutBenchmarker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
using BenchmarkDotNet.Attributes;
using Microsoft.Maui.Controls;
using Microsoft.Maui.Handlers;

namespace Microsoft.Maui.Benchmarks
{
[MemoryDiagnoser]
public class LayoutBenchmarker
{
static readonly Border[] Views = new[]
{
new Border(), new Border(), new Border(), new Border(), new Border(), new Border(), new Border(),
new Border(), new Border(), new Border(), new Border(), new Border(), new Border(), new Border()
};

static readonly int Iterations = Views.Length;

[Benchmark]
public void GetLayoutHandlerIndex()
{
var layout = new VerticalStackLayout();

for (int i = 0; i < Iterations; i++)
{
var view = Views[i];
layout.Add(view);
layout.GetLayoutHandlerIndex(view);
}
}
}
}
35 changes: 35 additions & 0 deletions src/Core/tests/UnitTests/Layouts/ZIndexTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,41 @@ public void LayoutHandlerIndexFollowsAddOrderWhenZIndexesAreEqual()
Assert.Equal(2, layout.GetLayoutHandlerIndex(view2));
Assert.Equal(3, layout.GetLayoutHandlerIndex(view3));
}

[Fact]
public void LayoutHandlerIndexIsNegativeWhenChildIsNotFound()
{
var layout = new FakeLayout();
var view0 = CreateTestView(zIndex: 0);

Assert.Equal(-1, layout.GetLayoutHandlerIndex(view0));

layout.Add(CreateTestView(zIndex: 0));
Assert.Equal(-1, layout.GetLayoutHandlerIndex(view0));


layout.Add(CreateTestView(zIndex: 0));
Assert.Equal(-1, layout.GetLayoutHandlerIndex(view0));
}

[Fact]
public void LayoutHandlerIndexPreservesAddOrderForEqualZIndexes()
{
var layout = new FakeLayout();
var view0 = CreateTestView(zIndex: 10);
var view1 = CreateTestView(zIndex: 10);
var view2 = CreateTestView(zIndex: 10);
var view3 = CreateTestView(zIndex: 5);
layout.Add(view0);
layout.Add(view1);
layout.Add(view2);
layout.Add(view3);

Assert.Equal(1, layout.GetLayoutHandlerIndex(view0));
Assert.Equal(2, layout.GetLayoutHandlerIndex(view1));
Assert.Equal(3, layout.GetLayoutHandlerIndex(view2));
Assert.Equal(0, layout.GetLayoutHandlerIndex(view3));
}

[Fact]
public void ItemsOrderByZIndex()
Expand Down