Fix keepalive for large jumps on tabs and lists. by dnfield · Pull Request #21350 · flutter/flutter

dnfield · 2018-09-03T08:04:05Z

Successor to #21207

The SliverMultiBoxAdaptorElement tries to keep track of the child it should be inserting children after when it needs to do an insertion. This breaks if the child it thinks it wants doesn't know what siblings it should have because it got thrown into the keepAliveBucket at some point - the insertion logic expects children to be connected and in the proper indexing order.

This pull makes sure that if the child you want to insert after is in the keep alive bucket, it's removed and reinserted to properly get connected to it's siblings. This case happens in, for example, a multi tab view where the first tab is kept alive and you immediately go to tab 4 (but not if you go from 1 to 2). It also manifests in a sliver list where you fling and then rebuild the list.

marekchen · 2018-09-03T10:06:56Z

@dnfield Does your pr solve this issue #18832

dnfield · 2018-09-03T15:12:46Z

@marekchen - no. The bug you're reporting there is not related to this issue.

googlebot · 2018-09-04T16:57:41Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

… alive

googlebot · 2018-09-04T17:00:15Z

CLAs look good, thanks!

dnfield · 2018-09-04T17:29:47Z

/cc @goderbauer - I'm seeing behavior occasionally where code added in #16445 is getting called after context is null (presumably because by the time the frame has been updated the widget is no longer in the tree). Added some code here to check for nulls in these cases.

This should still be ok because of the code I introduced in sliver.dart, which should correct the bad parent data a bit later in the whole process if necessary.

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

goderbauer · 2018-09-04T19:43:21Z

packages/flutter/test/material/tabs_test.dart

+    await tester.pumpAndSettle();
+    await tester.pump();
+    expect(controller.index, 3);
+    expect(find.text('4'), findsOneWidget);


maybe assert at the beginning that text('4') didn't find anything to ensure that the setup is and stays correct? And assert at the end that AlwaysKeepAlive still finds something?

goderbauer · 2018-09-04T19:45:00Z

packages/flutter/test/widgets/list_view_test.dart

+                  return new Container(
+                    child: new Text('$i'),
+                  );
+                }


Revert the whitespace-only change? Adding a comma after } would be the correct Flutter-style.

goderbauer · 2018-09-04T19:45:22Z

packages/flutter/test/widgets/list_view_test.dart

    log.clear();
  });

