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

[Android] Changes to reduce unnecessary layout requests #4229

Merged
merged 3 commits into from
Jan 27, 2022
Merged

Conversation

jsuarezruiz
Copy link
Contributor

Description of Change

Changes to RequestLayout in Layouts only when needed.

Testing with:
PerfTests.zip

Before:
perf01

After:
perf02

Notice that the onLayout and onMeasure time (green light color) has been greatly reduced.

NOTE: the gifs have been optimized to have a small size, the quality is low.

@hartez Could we take a look to this together?

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)

Does this PR touch anything that might affect accessibility?

  • No

@jsuarezruiz jsuarezruiz added area-layout StackLayout, GridLayout, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter platform/android 🤖 labels Jan 20, 2022
@jsuarezruiz jsuarezruiz added the partner Issue or Request from a partner team label Jan 20, 2022
@jsuarezruiz
Copy link
Contributor Author

Caching Frame in VisualElement and with some local variables and comparisons to check if Frame (size andposition) has changed slightly improves Measure ticks. To avoid having a too complex PR I have decided to splitit into two blocks where the one that provides the most perf benefits would be this one.

@jsuarezruiz
Copy link
Contributor Author

Need to review but could have an impact in #4149

@jsuarezruiz
Copy link
Contributor Author

jsuarezruiz commented Jan 21, 2022

Made more changes using https://github.com/jsuarezruiz/netmaui-chat-app-challenge as reference.

Really many more Layout invalidations are made than necessary. With this PR we pass from:

perf-sample-issue
perf-sample-issue

Affecting the scrolling of listings, navigation, etc. To:

perf-sample-issue2

Where the performance is more similar to Xamarin.Forms. However, we have a Measure loop located in the ScrollViewHandler. I leave those changes for another PR, although they are necessary.

Copy link
Contributor

@hartez hartez left a comment

Choose a reason for hiding this comment

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

I'm worried that the caching of the ScrollView size (and not measuring the content) will cause weird problems later.

Everything else looks good.

public override Size GetDesiredSize(double widthConstraint, double heightConstraint)
{
var result = base.GetDesiredSize(widthConstraint, heightConstraint);

if (FindInsetPanel(this) == null)
if (result != _previousDesiredSize && FindInsetPanel(this) == null)
Copy link
Contributor

Choose a reason for hiding this comment

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

If the measure/layout pass has been triggered by something in the ScrollView content, not measuring it here could mean that it doesn't get updated/redrawn properly. I think we should avoid adding this caching here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, this have a small impact, but let me review alternatives. If not, we can remove this, merge and continue improving in another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hartez Could you review it again?. Thanks.

@hartez hartez merged commit e2767b2 into main Jan 27, 2022
@hartez hartez deleted the layout-perf branch January 27, 2022 17:47
@github-actions github-actions bot locked and limited conversation to collaborators Dec 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-layout StackLayout, GridLayout, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter partner Issue or Request from a partner team platform/android 🤖
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants