Do not rebuild Routes when a new opaque Route is pushed on top #48900

goderbauer · 2020-01-15T18:12:34Z

Also known as: Don't rebuild OverlayEntries when one changes opaqueness.

Description

Prior to this change we were re-arranging the tree when an OverlayEntry went from onstage to offstage (due to opaqueness changes) and vice versa:

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

Lines 461 to 467 in 4f9b6cf

    
           return _Theatre( 
        
             onstage: Stack( 
        
               fit: StackFit.expand, 
        
               children: onstageChildren.reversed.toList(growable: false), 
        
             ), 
        
             offstage: offstageChildren, 
        
           );

Since the OverlayEntries have a global key their corresponding element was deactivated at the old location inside the Stack and re-activated at the new location outside the Stack. This caused all StatefulWidgets within that subtree to rebuild because their ancestor had changed.

This change avoids the rebuild by not re-arranging the tree. Instead, both onstage and offstage OverlayEntries are children of the _Theatre, so they don't have to be moved around in the tree. The RenderObject of the _Theatre implements a special version of a Stack that just ignores the offstage children during rendering and layout. (Since the offstage children are always all at the beginning of the child list, this is implemented by providing an integer skipCount indicating the number of children to skip.)

The unnecessary rebuilds were most commonly felt when pushing new Routes because Navigator uses an Overlay internally.

Related Issues

Fixes #45797.

Tests

I added the following tests:

OverlayEntries do not rebuild when opaque entry is added
OverlayEntries do not rebuild when opaqueness changes
entries below opaque entries are ignored for hit testing
Semantics of entries below opaque entries are ignored
Can used Positioned within OverlayEntry
Pushing opaque Route does not rebuild routes below

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: Migration Guide for Overlay Rebuild Optimization website#3581

goderbauer · 2020-01-15T18:30:49Z

@Hixie Even though I had to update framework tests for this change I would like to handle it as a non-breaking change. Can you grant an exception?

The changes to the tests were to update error messages and error-message-like output. In one instance I had to change a type due to the changed internals of _Theatre.

dnfield

FWIW, I agree changing the tests shouldn't be considered breaking here.

I may be missing something, but the skipCount seems sketchy to me. I think we shoudl at least be asserting that the children we think are offstage are in fact offstage, and vice-versa.

Is there ever a case where an opaque route should still layout other routes? Could we have a route that doesn't cover the whole view?

dnfield · 2020-01-15T19:49:09Z

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

-        children: onstageChildren.reversed.toList(growable: false),
-      ),
-      offstage: offstageChildren,
+      skipCount: children.length - onstageCount,


This seems like it would fail if the logic above added children in an order like [offstage, onstage, offstage, offstage] for some reason.

If we think that's invalid, we should probably assert it. If it's actually valid we need different logic here.

Doing something like [offstage, onstage, offstage, offstage] is not supported by the overlay. The top x entries are always onstage, everything else is either completely ignored or - if maintainState is true - rendered offstage.

Not sure what we should assert here. The assert would basically duplicate the algorithm above.

dnfield · 2020-01-15T19:50:26Z

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

+  @override
+  void debugVisitOnstageChildren(ElementVisitor visitor) {
+    assert(children.length >= widget.skipCount);
+    children.skip(widget.skipCount).forEach(visitor);


Similar concern - I think we at least want an assert here that all the widgets we're skipping are in fact offstage.

It's the other way around, though: The fact that we are skipping them makes them offstage. There's no other marker that marks them as offstage that we could assert on.

dnfield · 2020-01-15T19:51:11Z

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

+  int get skipCount => _skipCount;
+  int _skipCount;
+  set skipCount(int value) {
+    assert(value != null);


assert that the children we're skipping are offstage?

Same answer as above: The fact that we are skipping them makes them offstage. Not the other way around.

dnfield · 2020-01-15T19:52:13Z

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

+      );
+      if (isHit)
+        return true;
+      child = childParentData.previousSibling;


assert the previous sibling is onstage?

goderbauer · 2020-01-15T21:42:42Z

I may be missing something, but the skipCount seems sketchy to me. I think we shoudl at least be asserting that the children we think are offstage are in fact offstage, and vice-versa.

The definition of the (private) _Theatre widget is: "give me a list of children and a number n. I'll not layout/render the first n children (effectively putting them offstage)."

The number n defines what is going to be offstage. There's no other mechanism to tell if a child should be offstage or not...

dnfield

LGTM

Hixie · 2020-01-21T00:14:57Z

I haven't reviewed the test of the patch (I'm on my phone) but the test changes call be considered exempt from the breaking change policy as they are only testing internals.

goderbauer · 2020-01-22T17:31:19Z

This change positively affected some of our Gallery transition benchmarks:

dropped from 63 frames to 50 (minus 21%)

dropped from 50.45ms to 36.64ms (minus 27%)

dropped from 7.38ms to 6.13ms (minus 17%)

dnfield · 2020-01-22T17:47:46Z

Wow I'm so used to seeing posts like that linked to a today bug due to a regression. Nice!

#48900)" This reverts commit 8eecdbe.

#48900)" (#49366) This reverts commit 8eecdbe.

dkwingsmt · 2020-01-23T21:34:07Z

If we were to reland this PR, make sure to address the issue mentioned in #49318, where the order between multiple dependencies are not consistent. They might change due to an irrelevant change, or be different between platforms.

…ed on top (#48900)" (#49366)" This reverts commit 52c665d.

shamilovtim · 2020-05-11T17:27:16Z

Hello, is this released?

goderbauer added 5 commits January 14, 2020 13:41

RenderTheatre implements Stack

21ad850

more tests

f977bf1

self review

8b80081

more tests

8050823

another test

bc5dc19

fluttergithubbot added f: cupertino f: material design framework labels Jan 15, 2020

googlebot added the cla: yes label Jan 15, 2020

goderbauer removed f: cupertino f: material design labels Jan 15, 2020

goderbauer mentioned this pull request Jan 15, 2020

If a label has been manually removed, fluttergithubbot should not re-add it #48901

Closed

dnfield reviewed Jan 15, 2020

View reviewed changes

dnfield approved these changes Jan 15, 2020

View reviewed changes

goderbauer mentioned this pull request Jan 17, 2020

Optimize unneeded rebuild when pushing overlay/navigator opaque content #45797

Closed

goderbauer added the waiting for tree to go green label Jan 21, 2020

goderbauer merged commit 8eecdbe into flutter:master Jan 22, 2020

goderbauer deleted the theatre2 branch January 22, 2020 00:24

goderbauer mentioned this pull request Jan 22, 2020

Regression on flutter_gallery_ios32__transition_perf average_frame_build_time_millis #48247

Closed

bparrishMines added a commit that referenced this pull request Jan 23, 2020

Revert "Do not rebuild Routes when a new opaque Route is pushed on top (

b3e4477

#48900)" This reverts commit 8eecdbe.

bparrishMines mentioned this pull request Jan 23, 2020

Revert "Do not rebuild Routes when a new opaque Route is pushed on top" #49366

Merged

goderbauer added a commit that referenced this pull request Jan 23, 2020

Revert "Revert "Do not rebuild Routes when a new opaque Route is push…

190eb87

…ed on top (#48900)" (#49366)" This reverts commit 52c665d.

goderbauer mentioned this pull request Jan 23, 2020

Reland "Do not rebuild Routes when a new opaque Route is pushed on top" #49376

Merged

fluttergithubbot added f: cupertino f: material design labels Jan 23, 2020

goderbauer added a commit that referenced this pull request Jan 23, 2020

Revert "Revert "Do not rebuild Routes when a new opaque Route is push…

e39f87a

…ed on top (#48900)" (#49366)" This reverts commit 52c665d.

goderbauer added a commit that referenced this pull request Jan 24, 2020

Revert "Revert "Do not rebuild Routes when a new opaque Route is push…

c9efbd2

…ed on top (#48900)" (#49366)" This reverts commit 52c665d.

jonataslaw mentioned this pull request Feb 6, 2020

Fixes bug where route navigation rebuilds all below materialApp #44731

Closed

7 tasks

goderbauer mentioned this pull request Feb 7, 2020

Nested TickerMode cannot turn tickers back on #50355

Merged

13 tasks

liyuqian added perf: speed c: performance labels May 1, 2020

github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021

	return _Theatre(
	onstage: Stack(
	fit: StackFit.expand,
	children: onstageChildren.reversed.toList(growable: false),
	),
	offstage: offstageChildren,
	);

Do not rebuild Routes when a new opaque Route is pushed on top #48900

Do not rebuild Routes when a new opaque Route is pushed on top #48900

Conversation

goderbauer commented Jan 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

goderbauer commented Jan 15, 2020

Uh oh!

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

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 Jan 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

Hixie commented Jan 21, 2020

Uh oh!

goderbauer commented Jan 22, 2020

Uh oh!

dnfield commented Jan 22, 2020

Uh oh!

dkwingsmt commented Jan 23, 2020

Uh oh!

shamilovtim commented May 11, 2020

Uh oh!

goderbauer commented Jan 15, 2020 •

edited

Loading

goderbauer commented Jan 15, 2020 •

edited

Loading