+  testWidgets('ListView large scroll jump and keepAlive (first child not keepAlive', (WidgetTester tester) async {


nit: missing closing )

goderbauer · 2018-09-04T19:47:19Z

packages/flutter/test/widgets/list_view_test.dart

+      child: new Directionality(
+        textDirection: TextDirection.ltr,
+        child: new ListView(
+          itemExtent: null,


nit: remove itemExtent as null is the default?

goderbauer · 2018-09-04T19:49:28Z

packages/flutter/test/widgets/list_view_test.dart

+  @override
+  Widget build(BuildContext context) {
+    return new GestureDetector(
+      onTap: () => setState,


Why this? No state seems to be changing?

Forces a rebuild.

The issue is that on rebuild, an attempt at fixing this was dropping leading widgets with a keepAlive. Rebuild here checks to make sure we're retaining those widgets.

Makes sense. Probably worth adding a comment here explaining the strange usage of setstate.

goderbauer

LGTM

Hixie · 2018-09-07T06:40:37Z

packages/flutter/lib/src/rendering/sliver_multi_box_adaptor.dart

+    if (after != null) {
+      final SliverMultiBoxAdaptorParentData afterParentData = after.parentData;
+      final int index = afterParentData.index;
+      if (_keepAliveBucket.containsKey(index)) {


rather than looking up the index in the map, i think you can just check _keptAlive on the child's parentData (then assert that it's in the map)

(while you're at it, we should probably change _createOrObtainChild to do that too)

_createOrObtainChild gets an index rather than a child. Not sure that one makes sense to change to me. But this one does make sense.

Hixie · 2018-09-07T06:48:08Z

This change seems wrong to me. Suppose you keep alive almost every child. When you try to insert a child that used to be after another child that is now itself kept alive, this code is going to insert every child back into the tree in an O(N^2) loop (each insert calls insert for every previous child, and does a loop over the children to find where to insert it).

Why are we trying to insert something after a child that is being kept alive? That seems like the real problem here. We're not supposed to ever be passing in an after to insertAndLayoutChild that isn't itself already laid out and ready.

dnfield · 2018-09-07T16:52:54Z

Suppose you keep alive almost every child.

That's not a good idea for a sliverlist anyway - the whole point there is to be efficient with memory usage. Keeping almost every child alive won't offer you that benefit.

When you try to insert a child that used to be after another child that is now itself kept alive, this code is going to insert every child back into the tree in an O(N^2) loop (each insert calls insert for every previous child, and does a loop over the children to find where to insert it).

That is a worst case scenario, yes.

Why are we trying to insert something after a child that is being kept alive? That seems like the real problem here. We're not supposed to ever be passing in an after to insertAndLayoutChild that isn't itself already laid out and ready.

The basic problem is that the Sliver widget tries to keep track of an anchor point (_currentBeforeChild). It then tries to use that anchor point whenever someone wants to insert, so it doesn't have to go through the O(N^2) search. That all works fine until that _currentBeforeChild got moved to the KeepAlive bucket by SliverMultiBoxAdaptor (and this happens for sure when there's a flick scroll or multi-tab jump). SliverMultiBoxAdaptor has no way to tell Sliver that it messed up its cached child, and Sliver has no way to tell SliverMultiBoxAdaptor that it needs to find the siblings of its cached child.

The only way I can think of, short of the potentially costly search, is to have SliverMultiBoxAdaptor keep a Map of its RenderBoxes, which would ruin the memory savings.

The bottom line seems to be that KeepAlive is costly in Slivers, and if you use it too much you're going to incur a lot of memory or processing overhead.

dnfield · 2018-09-07T17:04:03Z

One small enhancement might be to check if the index on the parent data is closer to _firstChild.parentData.index or _lastChild.parentData.index and potentially search backwards from the _lastChild - but it's still an expensive algorithm in the pathological case.

dnfield · 2018-09-07T17:51:39Z

macOS failure is a Dart VM garbage collection bug - dart-lang/sdk#33111

…ling

dnfield · 2018-09-07T22:35:03Z

After offline conversation, this is the better fix @Hixie was looking for.

Ensure that the _childElements map is properly traversed as a sparse list and not inflated with garbage collected children
Fixed test to actually ensure rebuild was actually happening with the tap. Test now fails with incorrect fix (where it was passing before).
@Hixie - a couple additions beyond what we discussed to correctly handle underflow and make sure we don't end up trying to concurrently iterate and modify _childElements.

Steelsmasher · 2018-09-08T15:15:43Z

Has this fix been implemented? Which version of flutter will I need?

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

dnfield · 2018-09-10T07:35:54Z

@Steelsmasher it should be in master before much longer, and dev a little while after that.

Hixie · 2018-09-11T00:32:29Z

packages/flutter/lib/src/rendering/sliver_multi_box_adaptor.dart


+  /// Whether the widget is currently being kept alive, i.e. has [keepAlive] set
+  /// to true and is offscreen.
+  bool get keptAlive => _keptAlive;


normally we put the public getter above the private field and only have docs for the public getter. your call as to whether this case warrants an exception.

Hixie · 2018-09-11T00:33:16Z

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

        // build of this subtree. Wait until the end of the frame to update
        // the child when the child is guaranteed to be present.
        SchedulerBinding.instance.addPostFrameCallback((Duration timeStamp) {
+          // The element is no longer alive


End comments with a period. Also since the comment is outside the if it should be a question ("Is the element still alive?"), or you can put it inside the block, or you can just remove it, the code speaks for itself here and the comment isn't really adding anything.

Hixie · 2018-09-11T00:34:44Z

dnfield added 7 commits August 29, 2018 22:47

Only set _currentBeforeChild if newChild is connected to siblings

440c37e

test

8f72985

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

eceb4fe

restrict change to keepAlive

2c264fd

Fix for non-fixed slivers

e4ef253

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

35460fe

Add test for non-fixed sliver list

20c2d07

googlebot added the cla: yes label Sep 3, 2018

dnfield requested a review from Hixie September 3, 2018 08:07

googlebot added cla: no and removed cla: yes labels Sep 4, 2018

Handle another case where childElement can be null for automatic keep…

227e45d

… alive

dnfield force-pushed the i11895 branch from 1c064d6 to 227e45d Compare September 4, 2018 17:00

googlebot added cla: yes and removed cla: no labels Sep 4, 2018

better check

7ea950a

dnfield requested a review from goderbauer September 4, 2018 17:30

dnfield added 2 commits September 4, 2018 10:40

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

cf55262

add back unintentionally removed assert

d390d39

goderbauer reviewed Sep 4, 2018

View reviewed changes

packages/flutter/lib/src/widgets/automatic_keep_alive.dart Outdated Show resolved Hide resolved

goderbauer reviewed Sep 4, 2018

View reviewed changes

goderbauer approved these changes Sep 7, 2018

View reviewed changes

Hixie reviewed Sep 7, 2018

View reviewed changes

use _keptAlive

479f559

dnfield added 6 commits September 7, 2018 14:30

Fixes

c39ab91

Fix for underflow

b5c68fe

More correctly handle _childElement list to resolve problem

88cd85f

remove extraneous print

62b8762

Fix test to not tap offscreen widget

2d0c592

Prevent concurrent iteration and modification; correct underflow hand…

d470fdb

…ling

dnfield added 2 commits September 7, 2018 15:42

add comment

f05f2da

make the analyzer happy

e44e488

SteveAlexander reviewed Sep 10, 2018

View reviewed changes

packages/flutter/lib/src/widgets/sliver.dart Outdated Show resolved Hide resolved

add missing word in comment

39560d3

SteveAlexander mentioned this pull request Sep 10, 2018

Three things to make TabView work without crashing in my app #21630

Closed

Hixie reviewed Sep 11, 2018

View reviewed changes

comment fixup

630a4fb

dnfield merged commit 98ad574 into flutter:master Sep 11, 2018

dnfield mentioned this pull request Sep 17, 2018

Scrolling through SliverGridList Throws an AssertionFailure #16596

Closed

dnfield deleted the i11895 branch September 17, 2018 20:44

SteveAlexander mentioned this pull request Nov 28, 2018

fix for issue 24660 #24821

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 9, 2021

Conversation

dnfield commented Sep 3, 2018

Uh oh!

marekchen commented Sep 3, 2018

Uh oh!

dnfield commented Sep 3, 2018

Uh oh!

googlebot commented Sep 4, 2018

Uh oh!

googlebot commented Sep 4, 2018

Uh oh!

dnfield commented Sep 4, 2018

Uh oh!

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!

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!

Hixie commented Sep 7, 2018

Uh oh!

dnfield commented Sep 7, 2018

Uh oh!

dnfield commented Sep 7, 2018

Uh oh!

dnfield commented Sep 7, 2018

Uh oh!

dnfield commented Sep 7, 2018

Uh oh!

Steelsmasher commented Sep 8, 2018

Uh oh!

Uh oh!

dnfield commented Sep 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hixie commented Sep 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants