-
Notifications
You must be signed in to change notification settings - Fork 29.7k
[New feature][Performance] Introduce a caching mechanism to ListView to reduce unnecessary layout during scrolling #134864
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
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie or stuartmorgan on the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
be9a4f7 to
91aaa78
Compare
94310b2 to
f6111f8
Compare
f6111f8 to
4baf71b
Compare
Piinks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mainly thoughts and questions here first, I am still wrapping my head around this and all the possible edge cases. I am very excited to see this though! :)
| /// If true, the main axis extent of child will be cached as | ||
| /// index/extent pairs when the child first laid out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /// If true, the main axis extent of child will be cached as | |
| /// index/extent pairs when the child first laid out. | |
| /// If true, the main axis extent of the child will be cached as | |
| /// index/extent pairs when the child is first laid out. |
| /// previously cached data will be cleared. | ||
| /// | ||
| /// The cached data will be used to efficiently determine the target children | ||
| /// within the lazy loading scope, which helps improve layout performance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be some way for the developer to invalidate the cache? Such as changing a child key, reordering children in the list, adding/removing children from the list, etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point!
I considered exposing a property named like "Set invalidCachedItemExtents" to let the render object delete the corresponding cached data.
But this seems to lack grace and cannot invalidate all cached values easily. Do you have any good suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, we could rely on the SliverChildDelegate.shouldRebuild to help invalidate the cache. In the SliverChildListDelegate that would work, but in the SliverChildBuilderDelegate it would always invalidate.. Maybe @chunhtai has some thoughts on this. Particularly with the child reordering edge cases, I think @chunhtai has some familiarity. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure how this would work, even if the SliverChildListDelegate doesn't change, the child itself can still change size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right! That get's me every time. It is very common for folks to change size of scrollable children through animations or in response to the scroll position changing.
| // Remove the cached extent if widget's key changed. | ||
| _cachedItemExtents.remove(index); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the whole cache be cleared in this case? Or should some kind of update be triggered?
| index += 1; | ||
| if (!inLayoutRange) { | ||
| if (child == null || indexOf(child!) != index) { | ||
| // We are missing a child. Insert it (and lay it out) if possible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are missing a child as this comment indicates, what should that mean for the cache?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means that the missing child has been laid out before (removed from the tree due to offstage), and using the cached range will save layout costs.
| if (super.paintExtentOf(child!) != _cachedItemExtents[index]!.mainAxisExtent) { | ||
| throw FlutterError( | ||
| 'The item(index: $index) main axis extent has changed after caching, if this is ' | ||
| 'expected, change its Key to update the cached value.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing the key might not be an option in some cases. If the extent is different, can we just update it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing the key might not be an option in some cases.
You are right, we should provide a way to invalidate certain caches.
If the extent is different, can we just update it?
Maybe we should throw here because we've used the wrong cached value to find the target position.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems dangerous if developers are not sure what they are doing. It is also a bit confusing that it would require manual invalidation of caches. I think what would happen is developer will just use the invalidating caches everywhere so that it doesn't crash.
| _childrenWithoutLayout.clear(); | ||
| _childConstraints = constraints.asBoxConstraints(); | ||
| // Clear the cached extents if [crossAxisExtent] or [axis] changed. | ||
| if (constraints.crossAxisExtent != _lastCrossAxisExtent || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
viewportMainAxisExtent would probably clear the cache too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, the layout of items in the main axis direction is not constrained(it can scrollable on the main axis), so the cache is not cleared. At the same time, developers can explicitly clear the cache data through invalidCachedItemExtents if needed.
Maybe both behaviors are okay and I can accept both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, the layout of items in the main axis direction is not constrained
Totally! You are right. :)
Some folks size/position their children based on the viewport dimensions, especially not that we provide that information in the itemExtentBuilder feature you added recently. So it would probably be good to use a change in the viewport dimension as a cache invalidator.
Piinks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've looped in @chunhtai for his thoughts on this. Thanks for your work here!
| _childrenWithoutLayout.clear(); | ||
| _childConstraints = constraints.asBoxConstraints(); | ||
| // Clear the cached extents if [crossAxisExtent] or [axis] changed. | ||
| if (constraints.crossAxisExtent != _lastCrossAxisExtent || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, the layout of items in the main axis direction is not constrained
Totally! You are right. :)
Some folks size/position their children based on the viewport dimensions, especially not that we provide that information in the itemExtentBuilder feature you added recently. So it would probably be good to use a change in the viewport dimension as a cache invalidator.
chunhtai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This api seems to be a bit dangerous. It is hard to predict child size change. I am not sure if this is something we want to pursue. There is also a PreferredSizeWidget, but that will require developer to give the size beforehand.
| /// previously cached data will be cleared. | ||
| /// | ||
| /// The cached data will be used to efficiently determine the target children | ||
| /// within the lazy loading scope, which helps improve layout performance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure how this would work, even if the SliverChildListDelegate doesn't change, the child itself can still change size.
| if (super.paintExtentOf(child!) != _cachedItemExtents[index]!.mainAxisExtent) { | ||
| throw FlutterError( | ||
| 'The item(index: $index) main axis extent has changed after caching, if this is ' | ||
| 'expected, change its Key to update the cached value.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems dangerous if developers are not sure what they are doing. It is also a bit confusing that it would require manual invalidation of caches. I think what would happen is developer will just use the invalidating caches everywhere so that it doesn't crash.
|
This is my thoughts.
This feature is enabled optional by the developer and only applies if the developer is sure that the children's sizes will not changed during the life cycle. I think there are many such scenarios, for example, on the web, once the child renders, the size will not change unless the user refreshes the page. In these scenarios, caching can greatly improve scrolling performance.
|
|
I can see how it can be useful in some circumstances, but I am still not convince we should provide it in the framework as it can be easily misused. Have you considered put this into a third party package instead? |
|
I can see how it can be useful in some circumstances, but I am still not convince we should provide it in the framework as it can be easily misused. Have you considered put this into a third party package instead?
- `3rd party package`
If this has no benefits or brings side effects to the framework, or there are only very rare benefit scenarios, third-party package is an option. Or we can continue to discuss this topic in depth and collect developer voices.
Even as a 3rd package, we also need resolve the misusage before published, right? : )
- `Misusage`
In addition to complete documentation, some necessary checks in debug mode can well catch the misusage cases.
- `manual invalidation of caches`
Currently, I think we should only support a property that invalidate all cached data. When explicit size changes occur (such as reordering, adding or removing a child from the middle), even if the user forgets to manually clear it, the framework will throw an assertion.
|
I propose as 3rd party because it is has limited use cases.
My concern are:
|
|
That makes sense, thanks for your reviews.
|
|
I am going to close this PR. Still thanks for your contribution even though we decide to not move forward. |
|
@xu-baolin, just a heads-up, we do have a package that does similar thing and it performs well. https://superlistapp.github.io/super_sliver_list/ Cache invalidation is generally not a problem, since cached extents are always treated as estimations, every time items in the cache area are laid out cached extents are estimated and scroll offset / extends are corrected to reflect the difference. Just mentioning this to possibly save you some time. |
|
@knopp Hi, does your package solve this issue? #143817 (comment) |
Unlikely. The problem there seems to be that the items are laid out with small extent initially and then expanded as the image loads (asynchronously). |
Yes, it's a very serious problem for my app, the latest version of flutter still hasn't solved it, the related issue is here #99158, if you can solve it, it would be extremely helpful and I'm more than happy to donate! |
You can always fix this by caching the size of images yourself and then use SizedBox with cached size when buildings item while scrolling back. The main issue here is that your initially create all your items with very small extent and then they resize after a while when the image is loaded. No ListView implementation can fix this (in theory resizing items in cache area can correct the scroll offset transparently but this will fall apart when you scroll too quickly so the best way is to always create the widget with proper size once you know it). |
|
@knopp Very helpful, thank you |
Document description:
Introduced new property
enableItemExtentsCachingforListView, the documentation here describes how it works.Performance:(Window PC, Prefile mode, Scrolling from Top to Bottom with linear curves in one second)
Without caching (max 13.4ms/frame, avg 10.1ms/frame)
Enable caching (max 6.5ms/frame, avg 4.9ms/frame)
TODO:
1, Should we provide a method for users to proactively clear all the cached data(caching enabled)? I'm not so sure.
2, Add tests for this new feature.
Test Demo Code