Skip to content

Conversation

@xu-baolin
Copy link
Member

@xu-baolin xu-baolin commented Sep 16, 2023

Document description:

Introduced new property enableItemExtentsCaching for ListView, the documentation here describes how it works.

  /// {@template flutter.widgets.list_view.enableItemExtentsCaching}
  /// Whether to cache the main axis extents of all laid out children. The
  /// cached data will be used to calculate which children will be loaded during
  /// the 'Lazy Loading' phase.
  ///
  /// If true, the main axis extent of child will be cached as
  /// index/(key, extent) pairs when the child first laid out. If the [Key] of
  /// the item widget changes after caching, the corresponding cached extent
  /// will be removed and the new extent will be cached after layout.
  ///
  /// If false, the main axis extents of children will not be cached and all the
  /// previously cached data will be cleared.
  ///
  /// The default is false.
  ///
  /// This caching mechanism is suitable for scenarios where the main axis extents
  /// of children do not change or change infrequently during its life cycle.
  /// Using cached extent dimensions will improve scrolling performance,
  /// especially in scenarios that the scroll position changes drastically,
  /// as it will significantly reduce the layout times if the children
  /// have been laid out before.
  ///
  /// If the main axis extent of child at a certain index changes, the [Key] of
  /// the item widget needs to change to drive the framework to invalidate
  /// this child's cached value.
  ///
  /// All cached data will be cleared automatically if [scrollDirection] or
  /// [SliverConstraints.crossAxisExtent] changed.
  ///
  /// This has no effect if the extent of the children is predetermined,
  /// for example, the [itemExtent], [itemExtentBuilder] or [prototypeItem] have
  /// a non-null value, as they are already efficient.
  /// {@endtemplate}
  final bool enableItemExtentsCaching;

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)

Image_20230918093837

Enable caching (max 6.5ms/frame, avg 4.9ms/frame)

Image_20230918094106

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

import 'dart:ui';

import 'package:flutter/material.dart';
import 'package:flutter/rendering.dart';
final scrollController = ScrollController(initialScrollOffset: childCount * 30);
final childCount = 10000;
void main() {
  runApp(
    MaterialApp(
      showPerformanceOverlay: true,
      scrollBehavior: MyScrollBehavior(),
      home: Scaffold(
        body: const ExampleApp(),
        floatingActionButton: FloatingActionButton(onPressed: () {
          if (scrollController.offset < 100) {
            scrollController.animateTo(
              childCount * 20,
                duration: const Duration(seconds: 1),
                curve: Curves.linear,
            );
          } else {
            scrollController.animateTo(
              0,
              duration: const Duration(seconds: 1),
              curve: Curves.linear,
            );
          }
        },),
      ),
    ),
  );
}

class ExampleApp extends StatefulWidget {
  const ExampleApp({Key? key}) : super(key: key);

  @override
  State<StatefulWidget> createState() {
    return ExampleAppState();
  }
}

class ExampleAppState extends State<ExampleApp> {

  @override
  Widget build(BuildContext context) {
    return Scrollbar(
      thumbVisibility: true,
      controller: scrollController,
      child: ListView.builder(
        cacheExtent: 250,
        // scrollDirection: Axis.horizontal,
        controller: scrollController,
        itemCount: childCount,
        enableItemExtentsCaching: true,
        // itemExtent: 25.0,
        // itemExtentBuilder: (int index, SliverLayoutDimensions dimensions) {
        //   return 10;
        // },
        // itemExtentBuilder: (int index, SliverLayoutDimensions dimensions) {
        //   if (index % 2 == 0) {
        //     return 50;
        //   } else {
        //     return 100;
        //   }
        // },
        itemBuilder: (BuildContext context, int index) {
          var color = Colors.yellow;
          if (index % 2 == 0) {
            color = Colors.red;
          }

          return ColoredBox(
            color: color,
            child: Center(
              child: Text('Item $index'),
            ),
          );
        },
      ),
    );
  }
}

