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

[CollectionView] Review Strategies to reduce Child Measure Propagation #25391

Open
PureWeen opened this issue Oct 18, 2024 · 10 comments · May be fixed by #25664
Open

[CollectionView] Review Strategies to reduce Child Measure Propagation #25391

PureWeen opened this issue Oct 18, 2024 · 10 comments · May be fixed by #25664
Labels
area-controls-collectionview CollectionView, CarouselView, IndicatorView s/triaged Issue has been reviewed t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf)

Comments

@PureWeen
Copy link
Member

Description

#23052 fixed some serious issues with iOS but it would ideal to dial this back some in scenarios where it's not necessary to bubble up invalidation events.

For example #25264

MeasureFirstItem

  • On a CollectionView if the measure strategy is MeasureFirstItem we never need to propagate MeasureInvalidated up from the templated views after the first measure has happened. If the user needs cells to resize they need to invalidate the measure on the CV themselves

Contraints

  • If views are constrained in a way that the height and width will never change then don't propagate up
  • Give users a way to fix the constraints on a view so it never remeasures after the first measure
@PureWeen PureWeen added area-controls-collectionview CollectionView, CarouselView, IndicatorView t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf) labels Oct 18, 2024
@PureWeen PureWeen added this to the .NET 9 Servicing milestone Oct 18, 2024
@dotnet-policy-service dotnet-policy-service bot added the s/triaged Issue has been reviewed label Oct 18, 2024
@jonmdev
Copy link

jonmdev commented Oct 20, 2024

Would all these changes possibly solve this issue where we are getting endless remeasures and arranges on translation change in iOS?

#24996

This seems like more a general issue than just specific to CollectionView, since it is happening everywhere in the Layout system, no?

@albyrock87 I see you have been digging into the Layout system quite heavily and you always have good ideas. Any thoughts?

If you are developing "custom layout update modes" for scrolling and other objects, the it would be important for us to have access to enabling those on our own Layout objects as well. I observe no such "special modes" are needed in Xamarin, because the layout system just works properly.

Thanks for any thoughts and your continued hard work.

@albyrock87
Copy link
Contributor

I'll try to dig more into the issue you reported @jonmdev . Just know that before SR9 the MeasureInvalidated event was not being propagated to ancestors if not using legacy layouts.
We can continue the discussion in the issue you opened, let me just ask, are you using any of the legacy layouts in your collection view? This includes ContentView.
If you are, may you try to replace those legacy layouts with new ones like VerticalStackLayout or the new Grid and let me know if the endless re-layout is gone?

@jonmdev
Copy link

jonmdev commented Oct 20, 2024

I'll try to dig more into the issue you reported @jonmdev . Just know that before SR9 the MeasureInvalidated event was not being propagated to ancestors if not using legacy layouts. We can continue the discussion in the issue you opened, let me just ask, are you using any of the legacy layouts in your collection view? This includes ContentView. If you are, may you try to replace those legacy layouts with new ones like VerticalStackLayout or the new Grid and let me know if the endless re-layout is gone?

Okay let's continue that discussion in the other thread, but to clarify, none of "ContentView", "VerticalStackLayout" or "Grid" or "ScrollView" were used. Only AbsoluteLayout which is still a necessary Layout method as it is the simplest and purest one for manual layouts. We have no other option for manual and simple layouts, that I know of.

I don't use any "ContentView", "VerticalStackLayout" or "Grid" or "ScrollView" in any of my projects as I just manage my own Layouts using AbsoluteLayout.

I linked the code I used in that thread so you can see again. Thanks as always.

@FlavioGoncalves-Cayas
Copy link

Please revert the changes. What took <0.1 seconds with a MAUI-Version prior to 8.0.9x now takes more than 1 second.

@albyrock87
Copy link
Contributor

Please revert the changes. What took <0.1 seconds with a MAUI-Version prior to 8.0.9x now takes more than 1 second.

@FlavioGoncalves-Cayas would you be able to create a how-to-repro to demonstrate this huge slow down?

I think that'd help to better understand the impact and prioritize a fix.

I already have a few PRs in place to fix/improve this.

@FlavioGoncalves-Cayas
Copy link

FlavioGoncalves-Cayas commented Oct 29, 2024

Here is a rudimentary repro: https://github.com/FlavioGoncalves-Cayas/InvalidateMeasureIssueRepro

The layout in the production app where I stumbled upon the issue is more complex, so that the impact is more noticeable. I tried to compensate for it by adding more items to the bindablelayout in the repro.

The custom grid is my attempt at trying to measure how long the measure takes.

I am not planning to spend an awful lot of time on an accurate reproduction.

EDIT: The main problem I see is that all items are measured once again even when the single item where the change occurred did not even change its height

@FlavioGoncalves-Cayas
Copy link

I just pushed a change to the repro, as I was not getting a different behavior between Android and iOS.
Now you can see that the Android app does not remeasure all items. But the iOS version still does. However, the difference between 8.0.80 and 8.0.92 is not that noticeable. Maybe there is another thing at play here.
The issue I am having is maybe also not as closely related to this one as I initially thought. But still, something is not right with measuring on iOS. As it is still bubbling up measurement invalidation, although there are fixed sizes in play.

@albyrock87
Copy link
Contributor

@FlavioGoncalves-Cayas that's what I thought.
8.0.9x introduces bubbling up of measure invalidation, which should not cause re-layout except for nodes based on legacy layouts like ContentView or the old StackLayout or ScrollView.

So the impact should not be that huge.

Regarding native iOS invalidation topic, this has never been that smart, I have a PR opened to do an initial refactor which may allow us to make it smart in the near future.

#25465

@FlavioGoncalves-Cayas
Copy link

@albyrock87
Is there any way to prevent the bubbling up of measure invalidation, when I know that a control does not need it?
I'm afraid I cannot wait for the changes to be made. Especially when they will only come with .NET 9.

@albyrock87
Copy link
Contributor

We're proposing this #25324 to be included in a very small .NET8 service release.
It's basically a revert of the original PR hidden behind a flag.
What's obvious is that disabling the bubble up will break (again) iOS CollectionView item size change detection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-controls-collectionview CollectionView, CarouselView, IndicatorView s/triaged Issue has been reviewed t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) (sub: perf)
Projects
Status: Todo
4 participants