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.

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

Measure invalidation performance and fixes (batching version) #24823

Conversation

albyrock87
Copy link
Contributor

@albyrock87 albyrock87 commented Sep 18, 2024

Description of Change

This is #24532 plus batching of measure invalidation.

I executed a speedscope on davidortinau/AllTheLists LearningPage using the latest nightly 8.0.99-ci.net8.24468.1 build.

What I noticed is that #23052 OnChildMeasureInvalidatedInternal was showing up and taking 2% of total time (recursive calls).

What I've done:

  • Highly increases general performance when changing BindingContext on attached nodes by batching layout invalidation during binding context change & propagation
    • This is covered by a new suite of unit tests
    • This makes finally predictable the layout invalidation when BindingContext changes, from the leaf to the root
  • Avoids triggering measure invalidated completely while building the MAUI tree in memory (Handler = null)
  • Fixes some iOS SetNeedsLayout propagation issues and cleans up detection of auto SetNeedsLayout parent propagation with IPropagatesSetNeedsLayout internal interface
  • Removes "out-of-layout-pass" measurements from legacy layouts making them fast as new layouts

WinUI Speedscope

Kindly provided by @MartyIX

Before main
image
1726728202.MAIN.speedscope.json

After PR
image
1726728309.PR.speedscope.json

Before main
image
image
1726728235.MAIN.speedscope.json

After PR
image
image
1726728360.PR.speedscope.json

Android Speedscope

Before main
image
android-before.speedscope.json

After PR
image
after-android.speedscope.json.zip

iOS Speedscope

Before main
image
image
before.speedscope.json.zip

After PR
image
image
after.speedscope.json.zip

Before main

You can notice there are some hiccups while scrolling.

before.mp4

After PR

You can notice there hiccups are basically gone while scrolling, even if in this case I had a lot more nested children.

after.mp4

Issues Fixed

Fixes #24551

@albyrock87 albyrock87 requested a review from a team as a code owner September 18, 2024 18:43
@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Sep 18, 2024
frame = control.Frame;
desiredSize = control.DesiredSize;
Assert.Equal(5, frame.X, 0.5d);
Assert.Equal(60, frame.Width, 0.5d);
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 was failing on ListView and TableView due to the fact they were not propagating SetNeedsLayout to the parent.

@OvrBtn
Copy link

OvrBtn commented Sep 18, 2024

Thank you so much for your work, can't wait to try it out!

Do you think or have you tested if it will also affect performance of rendering pages when navigating/performing first render? Currently navigation can be a lot slower than Xamarin in certain cases, sometimes even with static pages without binding and collections.

@albyrock87
Copy link
Contributor Author

@OvrBtn it improves that a bit if you're using ContentView or legacy layouts.
I have other ideas to improve that scenario, but I didn't want to increase an already big PR.

@albyrock87 albyrock87 changed the title Measure invalidation performance and fixes Measure invalidation performance and fixes (batching version) Sep 18, 2024
@PureWeen
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@bcaceiro
Copy link

@albyrock87 great work! What about android, which seems to be the slowest platform overall so far in .net maui, have you seen even bigger improvements?

@albyrock87
Copy link
Contributor Author

albyrock87 commented Sep 20, 2024

@bcaceiro I'm not sure how, but in this CollectionView-related test, Android seems very fast compared to iOS and in fact I'm not able to see relevant differences (I've added speedscopes above).
I remember Android being very slow too, so I'm under the impression that latest service releases brought major improvements on that platform.

namespace Microsoft.Maui.Controls.Internals;

[Flags]
internal enum InvalidationTriggerFlags : ushort
Copy link
Contributor Author

Choose a reason for hiding this comment

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

InvalidationTrigger is a public API but at the same time it says

For internal use by platform renderers.

So I wonder if I could simply change that to:

[Flags]
public enum InvalidationTrigger : ushort
{
	None = 0,
	BatchingBindingContext = 1 << 0,
    // leaving empty space here in case we want to add other types of batching
 	NotifyOnChildMeasureInvalidated = 1 << 4,
	HorizontalOptionsChanged = 1 << 5,
	VerticalOptionsChanged = 1 << 6,
	MarginChanged = 1 << 7,
	SizeRequestChanged = 1 << 8,
	MeasureChanged = 1 << 9,
	RendererReady = 1 << 10,
	Undefined = 1 << 11,
}

@rmarinho rmarinho requested review from jonathanpeppers and PureWeen and removed request for tj-devel709 September 24, 2024 11:15
@albyrock87 albyrock87 force-pushed the improve-measure-invalidation-with-batch-measure-invalidation branch 2 times, most recently from bef003d to 4429238 Compare October 18, 2024 20:32
@PureWeen
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@albyrock87
Copy link
Contributor Author

  • iOS failing tests are related to CollectionViewHandler2
  • @PureWeen may you rerun only failed Jobs on other platforms?

albyrock87 added a commit to nalu-development/maui-custom that referenced this pull request Oct 19, 2024
albyrock87 added a commit to nalu-development/maui-custom that referenced this pull request Oct 19, 2024
@jsuarezruiz jsuarezruiz added the t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf) label Oct 30, 2024
@jsuarezruiz
Copy link
Contributor

/rebase

@github-actions github-actions bot force-pushed the improve-measure-invalidation-with-batch-measure-invalidation branch from 4429238 to c0057a0 Compare October 30, 2024 07:08
@jsuarezruiz
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen
Copy link
Member

PureWeen commented Nov 1, 2024

/rebase

@albyrock87
Copy link
Contributor Author

Closing in favor of #25664

@albyrock87 albyrock87 closed this Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community ✨ Community Contribution do-not-merge Don't merge this PR t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve measure invalidation and legacy Layout(s) performance
5 participants