class MyScrollBehavior extends MaterialScrollBehavior {
  @override
  Widget buildScrollbar(
      BuildContext context, Widget child, ScrollableDetails details) {
    return child;
  }

  @override
  Set<PointerDeviceKind> get dragDevices => <PointerDeviceKind>{
    PointerDeviceKind.touch,
    PointerDeviceKind.mouse,
  };
}

@flutter-dashboard
Copy link

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.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: scrolling Viewports, list views, slivers, etc. labels Sep 16, 2023
@xu-baolin xu-baolin force-pushed the xbl230917 branch 2 times, most recently from be9a4f7 to 91aaa78 Compare September 18, 2023 09:32
@xu-baolin xu-baolin changed the title [WIP][Performance] Introduce a caching mechanism to ListView to reduce unnecessary layout during scrolling [New feature][Performance] Introduce a caching mechanism to ListView to reduce unnecessary layout during scrolling Sep 18, 2023
@xu-baolin xu-baolin added c: new feature Nothing broken; request for a new capability c: performance Relates to speed or footprint issues (see "perf:" labels) labels Sep 18, 2023
@goderbauer goderbauer requested a review from Piinks September 19, 2023 22:21
Copy link
Contributor

@Piinks Piinks left a 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! :)

Comment on lines +55 to +56
/// If true, the main axis extent of child will be cached as
/// index/extent pairs when the child first laid out.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// 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.
Copy link
Contributor

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?

Copy link
Member Author

@xu-baolin xu-baolin Sep 28, 2023

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?

Copy link
Contributor

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. :)

Copy link
Contributor

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.

Copy link
Contributor

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.

Comment on lines +97 to +98
// Remove the cached extent if widget's key changed.
_cachedItemExtents.remove(index);
Copy link
Contributor

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.
Copy link
Contributor

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?

Copy link
Member Author

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.',
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

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 ||
Copy link
Contributor

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.

Copy link
Member Author

@xu-baolin xu-baolin Sep 28, 2023

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.

Copy link
Contributor

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.

Copy link
Contributor

@Piinks Piinks left a 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 ||
Copy link
Contributor

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.

Copy link
Contributor

@chunhtai chunhtai left a 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.
Copy link
Contributor

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.',
Copy link
Contributor

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.

@xu-baolin
Copy link
Member Author

xu-baolin commented Oct 12, 2023 via email

@chunhtai
Copy link
Contributor

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?

@xu-baolin
Copy link
Member Author

xu-baolin commented Oct 13, 2023 via email

@chunhtai
Copy link
Contributor

chunhtai commented Oct 13, 2023

Even as a 3rd package, we also need resolve the misusage before published, right? : )

I propose as 3rd party because it is has limited use cases.

In addition to complete documentation, some necessary checks in debug mode can well catch the misusage cases.

My concern are:

  1. It is hard to predict child would change size or not. Developer may think their widget has fixed size while it may not, and they use this feature without knowing it
  2. We only know things are wrong (thus throw or assert) when the size did change. That means if a widget only change size in a corner case, it will be hard to catch this kind of bug during developerment.

@xu-baolin
Copy link
Member Author

xu-baolin commented Oct 16, 2023 via email

@chunhtai
Copy link
Contributor

I am going to close this PR. Still thanks for your contribution even though we decide to not move forward.

@chunhtai chunhtai closed this Oct 16, 2023
@knopp
Copy link
Member

knopp commented Feb 20, 2024

@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.

@fyxtc
Copy link

fyxtc commented Mar 3, 2024

@knopp Hi, does your package solve this issue? #143817 (comment)

@knopp
Copy link
Member

knopp commented Mar 3, 2024

@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).

@fyxtc
Copy link

fyxtc commented Mar 4, 2024

@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!

@knopp
Copy link
Member

knopp commented Mar 4, 2024

@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).

@fyxtc
Copy link

fyxtc commented Mar 5, 2024

@knopp Very helpful, thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c: new feature Nothing broken; request for a new capability c: performance Relates to speed or footprint issues (see "perf:" labels) f: scrolling Viewports, list views, slivers, etc. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants