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

iskakaushik · 2020-05-11T22:03:06Z

go/flutter-pipeline-improvements for more details.

iskakaushik · 2020-05-11T22:04:22Z

I'm attempting to re-land this change after identifying and fixing an underlying race in #18279.

The original change that got reverted was: #17688

chinmaygarde · 2020-05-12T02:32:54Z

shell/common/animator.cc

-              ? 1
-              : 2)),
-#endif  // FLUTTER_SHELL_ENABLE_METAL
+      layer_tree_holder_(std::make_shared<LayerTreeHolder>()),


chinmaygarde · 2020-05-12T02:35:39Z

shell/common/rasterizer.cc

-      break;
+  // Note: This behaviour is left as-is to be inline with the pipeline
+  // semantics. TODO(kaushikiska): explore removing this block.
+  if (!layer_tree_holder->IsEmpty()) {


Whats making this method confusing to me is the re-submission logic. IMO, we should get rid of the resubmitted_layer_tree_ and have the caller to DoDraw (this frame) insert the layer tree that because of resubmit insert that tree back into the holder. We can then make DoDraw const so we its easier to read.

I believe we can get rid of this block if we say that whoever pushes a layertree to holder also has to perform a draw explicitly.

There is an entire comment section below about how thread merging interacts with pipeline pressure that is now irrelevant and confusing(GitHub wont allow me to comment on that line in this patch but the link is here). I think we need to do a review of the documentation about to make sure outdated concepts are removed. Specifically, we need to make sure we have consistent documentation that discusses the following edge cases:

1: What happens when there is frame re-resubmission because of thread merging and unmerging?
2: What happens when new layer tree are not being produced in case of external texture composition?

Fortunately, this new world is simpler as there is no concept of pipeline backpressure.

I 100% agree with this. One of the motivations for me to remove the pipeline mechanism is to facilitate easier thread merging and resubmission. I'd like to keep the thread merging behavior exactly the same for this commit and will simplify the resubmission logic along with clearing up some of the RasterStatus enums that are no longer valid.

chinmaygarde · 2020-05-12T02:42:39Z

shell/common/layer_tree_holder.h

+
+namespace flutter {
+
+class LayerTreeHolder {


Documentation for this class. The following would be good to answer in this docset:
1: Who owns the holder?
2: Threading requirements.
3: Which components typically push and pop a layer trees?
4: What does the new-ness of layer trees mean exactly (clarify its the target time)?

chinmaygarde · 2020-05-12T02:43:12Z

shell/common/layer_tree_holder.h

+
+  bool IsEmpty() const;
+
+  std::unique_ptr<LayerTree> Get();


How about PushIfNewer and Pop? Get implies a getter that is idempotent.

chinmaygarde · 2020-05-12T02:43:37Z

shell/common/layer_tree_holder.h

+
+  bool IsEmpty() const;
+
+  std::unique_ptr<LayerTree> Get();


[[nodiscard]]

chinmaygarde · 2020-05-12T02:43:45Z

shell/common/layer_tree_holder.h

+
+class LayerTreeHolder {
+ public:
+  LayerTreeHolder() = default;


go/flutter-pipeline-improvements for more details.

…#18285)

* 9e3e3ba [web] Represent CSS identity transforms as 'none' instead of null (flutter/engine#18288) * 80fa77e Roll src/third_party/skia 3ebadcc98eab..056d543c91e0 (8 commits) (flutter/engine#18344) * 480d8e4 Fix scenario platform view tests on Android (flutter/engine#18332) * 0385664 Roll src/third_party/dart d6fed1f62444..29c00e28f350 (16 commits) (flutter/engine#18347) * 8371b44 Roll src/third_party/skia 056d543c91e0..71903997254f (7 commits) (flutter/engine#18350) * f321613 Add guards on FlValue methods to check for NULL values (flutter/engine#18226) * dc93db5 Move FlutterLoader disk I/O to a background thread to comply with Android strict mode (flutter/engine#18241) * bf1287c null-annotate lerp.dart, annotations.dart, channel_buffers.dart, hash_codes.dart (flutter/engine#18348) * 9600354 Add FlBasicMessageChannel (flutter/engine#18189) * df2dfac Roll src/third_party/skia 71903997254f..6c3db04c8b03 (9 commits) (flutter/engine#18361) * 5ad4f9e Add FlJsonMessageCodec (flutter/engine#18221) * 84ea892 Roll src/third_party/skia 6c3db04c8b03..7156db260239 (4 commits) (flutter/engine#18370) * f848069 Roll src/third_party/dart 29c00e28f350..f99631b12c4a (29 commits) (flutter/engine#18373) * c791ce9 Roll src/fuchsia/sdk/mac from gOhJW... to Vepm4... (flutter/engine#18377) * 5b62a63 Roll src/third_party/dart f99631b12c4a..e0257265d34e (2 commits) (flutter/engine#18378) * 0b41009 Roll src/third_party/skia 7156db260239..5b2ede3d0d44 (8 commits) (flutter/engine#18380) * 08b61ce Delete unused decode UTF-8, JSON functions (flutter/engine#18360) * 73d835c Roll src/third_party/dart e0257265d34e..2676764792b2 (4 commits) (flutter/engine#18383) * 9e166fb Roll src/third_party/skia 5b2ede3d0d44..39ec60aa8348 (5 commits) (flutter/engine#18384) * 2cdbc7f Remove pipeline in favor of layer tree holder (flutter/engine#18285) * 47513a7 Roll src/third_party/skia 39ec60aa8348..79c5674a4ca1 (3 commits) (flutter/engine#18389)

This reverts commit 2cdbc7f.

go/flutter-pipeline-improvements for more details.

flutter#18427) This reverts commit 2cdbc7f.

iskakaushik requested a review from jason-simmons May 11, 2020 22:03

googlebot added cla: yes labels May 11, 2020

auto-assign bot requested a review from GaryQian May 11, 2020 22:03

iskakaushik requested review from chinmaygarde and removed request for GaryQian May 11, 2020 22:04

iskakaushik force-pushed the reland-pipeline-removal branch from 9732387 to 3474fa7 Compare May 12, 2020 01:21

chinmaygarde reviewed May 12, 2020

View reviewed changes

iskakaushik force-pushed the reland-pipeline-removal branch 2 times, most recently from d3686ce to 57801c2 Compare May 12, 2020 22:58

iskakaushik and others added 2 commits May 12, 2020 16:37

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

6105750

go/flutter-pipeline-improvements for more details.

rename and add docs

0b8fe08

iskakaushik force-pushed the reland-pipeline-removal branch from 57801c2 to 0b8fe08 Compare May 12, 2020 23:37

chinmaygarde approved these changes May 13, 2020

View reviewed changes

iskakaushik added waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. and removed waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. labels May 13, 2020

iskakaushik merged commit 2cdbc7f into flutter:master May 14, 2020

iskakaushik deleted the reland-pipeline-removal branch May 14, 2020 17:46

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

Roll engine ae2222f47e78..2cdbc7f92793 (20 commits) flutter/flutter#57241

Closed

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

2cdbc7f Remove pipeline in favor of layer tree holder (flutter/engine…

cdea622

…#18285)

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

2cdbc7f Remove pipeline in favor of layer tree holder (flutter/engine…

09f1ef1

…#18285)

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

Roll engine ae2222f47e78..47513a70eb6d (21 commits) flutter/flutter#57252

Merged

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

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

c04a822

This reverts commit 2cdbc7f.

iskakaushik added a commit that referenced this pull request May 15, 2020

Revert "Remove pipeline in favor of layer tree holder (#18285)" (#18427)

2494d1c

This reverts commit 2cdbc7f.

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

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

38b97ce

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#18285)" (

c13b3d8

flutter#18427) This reverts commit 2cdbc7f.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

iskakaushik commented May 11, 2020

Uh oh!

iskakaushik commented May 11, 2020

Uh oh!

chinmaygarde May 12, 2020

Uh oh!

chinmaygarde May 12, 2020

Uh oh!

chinmaygarde May 12, 2020

Uh oh!

iskakaushik May 12, 2020

Uh oh!

chinmaygarde May 12, 2020

Uh oh!

iskakaushik May 12, 2020

Uh oh!

chinmaygarde May 12, 2020

Uh oh!

iskakaushik May 12, 2020

Uh oh!

chinmaygarde May 12, 2020

Uh oh!

chinmaygarde May 12, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants


		namespace flutter {

		class LayerTreeHolder {

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

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

Uh oh!

Conversation

iskakaushik commented May 11, 2020

Uh oh!

iskakaushik commented May 11, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants