Remove pipeline in favor of layer tree holder #17688

iskakaushik · 2020-04-13T22:30:00Z

go/flutter-pipeline-improvements for more details

shell/common/animator.cc

shell/common/layer_tree_holder.cc

flar · 2020-04-16T23:25:14Z

One significant issue here is that a lot of timeline tracing seems to have been eliminated in the pipeline files. I'm thinking that the FLOW ids are gone now, and a number of async events as well. Have you looked at the observatory output for how informative it is with these changes? Perhaps post a view of the observatory differences for a given animating app before/after?

iskakaushik · 2020-04-17T19:42:25Z

@flar I haven't looked at the observatory events. I will look at the post-events and see what makes sense in terms of adding support for any of the removed events. In a similar vein, I believe some changes will be needed on the driver tests side to make the benchmarks still be valid.

iskakaushik · 2020-04-20T22:01:13Z

@flar , I've looked at the traces briefly.

Before this change the timeline looked like so:

After my change:

Based on this, I think we aren't losing any useful information. Here are the traces:
https://drive.google.com/drive/folders/1Zo9Sm4Sxy1TZ02b3n6bwD_hbA8QehZci?usp=sharing

flar · 2020-04-20T22:32:09Z

What about the flows, though?

iskakaushik · 2020-04-22T18:05:25Z

This removes 1 flow event: PipelineItem. As I understand it this is used to track 3 events:

When the continuation is obtained.
When the layer tree resource is produced.
When the produced layer tree is consumed.

I will make changes to this PR to track similar events in the layer_tree_holder.cc class.
Other than this event we also track the pipeline depth, but thats no longer needed.

shell/common/layer_tree_holder.cc

shell/common/rasterizer.cc

shell/common/rasterizer.h

shell/common/rasterizer.cc

ci/licenses_golden/licenses_flutter

iskakaushik · 2020-05-07T15:12:43Z

@flar please take a look when you get a chance.

flar

Nothing major, just spelling nits mostly. Approving anyway, but look at my suggested edits...

flar · 2020-05-07T19:04:21Z

shell/common/layer_tree_holder_unittests.cc

+  layer_tree_holder.ReplaceIfNewer(std::move(layer_tree));
+  ASSERT_FALSE(layer_tree_holder.IsEmpty());
+  const auto stored = layer_tree_holder.Get();
+  ASSERT_EQ(stored->frame_size(), frame_size);


also ASSERT_TRUE(isEmpty)?

flar · 2020-05-07T19:08:56Z

shell/common/rasterizer.h

-  ///             actually performs the GPU operations within the layer tree
-  ///             pipeline.
+  ///             The layer tree holder is a thread safe way to produce frame
+  ///             workloads from the UI thread and raster them on the raster


raster -> rasterize

flar · 2020-05-07T19:11:17Z

shell/common/rasterizer.h

+  ///             workloads from the UI thread and raster them on the raster
+  ///             thread. To account for scenarious where the UI thread
+  ///             continues to produce the frames while a raster task is queued,
+  ///             `Rasterizer::DoDraw` that gets executed on the raster thread,


Unnecessary comma at the end of this line separates the noun from its verb, should be "... on the raster thread must pick up ..."

flar · 2020-05-07T19:13:31Z

@chinmaygarde self requested a review a while back, but I don't see any comments...?

go/flutter-pipeline-improvements for more details

iskakaushik · 2020-05-08T17:50:49Z

re-run succeeded: https://ci.chromium.org/p/flutter/builders/try/Mac%20Host%20Engine/3487

…#17688)

This reverts commit 983de2c.

…#17688)

