Fix excessive rebuilds of DSS #69724

dnfield · 2020-11-03T23:48:44Z

Description

DraggableScrollableSheet internally ends up calling the builder function every time the sheet scrolls - this is really unfortunate when, for example, it contains a sizable ListView that gets rebuilt every time the sheet slides up or down one pixel.

To resolve this, move the call of the builder function to didChangeDependencies so it's only called when our position in the tree changes, and reuse the cached child in build.

Related Issues

Fixes #67219

Tests

Assert that the builder is only called once when dependencies do not change and the sheet is dragged.

This is not a breaking change.

dnfield · 2020-11-03T23:51:23Z

packages/flutter/test/widgets/draggable_scrollable_sheet_test.dart

+    ));
+    expect(buildCount, 1);
+    await tester.flingFrom(const Offset(0, 325), const Offset(0, -325), 200);
+    expect(buildCount, 1);


Before this change, the buildCount was coming out to 48.

goderbauer · 2020-11-04T19:36:43Z

packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart

      }
      _extent._currentExtent.value = _extent.initialExtent;
    }
+    _child = widget.builder(context, _scrollController);


What if the DraggableScrollableSheet is given a new builder during its lifetime?

Unrelated: Looks like changes to minChildSize and maxChildSize are also not respected, which seems like another bug?

I think I can address this with didUpdateWidget - will push something momentarily.

I seem to recall that a builder is best called on every build (that's what the canonical Builder widget does at least). I don't remember why, though. /cc @Hixie

In this case, that's too much - so I'm trying to cache the output of that builder to avoid rebuilding it whenever the sheet is dragged up or down.

This is simlar to how we don't rebuild every child sliver when a listview scrolls.

Sounds good.

goderbauer · 2020-11-04T19:55:41Z

Conceptually, could you have the DraggableScrollableSheet build an intermediate widget that takes the prebuild child widget. The intermediate widget then contains the layoutbuilder and the logic to frequently rebuild, but since it is configured with a prebuild child, the child wouldn't build again?

Something like:

class DraggableScrollableSheet extends StatelessWidget {
  DraggableScrollableSheet(
  // ...
  this.builder,
  );

  // ..
  Widget build(BuildCOntext context) {
    return _Intermediate(
      //..
      child: builder(context),
    );
  }

_Intermediate is basically what DraggableScrollableSheet is today with a child instead of a builder param.

Only difficulty: We'd have to move ownership of the scroll controller out of the _Intermediate widget to this new DraggableScrollableSheet since the builder needs it as argument. That doesn't seem impossible, though?

dnfield · 2020-11-05T16:21:36Z

Can't I just update the child in didUpdateWidget and it shoul dbe fine?

dnfield · 2020-11-05T16:37:33Z

@goderbauer added some tests that failed based on your suggestion, fixed them with didUpdateWidget - nice catch on the min/max/initial extent too. PTAL.

dnfield · 2020-11-05T16:47:09Z

packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart

+      _child = widget.builder(context, _scrollController);
+    }
+
+    _extent = _DraggableSheetExtent(


This is cheap to create, so it seems silly to check the min/max/initial sizes for changes.

Is this still true even with us now creating a new _scrollController as well, which will cause other widgets to move their listeners over to the new one?

I think it's probably still fine

goderbauer · 2020-11-05T17:42:38Z

packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart

+      _child = widget.builder(context, _scrollController);
+    }
+
+    _extent = _DraggableSheetExtent(


How is the new extend wired up to the _DraggableScrollableSheetScrollController?

goderbauer · 2020-11-05T17:50:20Z

packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart

      }
      _extent._currentExtent.value = _extent.initialExtent;
    }
+    _child = widget.builder(context, _scrollController);


