maui icon indicating copy to clipboard operation
maui copied to clipboard

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

Open MartyIX opened this issue 2 years ago • 4 comments

Description of Change

Notes:

  • Clicking "Batch Generate Grid" multiple times lead to ever increasing time to finish the operation. I'm not sure why.

  • Relevant issue https://github.com/dotnet/maui/issues/17303

  • This PR is mostly a bait to attract the MAUI team's attention to the problem that it seems it's not that hard to improve status quo substantially. However, my discussion with @jonathanpeppers[^1] somewhat suggests that the approach of this PR is not the best (mediocre at best I guess):

    that [this PR] would help Windows but same issue on all platforms, I think maybe this could be a redesign thing for future, like .NET 9

    me:

    https://github.com/dotnet/maui/blob/0a562dcd1de72db1d52426fdd89c29e02fb00f1b/src/Core/src/Handlers/View/ViewHandler.Android.cs#L75 - so it differs on android https://github.com/dotnet/maui/blob/main/src/Core/src/Handlers/View/ViewHandler.iOS.cs - all the same https://github.com/dotnet/maui/blob/main/src/Core/src/Handlers/View/ViewHandler.Windows.cs - all the same https://github.com/dotnet/maui/blob/main/src/Core/src/Handlers/View/ViewHandler.Tizen.cs - all the same So it seems like Android can be an exception and others would just handle one of these transform properties

    Jonathan:

    The part that makes me want to rethink this:

    1. Every control has TranslateX/Y default to 0
    2. 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

Master

851 ms

image

PR:

676 ms

image

-> 20% faster.

Issues Fixed

Contributes to #21787

[^1]: I hope he doesn't mind me copying it here.

MartyIX avatar Apr 13 '24 19:04 MartyIX

@PureWeen What do you think please?

MartyIX avatar Apr 13 '24 19:04 MartyIX

This looks interesting! performance should be on top of the list, and changes like this, as well as to other platforms should be really a priority. Since it is in the core, would love to see this going forward. Not developing anything for Windows, but super interested for Android and iOS

bcaceiro avatar Apr 14 '24 09:04 bcaceiro

  • Clicking "Batch Generate Grid" multiple times lead to ever increasing time to finish the operation. I'm not sure why.

@AdamEssenmacher Can the slowdown be caused by #21809 please?

MartyIX avatar Apr 14 '24 17:04 MartyIX

@MartyIX I don't think so. Your Labels are getting GC'ed.

It does seem like there's a memory leak in this sample though, so the slowdown could be related to that in some way. It's just not the propagating leak I describe in #21809

I'll further observe that the major leak appearing in this sample goes away and the time to run each batch becomes very consistent if you modify the batch generation method like:

	private async void BatchGenerate_Clicked(object sender, EventArgs e)
	{
		Stopwatch sw = Stopwatch.StartNew();		

		int batchSize = int.Parse(BatchSize.Text);

		for (int i = 0; i < batchSize; i++)
		{
			contentGrid.Clear();

			for (int rowIndex = 0; rowIndex < rowCount; rowIndex++)
			{
				for (int columnIndex = 0; columnIndex < columnCount; columnIndex++)
				{
					Label label = new Label() { Text = $"[{columnIndex}x{rowIndex}]" };
					contentGrid.Add(label, column: columnIndex, row: rowIndex);
				}
			}

			await Task.Delay(500);
		}

		sw.Stop();
		info.Text = $"Grid was created {batchSize} times and it took {sw.ElapsedMilliseconds} ms in total. Avg run took {Math.Round(sw.ElapsedMilliseconds / (double)batchSize, 2)} ms";
	}

So, I suspect this batch test has something else entirely going on, most likely related to UI elements being added to the visual tree that never get a chance to be fully flushed out to the screen.

AdamEssenmacher avatar Apr 14 '24 21:04 AdamEssenmacher

/azp run

jsuarezruiz avatar Apr 22 '24 11:04 jsuarezruiz

Azure Pipelines successfully started running 3 pipeline(s).

azure-pipelines[bot] avatar Apr 22 '24 11:04 azure-pipelines[bot]