* 9ea2db5 Add FlMessageCodec, FlBinaryCodec, FlStringCodec (flutter/engine#18186) * f422757 Roll src/fuchsia/sdk/linux from MhpFP... to c1q_S... (flutter/engine#18222) * 2ab918c Roll src/third_party/skia a14084ba1b41..8f6c3ed7c7be (1 commits) (flutter/engine#18223) * 40167b6 Make robolectric tests run against SDK 29 by default (flutter/engine#17996) * e5b0db6 Roll src/third_party/skia 8f6c3ed7c7be..b55372444d1b (4 commits) (flutter/engine#18224) * ed08c3e Roll src/fuchsia/sdk/mac from 4MCVP... to T5tT0... (flutter/engine#18228) * 6767517 Roll src/third_party/skia b55372444d1b..ac09f7cd7a28 (2 commits) (flutter/engine#18229) * 983de2c Remove pipeline in favor of layer tree holder (flutter/engine#17688) * a1218dd Roll src/third_party/skia ac09f7cd7a28..c683912173bb (2 commits) (flutter/engine#18230) * 68bf137 skip painting clipped out pictures (flutter/engine#18204) * 7035255 make compiler worker count configurable (flutter/engine#17616) * c8ff03c Publish validation layer deps as part of the fuchsia artifacts (flutter/engine#18214) * 576f0e1 Roll src/third_party/skia c683912173bb..7359165e660c (1 commits) (flutter/engine#18234) * 1b3b4ec skip font loading tests for safari (flutter/engine#18232) * 9319d7c Roll src/third_party/skia 7359165e660c..6913d1bb1d7a (1 commits) (flutter/engine#18237) * 1b56f35 Roll src/third_party/dart 617bc54b715d..2a14a62112e6 (30 commits) (flutter/engine#18239) * ff6942f Add fontFeatures and decorationThickness to textstyle (flutter/engine#18235) * 4418ce8 Revert "Remove pipeline in favor of layer tree holder (#17688)" (flutter/engine#18242) * 9d8daf2 Roll src/third_party/skia 6913d1bb1d7a..bf1904fd4898 (3 commits) (flutter/engine#18243) * Updated bin/internal/fuchsia-linux.version * Updated bin/internal/fuchsia-mac.version

go/flutter-pipeline-improvements for more details.

flutter#18242)

auto-assign bot requested a review from jason-simmons April 13, 2020 22:30

googlebot added the cla: yes label Apr 13, 2020

iskakaushik removed the request for review from jason-simmons April 13, 2020 22:30

chinmaygarde self-requested a review April 13, 2020 23:05

iskakaushik requested a review from flar April 15, 2020 01:30

flar suggested changes Apr 16, 2020

View reviewed changes

shell/common/animator.cc Show resolved Hide resolved

shell/common/layer_tree_holder.cc Outdated Show resolved Hide resolved

shell/common/layer_tree_holder.cc Outdated Show resolved Hide resolved

iskakaushik force-pushed the layer_tree_holder branch from 57c6f8f to 4a4aef5 Compare April 22, 2020 18:36

iskakaushik changed the title ~~[WIP] remove pipeline in favor of layer tree holder~~ Remove pipeline in favor of layer tree holder Apr 22, 2020

flar suggested changes Apr 23, 2020

View reviewed changes

shell/common/layer_tree_holder.cc Outdated Show resolved Hide resolved

shell/common/rasterizer.cc Outdated Show resolved Hide resolved

shell/common/rasterizer.h Outdated Show resolved Hide resolved

iskakaushik force-pushed the layer_tree_holder branch 3 times, most recently from dc971a2 to f3bf7ff Compare April 28, 2020 16:22

iskakaushik force-pushed the layer_tree_holder branch from f3bf7ff to 8041778 Compare May 1, 2020 19:41

flar suggested changes May 1, 2020

View reviewed changes

shell/common/rasterizer.cc Show resolved Hide resolved

shell/common/rasterizer.cc Show resolved Hide resolved

ci/licenses_golden/licenses_flutter Show resolved Hide resolved

flar mentioned this pull request May 4, 2020

Spiky post_backdrop_filter_* benchmarks -- possible lab issue? flutter/flutter#48818

Closed

flar approved these changes May 7, 2020

View reviewed changes

Kaushik Iska added 4 commits May 7, 2020 13:52

Remove pipeline in favor of layer tree holder

2260067

go/flutter-pipeline-improvements for more details

revise docs and address CR comments

68ad126

fix deadlock

15c02ac

add tests

55dacac

iskakaushik force-pushed the layer_tree_holder branch from 85523b5 to 55dacac Compare May 7, 2020 22:13

chinmaygarde approved these changes May 7, 2020

View reviewed changes

iskakaushik merged commit 983de2c into flutter:master May 8, 2020

engine-flutter-autoroll mentioned this pull request May 8, 2020

Roll engine 9b905d3f03f2..983de2c4029d (8 commits) flutter/flutter#56712

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 8, 2020

983de2c Remove pipeline in favor of layer tree holder (flutter/engine…

bebcf1d

…#17688)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 8, 2020

983de2c Remove pipeline in favor of layer tree holder (flutter/engine…

325821c

…#17688)

engine-flutter-autoroll mentioned this pull request May 8, 2020

Roll engine 9b905d3f03f2..703525502b19 (11 commits) flutter/flutter#56718

Closed

iskakaushik pushed a commit to iskakaushik/engine that referenced this pull request May 8, 2020

Revert "Remove pipeline in favor of layer tree holder (flutter#17688)"

f67654e

This reverts commit 983de2c.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 8, 2020

983de2c Remove pipeline in favor of layer tree holder (flutter/engine…

8c349e4

…#17688)

engine-flutter-autoroll mentioned this pull request May 8, 2020

Roll engine 9b905d3f03f2..1b3b4ec88f34 (14 commits) flutter/flutter#56723

Closed

fluttergithubbot pushed a commit that referenced this pull request May 8, 2020

Revert "Remove pipeline in favor of layer tree holder (#17688)" (#18242)

4418ce8

engine-flutter-autoroll mentioned this pull request May 8, 2020

Roll engine 9b905d3f03f2..9319d7c26284 (15 commits) flutter/flutter#56728

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 8, 2020

983de2c Remove pipeline in favor of layer tree holder (flutter/engine…

9b68d34

…#17688)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 9, 2020

983de2c Remove pipeline in favor of layer tree holder (flutter/engine…

494a552

…#17688)

This was referenced May 9, 2020

Roll engine 9b905d3f03f2..ff6942f57b57 (17 commits) flutter/flutter#56737

Closed

Roll engine 9b905d3f03f2..9d8daf2383ea (19 commits) flutter/flutter#56740

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 9, 2020

983de2c Remove pipeline in favor of layer tree holder (flutter/engine…

6ce3b6e

…#17688)

iskakaushik added a commit to iskakaushik/engine that referenced this pull request May 11, 2020

Remove pipeline in favor of layer tree holder (flutter#17688)

9732387

go/flutter-pipeline-improvements for more details.

iskakaushik mentioned this pull request May 11, 2020

Remove pipeline in favor of layer tree holder (#17688) #18285

Merged

iskakaushik added a commit to iskakaushik/engine that referenced this pull request May 12, 2020

Remove pipeline in favor of layer tree holder (flutter#17688)

3474fa7

go/flutter-pipeline-improvements for more details.

iskakaushik added a commit to iskakaushik/engine that referenced this pull request May 12, 2020

Remove pipeline in favor of layer tree holder (flutter#17688)

237249e

go/flutter-pipeline-improvements for more details.

iskakaushik added a commit to iskakaushik/engine that referenced this pull request May 12, 2020

Remove pipeline in favor of layer tree holder (flutter#17688)

5d32050

go/flutter-pipeline-improvements for more details.

iskakaushik added a commit to iskakaushik/engine that referenced this pull request May 12, 2020

Remove pipeline in favor of layer tree holder (flutter#17688)

6105750

go/flutter-pipeline-improvements for more details.

wandyers pushed a commit to wandyers/engine that referenced this pull request May 23, 2020

Remove pipeline in favor of layer tree holder (flutter#17688)

e1d29cc

go/flutter-pipeline-improvements for more details.

wandyers pushed a commit to wandyers/engine that referenced this pull request May 23, 2020

Revert "Remove pipeline in favor of layer tree holder (flutter#17688)" (

c8ad89f

flutter#18242)

Remove pipeline in favor of layer tree holder #17688

Remove pipeline in favor of layer tree holder #17688

Uh oh!

Conversation

iskakaushik commented Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

flar commented Apr 16, 2020

Uh oh!

iskakaushik commented Apr 17, 2020

Uh oh!

iskakaushik commented Apr 20, 2020

Uh oh!

flar commented Apr 20, 2020

Uh oh!

iskakaushik commented Apr 22, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iskakaushik commented May 7, 2020

Uh oh!

flar left a comment

Choose a reason for hiding this comment

Uh oh!

flar May 7, 2020

Choose a reason for hiding this comment

Uh oh!

flar May 7, 2020

Choose a reason for hiding this comment

Uh oh!

flar May 7, 2020

Choose a reason for hiding this comment

Uh oh!

flar commented May 7, 2020

Uh oh!

iskakaushik commented May 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

iskakaushik commented Apr 13, 2020 •

edited

Loading