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

Improve performance and stability of BindableLayout #23136

Merged
merged 4 commits into from
Sep 6, 2024

Conversation

albyrock87
Copy link
Contributor

@albyrock87 albyrock87 commented Jun 19, 2024

Description of Change

This PR improves the performance of BindableLayout in all cases where the item source changes entirely or there is a Reset or Replace event (see more on #23135).
The strategy is simple: reuse existing views when possible, and simply change the BindingContext.

Video - Before

before

Video - After

after

Issues Fixed

Fixes #23135

@albyrock87 albyrock87 requested a review from a team as a code owner June 19, 2024 12:47
@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Jun 19, 2024
@davidortinau
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen PureWeen added the area-layout StackLayout, GridLayout, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter label Jun 20, 2024
@PureWeen
Copy link
Member

/rebase

@PureWeen
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@MartyIX
Copy link
Contributor

MartyIX commented Aug 18, 2024

https://github.com/dotnet/maui/pull/23136/checks?check_run_id=28884287267 mentions a test failure that might be a real change in behavior:

Microsoft.Maui.TestCases.Tests.Issues.Issue22417(Windows).AddItemsToCarouselViewWorks

VisualTestUtils.VisualTestFailedException :
Snapshot different than baseline: AddItemsToCarouselViewWorks.png (20.50% difference)
If the correct baseline has changed (this isn't a a bug), then update the baseline image.
See test attachment or download the build artifacts to get the new snapshot file.

More info: https://aka.ms/visual-test-workflow

@albyrock87
Copy link
Contributor Author

@MartyIX thanks for point it out, there was indeed an issue which I fixed in 997079d but it was related to another failing test.

The one you're talking about seems to be a different issue given that Carousel test is not using a BindableLayout.

@PureWeen
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@MartyIX
Copy link
Contributor

MartyIX commented Aug 19, 2024

Now I can see this test to fail:

BindableLayout Does Not Leak
Assert.Equal() Failure: Values differ\r\nExpected: 6\r\nActual:   3

Not sure if it is a real issue but looks like it can.

@albyrock87
Copy link
Contributor Author

Thanks @MartyIX for keeping an eye on this.
The test had to be updated to account for the enhanced behavior: baa4aa2
No real memory leaks fortunately ;)

@PureWeen
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@albyrock87
Copy link
Contributor Author

Faling unrelated tests:

  • Core.UnitTests (Android) UnitTests.NavigationPageLifecycleTests.RemoveInnerPage(true)
  • Controls.TestCases.Shared.Tests (Android): RegisterTwoDoubleTapHandlersAndCheckNumberOfFiredEventsAsync
  • Controls.DeviceTests (iOS): HandlerDoesNotLeak(typeof(WebView))

@MartyIX
Copy link
Contributor

MartyIX commented Aug 26, 2024

Tests appear to be green now.

@PureWeen
Copy link
Member

PureWeen commented Sep 5, 2024

/rebase

@PureWeen
Copy link
Member

PureWeen commented Sep 5, 2024

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen PureWeen enabled auto-merge (squash) September 6, 2024 14:34
@PureWeen PureWeen merged commit bcba26f into dotnet:main Sep 6, 2024
97 checks passed
@cagriy
Copy link

cagriy commented Sep 26, 2024

I see a change in the BindableLayout behaviour 8.0.90, which may or may not be related to this, but I wanted to report it:
I update BindableLayout using

BindableLayout.SetItemsSource(MyStackLayout, MyListOfObjects);

Before the update, the layout was redrawn and reapplied the settings (in my case colors) that were hardcoded in XAML, after the update, the content gets updated, but the changes to layout items (such as colors) persist.

I managed to resolve the issue by adding a

MyStackLayout.Children.Clear();

before setting the item source. I just wanted to report it in case it was an unintentional breaking change.

@albyrock87
Copy link
Contributor Author

@cagriy you're correct, this was an intentional breaking change, and your workaround is correct.

BindableLayout is now trying to reuse the views as much as possible considering that creating a platform view from scratch has higher cost than changing a few binded property.

One thing you could do instead of the Clear to benefit from this change is override OnBindingContextChanged in your custom view and reset the colors there when BindingContext is not null.

@cagriy
Copy link

cagriy commented Sep 26, 2024

@albyrock87 , that's fantastic, and thanks for the quick reply, would it be a good idea to move this change into the breaking changes section so that others won't need to troubleshoot the same issue?

@samhouts samhouts added the fixed-in-net9.0-nightly This may be available in a nightly release! label Oct 1, 2024
@samhouts samhouts added fixed-in-9.0.0-rc.2.24503.2 and removed fixed-in-net9.0-nightly This may be available in a nightly release! labels Oct 14, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Nov 16, 2024
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 community ✨ Community Contribution fixed-in-9.0.0-rc.2.24503.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BindableLayout performance could be better
6 participants