Colored box and container optimization by dnfield · Pull Request #50979 · flutter/flutter

dnfield · 2020-02-18T18:30:46Z

Description

It's fairly common to create a Container that has just a color property and no other decoration properties on it.

We currently go down a slightly more expensive path than necessary to just draw a simple background color.

This PR adds a ColoredBox widget, and optimizes container to use that widget. It also lifts the restriction that Container specify either color or decoration.

Related Issues

Fixes #9672
Breaks suggestions in #28753

Tests

I added the following tests:

Tests that the new widget avoids drawing when its size is zero, and paints its child if it has one.

The complex_layout_scroll_perf test showed a slight improvement in everything but 99th percentile frame build and rasterizer times - although I strongly suspect that's mainly noise, and I'm also not running on a Moto G4. I anticipate a small improvement in several of our benchmarks due to this patch.

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.

Yes, this is a breaking change.

Migration guide: flutter/website#3728

skia-gold · 2020-02-18T18:38:18Z

Gold has detected one or more untriaged digests on patchset 2.
Please triage them at https://flutter-gold.skia.org/search?issue=50979.

HansMuller · 2020-02-18T18:37:42Z

+        ],
+      ));
+      expect(find.byType(ColoredBox), findsOneWidget);
+      final RenderColoredBox renderColoredBox = tester.renderObject(find.byType(ColoredBox)) as RenderColoredBox;


tester.renderObject<RenderColoredBox>

HansMuller · 2020-02-18T18:43:06Z

+
+  @override
+  void paint(PaintingContext context, Offset offset) {
+    if (size != Size.zero) {


I suppose to cover the corner of this corner case: (size > Size.zero)

HansMuller · 2020-02-18T18:44:09Z

+}
+
+/// A render object that paints its area with the specified [Color].
+class RenderColoredBox extends RenderProxyBox {


Does this class need to be public?

I suppose not. Making it private and will update the test.

dnfield · 2020-02-18T19:01:09Z

This turns out to be a breaking change - we have tests in the framework that assert the right color is painted by looking for the BoxDecoration that a Container creates for the color.

I think the right thing to do here is to add a matcher (which would fix #28753) and migrate tests to use that, in a separate PR. We can then migrate any internal customers to make that a soft break.

goderbauer · 2020-02-18T21:47:46Z

Cirrus seems unhappy.

goderbauer · 2020-02-18T21:48:31Z

 }
+
+/// A widget that paints its area with a specified [Color] and then draws its
+/// child.


maybe be specific and say that the child is drawn on top of that color?

(even though that's kinda obvious, I guess..)

goderbauer · 2020-02-18T21:49:07Z

+class ColoredBox extends SingleChildRenderObjectWidget {
+  /// Creates a widget that paints its area with the specified [Color].
+  ///
+  /// The `color` parameter must not be null.


color should be in [] since it exists as a property.

goderbauer · 2020-02-18T21:51:34Z

-  /// `BoxDecoration`.
+  /// The `color` and `decoration` arguments cannot both be supplied, since
+  /// it would potentially result in the decoration drawing over the background
+  /// color. To supply a decoration with a color, use `decoration: new


drop the new here since it is no optional?

goderbauer · 2020-02-18T21:54:32Z

+class _RenderColoredBox extends RenderProxyBoxWithHitTestBehavior {
+  _RenderColoredBox({@required Color color})
+    : _color = color,
+      super(behavior: color.alpha == 0 ? HitTestBehavior.deferToChild : HitTestBehavior.opaque);


This surprised me. Was the container doing anything special with hittest behavior based on transparency?

I'm going to file a new bug for this: #51000 - agree we shouldn't introduce new behavior here (and not behavior that would differ from using a decoration)

liyuqian · 2020-02-18T23:01:06Z

+        textDirection: TextDirection.ltr,
+        children: const <Widget>[
+          SizedBox.shrink(
+            child: ColoredBox(color: colorToPaint, child: child),


Does the child = SizedBox.expand(key: key) also have 0 size? My first reaction is that a parent ColoredBox is at least the size of its child, so a 0-sized parent ColoredBox infers a 0-sized child.

We have to break out of the constraints of the tester to make any of this work - we could also use a child instead of a parent to do it, but we need to make sure we're in a context where we won't just take up the full available space anwyya (e.g. using a flexible).

dnfield · 2020-02-19T00:30:25Z


    final Container container = _getContainerFromBanner(tester);
-    expect(container.decoration, const BoxDecoration(color: color));
+    expect(container.color, color);


Here and elsewhere: A simple container with a color no longer creates a BoxDecoration. This is the break.

When making a change like this, are there metrics for before and after? I'm curious about rendering speed (which I would expect to improve) and app size (which I would expect to increase slightly).

I recorded but have lost some performance gains in various timeline summary benchmarks we run. The results are not earth shaking, but it is a bump in performance for frame times - we use a lot of containers in the framework, and containers are a very common widget to use.

On the Flutter Gallery, building with this patch is not enough to meaningfully impact the app.so/app.dill sizes when building the release AOT bundle. They measure using the same number of blocks on my local disk with and without this patch.

If I generate the heapsnapshots for each, the one with the ColoredBox takes slightly more space, indicating that the colored box is getting properly shaken out of both types.

The added classes are pretty tiny though :)

Thanks for the info. Reading through this, I was suddenly curious what the life cycle of an SDK optimization like this was.

dnfield · 2020-02-19T00:32:27Z

              itemExtent: 150,
              delegate: SliverChildBuilderDelegate(
-                  (BuildContext context, int index) => Container(color: Colors.amber),
+                (BuildContext context, int index) {


@Piinks - I've refactored this test to avoid checking the color(s) of the various slivers, and instead more explicitly check semantics labels corresponding to indexes.

This test would have failed because Container no longer creates a BoxDecoration in this case. Other approaches would include:

Explicitly passing a decoration to the container we create in this test

Using Keys - probably bad because it could affect ordering of the list, but this test doesn't seemto need that

Refactor the test to use text widgets inside the slivers, which seemed more invasive.

Sgtm! Thanks for updating the test!

dnfield · 2020-02-19T00:32:57Z

    // Screen is 600px high. Moved bottom item 500px up. It's now at the top.
-    expect(tester.getTopLeft(find.widgetWithText(DecoratedBox, '5')).dy, 0.0);
-    expect(tester.getBottomLeft(find.widgetWithText(DecoratedBox, '10')).dy, 600.0);
+    expect(tester.getTopLeft(find.widgetWithText(Container, '5')).dy, 0.0);


This test was looking for an implementation detail of Container, no real need to, and that detail doesn't get created anymore in this case.

In fairness, it was an evil test. 😄

dnfield · 2020-02-19T00:34:39Z

Landing this will require updating tests in our own packages repo.

For flutter/flutter#50979

dnfield · 2020-02-20T01:29:17Z

I think the next step here is to work on a migration guide. I'll also try to run this through an internal test roll to see what the fallout is there.

goderbauer

LGTM

goderbauer · 2020-02-24T17:30:33Z


+  /// The color to paint behind the [child].
+  ///
+  /// This will paint behind the [decoration], if any.


Update this comment since you can no longer have both?

dnfield · 2020-02-25T05:29:13Z

This should not land until cl/296911768

redbrogdon · 2020-02-25T20:43:42Z

       assert(color == null || decoration == null,
         'Cannot provide both a color and a decoration\n'
-         'The color argument is just a shorthand for "decoration: new BoxDecoration(color: color)".'
+         'To provide both, use "decoration: new BoxDecoration(color: color)".'


nit: Don't need the new here?

redbrogdon · 2020-02-25T20:47:01Z

    // Screen is 600px high. Moved bottom item 500px up. It's now at the top.
-    expect(tester.getTopLeft(find.widgetWithText(DecoratedBox, '5')).dy, 0.0);
-    expect(tester.getBottomLeft(find.widgetWithText(DecoratedBox, '10')).dy, 600.0);
+    expect(tester.getTopLeft(find.widgetWithText(Container, '5')).dy, 0.0);


In fairness, it was an evil test. 😄

redbrogdon · 2020-02-25T20:48:28Z


    final Container container = _getContainerFromBanner(tester);
-    expect(container.decoration, const BoxDecoration(color: color));
+    expect(container.color, color);


When making a change like this, are there metrics for before and after? I'm curious about rendering speed (which I would expect to improve) and app size (which I would expect to increase slightly).

dnfield · 2020-02-25T23:35:10Z

Internal CL has landed. This is safe to land as a soft break at this point.

* migration guide for flutter/flutter#50979 * review * revert unintended change * Update container-color.md

dnfield added 2 commits February 18, 2020 10:25

Colored box and container optimization

18b5bd6

stale doc

e7f1e25

fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Feb 18, 2020

googlebot added the cla: yes label Feb 18, 2020

dnfield requested review from HansMuller, goderbauer and liyuqian February 18, 2020 18:30

HansMuller reviewed Feb 18, 2020

View reviewed changes

Review, add missing impl method

6872571

dnfield added 3 commits February 18, 2020 11:11

hit testing

694389e

better hit test?

73e6034

add back assert

04d5eb1

goderbauer reviewed Feb 18, 2020

View reviewed changes

liyuqian reviewed Feb 18, 2020

View reviewed changes

dnfield added 3 commits February 18, 2020 15:38

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

737517a

Fix tests

14d0d4e

more breaks

ae49a8a

fluttergithubbot added the p: material_ui material_ui package in flutter/packages label Feb 19, 2020

dnfield commented Feb 19, 2020

View reviewed changes

dnfield mentioned this pull request Feb 19, 2020

Temporary fix for test with upcoming breaking change flutter/packages#98

Merged

dnfield added a commit to flutter/tests that referenced this pull request Feb 19, 2020

Update flutter_packages.test

647267b

For flutter/flutter#50979

dnfield mentioned this pull request Feb 19, 2020

Update flutter_packages.test flutter/tests#21

Merged

Piinks added c: API break Backwards-incompatible API changes a: tests "flutter test", flutter_test, or one of our tests labels Feb 19, 2020

Piinks added perf: speed Performance issues related to (mostly rendering) speed c: performance Relates to speed or footprint issues (see "perf:" labels) labels Feb 19, 2020

dnfield added a commit to flutter/tests that referenced this pull request Feb 19, 2020

Update flutter_packages.test (#21)

73cd490

For flutter/flutter#50979

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

9e765b4

dnfield added a commit to dnfield/website that referenced this pull request Feb 23, 2020

migration guide for flutter/flutter#50979

43fe09e

dnfield mentioned this pull request Feb 23, 2020

migration guide for ColoredBox/container optimization flutter/website#3728

Merged

goderbauer approved these changes Feb 24, 2020

View reviewed changes

dnfield added 2 commits February 24, 2020 09:39

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

ac486d1

update doc

ae1d6a7

redbrogdon reviewed Feb 25, 2020

View reviewed changes

dnfield added 2 commits February 25, 2020 14:40

drop optional new

03d095a

oops

670e57d

redbrogdon approved these changes Feb 26, 2020

View reviewed changes

dnfield added 2 commits February 26, 2020 12:46

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

1be4f78

oops again

195353d

dnfield merged commit 444b13b into flutter:master Feb 27, 2020

dnfield deleted the colored_box branch February 27, 2020 00:04

terrylucas mentioned this pull request Feb 27, 2020

Travis failure looks like it started happening with the latest Flutter roll. flutter/devtools#1677

Closed

dnfield mentioned this pull request Mar 5, 2020

Make color property accessible when testing Container widgets #28753

Closed

sfshaza2 added a commit to flutter/website that referenced this pull request Mar 6, 2020

migration guide for ColoredBox/container optimization (#3728)

c4af085

* migration guide for flutter/flutter#50979 * review * revert unintended change * Update container-color.md

shihaohong mentioned this pull request May 27, 2020

[animations] Migrate _getScrimColor to look for ColoredBox flutter/packages#163

Merged

shihaohong mentioned this pull request Jul 13, 2020

ListTile should provide ability to set background color #33392

Closed

github-actions Bot locked as resolved and limited conversation to collaborators Aug 1, 2021

Conversation

dnfield commented Feb 18, 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!

skia-gold commented Feb 18, 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!

dnfield commented Feb 18, 2020

Uh oh!

goderbauer commented Feb 18, 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!

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!

dnfield commented Feb 19, 2020

Uh oh!

dnfield commented Feb 20, 2020

Uh oh!

goderbauer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield commented Feb 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield commented Feb 18, 2020 •

edited

Loading