I seem to recall that a builder is best called on every build (that's what the canonical Builder widget does at least). I don't remember why, though. /cc @Hixie

goderbauer · 2020-11-05T17:53:29Z

packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart

+  @override
+  void didUpdateWidget(DraggableScrollableSheet oldWidget) {
+    super.didUpdateWidget(oldWidget);
+    if (oldWidget.builder != widget.builder) {


If anything, I think you need to call the builder even if the closure has the same identity. For example, if somebody passes in a tear-off function of a State object, you always get the same builder object, but values closured into that builder may have changed.

goderbauer · 2020-11-06T18:26:37Z

packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart

 class _DraggableScrollableSheetState extends State<DraggableScrollableSheet> {
  late _DraggableScrollableSheetScrollController _scrollController;
  late _DraggableSheetExtent _extent;
+  // The child only gets rebuilt when dependencies change. Otherwise, excessive


nit: comment is out of date, also rebuilds on didUpdateWidget.

goderbauer · 2020-11-06T18:27:16Z

packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart

+  @override
+  void didUpdateWidget(DraggableScrollableSheet oldWidget) {
+    super.didUpdateWidget(oldWidget);
+    _child = widget.builder(context, _scrollController);


Maybe add a quick comment here reminding us in the future why we have to call this unconditionally (and not just when builder changes)?

goderbauer · 2020-11-06T18:28:41Z

packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart

+  void didUpdateWidget(DraggableScrollableSheet oldWidget) {
+    super.didUpdateWidget(oldWidget);
+    _child = widget.builder(context, _scrollController);
+    _updateExtent();


I think _updateExtent has to run before we execute the builder since it instantiates a new scrollController that's used by the builder?

goderbauer · 2020-11-06T18:29:53Z

packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart

+      _child = widget.builder(context, _scrollController);
+    }
+
+    _extent = _DraggableSheetExtent(


Is this still true even with us now creating a new _scrollController as well, which will cause other widgets to move their listeners over to the new one?

goderbauer · 2020-11-06T18:31:56Z

packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart

      }
      _extent._currentExtent.value = _extent.initialExtent;
    }
+    _child = widget.builder(context, _scrollController);


Sounds good.

goderbauer

LGTM

SiyrisSoul · 2020-11-13T22:01:02Z

@dnfield With this change I'm getting inconsistent behavior between builds of my app. It works perfectly when first implementing but after a hot restart or full rebuild it doesn't work and only seems to register the scroll controller for the content inside the sheet. If I change the widget tree by for example adding a container around the DSS then it will work again just once, and again stop being draggable after any restarts or refreshes.

dnfield · 2020-11-13T22:56:13Z

Hmm. We probably need something for reassemble.

dnfield · 2020-11-13T23:02:24Z

@TutOsirisOm - if you add something like this does it work:

diff --git a/packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart b/packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart
index 9303c98c78..68fa9608c7 100644
--- a/packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart
+++ b/packages/flutter/lib/src/widgets/draggable_scrollable_sheet.dart
@@ -330,6 +330,16 @@ class _DraggableScrollableSheetState extends State<DraggableScrollableSheet> {
     _child = widget.builder(context, _scrollController);
   }
 
+  @override
+  void reassemble() {
+    super.reassemble();
+    _updateExtent();
+    // Call this unconditionally - the closure may not change even though it
+    // refers to things outside of its identity, e.g. a tearoff from state that
+    // has an `if (stateVariable)`.
+    _child = widget.builder(context, _scrollController);
+  }
+
   void _updateExtent() {
     _extent = _DraggableSheetExtent(
       minExtent: widget.minChildSize,

Hixie · 2020-11-13T23:12:26Z

Calling it from reassemble itself is probably sketchy (reassemble is called before the build phase). Better to do something like set _child to null and have the build method call the builder if it's null (??=), that way you know the timing is going to be appropriate.

dnfield · 2020-11-13T23:19:34Z

There's something strange going on here still.

SiyrisSoul · 2020-11-15T19:15:48Z

I'm going to mess around with this today. Any progress been made in the meantime?

dnfield · 2020-11-15T19:19:35Z

I started looking Friday afternoon but didn't make a lot of progress. I can look more tomorrow if you dont get to it first

SiyrisSoul · 2020-11-15T21:17:48Z

@dnfield Can we move _updateExtent() from didUpdateWidget()? I'm honestly not entirely sure what calling it there is supposed to do. At least in my use case removing it made everything seemingly work as intended. I also tried moving it into didChangeDependencies() above setting the _child with no issues, but again not sure what case exactly I should be testing for.

SiyrisSoul · 2020-11-19T19:07:38Z

Any progress on this? I haven't yet noticed anything not working after the changes I mentioned above.

dnfield · 2020-11-19T19:09:51Z

I haven't had a chance to get back to this yet. Sorry. I may be able to today, but I have a couple other higher priority items to make progress on first.

dnfield · 2020-11-19T22:30:40Z

It is unlikely that I will be able to get to this. If we need to revert this change until we can fix this I think that's fine.

dnfield · 2020-11-19T22:31:03Z

Unlikely to get to it in the next two weeks I mean :)

SiyrisSoul · 2020-11-19T22:50:01Z

That's fine, in the meantime I'll look into it some more but as is the changes I made earlier are working as intended for me. :)

This reverts commit 16dce76.

gabrieldelorean · 2021-01-21T22:27:46Z

Any progress on this?

dnfield · 2021-01-21T23:15:01Z

I have not had time to look at this again and am not sure when I'll get back to it.

I still suspect something like this fix can work, but it needs to address hot restart issues as well as something wrong with how the scroll controller was or wasn't getting recreated.

agiletechie · 2022-10-01T18:09:37Z

is this being worked on?

dnfield requested review from HansMuller, Hixie and goderbauer November 3, 2020 23:48

google-cla bot added the cla: yes label Nov 3, 2020

dnfield mentioned this pull request Nov 3, 2020

DraggableScrollableSheet is rebuilding the screen while dragging #67219

Closed

Fix excessive rebuilds of DSS

3567e83

dnfield force-pushed the drag_build branch from 8e243d5 to 3567e83 Compare November 3, 2020 23:50

dnfield commented Nov 3, 2020

View reviewed changes

goderbauer reviewed Nov 4, 2020

View reviewed changes

Respect widget config changes

5984733

dnfield commented Nov 5, 2020

View reviewed changes

goderbauer reviewed Nov 5, 2020

View reviewed changes

review

0cee9f4

goderbauer added the framework flutter/packages/flutter repository. See also f: labels. label Nov 6, 2020

goderbauer reviewed Nov 6, 2020

View reviewed changes

dnfield added 2 commits November 6, 2020 10:59

Merge remote-tracking branch 'upstream/master' into drag_build

6bbe84b

review

b1ba129

goderbauer approved these changes Nov 6, 2020

View reviewed changes

dnfield added the waiting for tree to go green label Nov 6, 2020

fluttergithubbot merged commit 16dce76 into flutter:master Nov 6, 2020

dnfield added a commit that referenced this pull request Dec 1, 2020

Revert "Fix excessive rebuilds of DSS (#69724)"

3100ec0

This reverts commit 16dce76.

dnfield mentioned this pull request Dec 1, 2020

Revert "Fix excessive rebuilds of DSS" #71459

Merged

dnfield added a commit that referenced this pull request Dec 2, 2020

Revert "Fix excessive rebuilds of DSS (#69724)" (#71459)

a2aa2ed

This reverts commit 16dce76.

caseycrogers mentioned this pull request Sep 19, 2021

Make DraggableScrollableSheet Reflect Parameter Updates #90354

Merged

8 tasks

Fix excessive rebuilds of DSS #69724

Fix excessive rebuilds of DSS #69724

Uh oh!

Conversation

dnfield commented Nov 3, 2020

Description

Related Issues

Tests

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer commented Nov 4, 2020

Uh oh!

dnfield commented Nov 5, 2020

Uh oh!

dnfield commented Nov 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

SiyrisSoul commented Nov 13, 2020

Uh oh!

dnfield commented Nov 13, 2020

Uh oh!

dnfield commented Nov 13, 2020

Uh oh!

Hixie commented Nov 13, 2020

Uh oh!

dnfield commented Nov 13, 2020

Uh oh!

SiyrisSoul commented Nov 15, 2020

Uh oh!

dnfield commented Nov 15, 2020

Uh oh!

SiyrisSoul commented Nov 15, 2020

Uh oh!