Replace Pipeline's `ProduceToFront` with `ProduceIfEmpty` to handle thread merging. #17122

cyanglaz · 2020-03-12T22:53:12Z

Part of the effort enabling thread merging.

This is the implementation based on offline discussion with @iskakaushik and @flar, and an update from #9652.

The summary of the discussion can be concluded as following:
When changing thread configuration, we cancel the current frame and hope to resubmit the same layer tree in the next frame. (We will call the frame to be resubmitted "resubmitting frame below") If at the moment when we resubmit, there has already been a new frame inserted into the queue, we drop resubmitting frame, because the new frame in the queue is newer than the resubmitting frame.
For a detailed discussion including all the options we have explored: go/flutter-pipeline-for-thread-merging

Above is a short term solution to handle thread merging under current pipeline structure.

In long term, we want to update the pipeline structure completely, which would make pipeline handling on thread merging much simpler.

related issue: flutter/flutter#23975

flar

Also, would like to see input from @chinmaygarde

flar · 2020-03-13T21:58:29Z

shell/common/rasterizer.cc

+        front_continuation.Complete(std::move(resubmitted_layer_tree_));
+    if (result) {
+      consume_result = PipelineConsumeResult::MoreAvailable;
+    }


It seems to me that if Complete returns false here, then there must be something in the queue, so the call to Complete just above this (line 126?) would probably have already returned MoreAvailable anyway? Am I missing something?

Oh, you are right! Reverting.

chinmaygarde · 2020-03-16T02:55:09Z

This is the implementation based on offline discussion with @iskakaushik and @flar

Can you provide a short summary for folks not in that discussion? I feel like I am missing some context here. Just updating the commit description is fine.

In long term, we want to update the pipeline structure completely, which would make pipeline handling on thread merging much simpler.

Highly in favor of this. The initial implementation was geared towards a set of requirements that have evolved quite a bit and this structure is not longer well suited to solving the same.

cyanglaz · 2020-03-16T16:53:34Z

Can you provide a short summary for folks not in that discussion? I feel like I am missing some context here. Just updating the commit description is fine.

Sure! I have updated the description of the PR. Please let me know if there are still things missing :)

iskakaushik

LGTM modulo requests for doc improvements and a nit about test name.

iskakaushik · 2020-03-18T20:36:43Z

shell/common/pipeline.h

-  // used to en-queue high-priority resources.
-  ProducerContinuation ProduceToFront() {
+  // Create a `ProducerContinuation` that will only push the task if the queue
+  // is empty.


It would be useful to add a brief description for the recommended usage of this method. Something along the lines of:

Prefer using Produce. ProducerContinuation returned by this method doesn't guarantee that the frame will be rendered.

iskakaushik · 2020-03-18T20:38:28Z

shell/common/pipeline_unittests.cc

 }

-TEST(PipelineTest, PushingToFrontOverridesOrder) {
+TEST(PipelineTest, ProduceIfEmptyWhenQueueIsNotEmptyDoesNotConsume) {


rename test to ProduceIfEmptyDoesNotConsumeWhenQueueIsNotEmpty

iskakaushik · 2020-03-18T20:38:58Z

shell/common/pipeline.h

-      while (queue_.size() > depth_) {
-        queue_.pop_back();
+      if (!queue_.empty()) {
+        empty_.Signal();


can you add a note here describing why this semaphore needs to be signaled?

cyanglaz

@iskakaushik Updated based on your comments!

cyanglaz · 2020-03-19T17:13:52Z

shell/common/pipeline.h

-  // used to en-queue high-priority resources.
-  ProducerContinuation ProduceToFront() {
+  // Create a `ProducerContinuation` that will only push the task if the queue
+  // is empty.


cyanglaz · 2020-03-19T17:13:57Z

shell/common/pipeline.h

-      while (queue_.size() > depth_) {
-        queue_.pop_back();
+      if (!queue_.empty()) {
+        empty_.Signal();


cyanglaz · 2020-03-19T17:14:33Z

shell/common/pipeline_unittests.cc

 }

-TEST(PipelineTest, PushingToFrontOverridesOrder) {
+TEST(PipelineTest, ProduceIfEmptyWhenQueueIsNotEmptyDoesNotConsume) {


iskakaushik

LGTM

…handle thread merging. (flutter/engine#17122)

…handle thread merging. (flutter/engine#17122) (#52972)

produceIfEmpty

f6b8e69

auto-assign bot requested a review from GaryQian March 12, 2020 22:53

cyanglaz requested review from iskakaushik and removed request for GaryQian March 12, 2020 22:53

googlebot added the cla: yes label Mar 12, 2020

cyanglaz requested a review from flar March 12, 2020 22:53

cyanglaz assigned amirh and unassigned amirh Mar 12, 2020

cyanglaz requested a review from amirh March 12, 2020 22:53

cyanglaz mentioned this pull request Mar 13, 2020

Add a front queue to pipeline so the produceToFront can run safely. #16961

Closed

flar reviewed Mar 13, 2020

View reviewed changes

flar requested a review from chinmaygarde March 13, 2020 21:59

Chris Yang added 2 commits March 13, 2020 15:39

revert complete to return void

5522461

revert comments

6c1e5dc

iskakaushik approved these changes Mar 18, 2020

View reviewed changes

cyanglaz commented Mar 19, 2020

View reviewed changes

review updates

a60d0f9

chinmaygarde approved these changes Mar 19, 2020

View reviewed changes

iskakaushik approved these changes Mar 20, 2020

View reviewed changes

Merge branch 'master' into pipeline_produce_to_front_if_empty

0d38430

cyanglaz merged commit 2d42c74 into flutter:master Mar 20, 2020

cyanglaz deleted the pipeline_produce_to_front_if_empty branch March 20, 2020 18:05

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

2d42c74 Replace Pipeline's ProduceToFront with ProduceIfEmpty to …

d0275e4

…handle thread merging. (flutter/engine#17122)

engine-flutter-autoroll mentioned this pull request Mar 20, 2020

Roll engine 77e6d32a1ebb..2d42c74a348d (1 commits) flutter/flutter#52972

Merged

fluttergithubbot pushed a commit to flutter/flutter that referenced this pull request Mar 20, 2020

2d42c74 Replace Pipeline's ProduceToFront with ProduceIfEmpty to …

a0d93f9

…handle thread merging. (flutter/engine#17122) (#52972)

cyanglaz mentioned this pull request Aug 25, 2021

Set pipeline depth properly when dynamic thread merging is enabled for iOS platform views flutter/flutter#37665

Closed

Replace Pipeline's ProduceToFront with ProduceIfEmpty to handle thread merging. #17122

Replace Pipeline's ProduceToFront with ProduceIfEmpty to handle thread merging. #17122

Uh oh!

Conversation

cyanglaz commented Mar 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

chinmaygarde commented Mar 16, 2020

Uh oh!

cyanglaz commented Mar 16, 2020

Uh oh!

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

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

iskakaushik left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Replace Pipeline's `ProduceToFront` with `ProduceIfEmpty` to handle thread merging. #17122

Replace Pipeline's `ProduceToFront` with `ProduceIfEmpty` to handle thread merging. #17122

cyanglaz commented Mar 12, 2020 •

edited

Loading