From f6b8e695eedab060690a23a73df91621a3a91b27 Mon Sep 17 00:00:00 2001 From: Chris Yang Date: Thu, 12 Mar 2020 15:49:25 -0700 Subject: [PATCH 1/4] produceIfEmpty --- shell/common/pipeline.h | 56 +++++++++++++++++++----------- shell/common/pipeline_unittests.cc | 31 ++++++----------- shell/common/rasterizer.cc | 9 +++-- 3 files changed, 51 insertions(+), 45 deletions(-) diff --git a/shell/common/pipeline.h b/shell/common/pipeline.h index 587c81ff6fbc2..54461b46d68e1 100644 --- a/shell/common/pipeline.h +++ b/shell/common/pipeline.h @@ -60,20 +60,25 @@ class Pipeline : public fml::RefCountedThreadSafe> { } } - void Complete(ResourcePtr resource) { - if (continuation_) { - continuation_(std::move(resource), trace_id_); - continuation_ = nullptr; - TRACE_EVENT_ASYNC_END0("flutter", "PipelineProduce", trace_id_); - TRACE_FLOW_STEP("flutter", "PipelineItem", trace_id_); + // Return `true` if successful. + // + // If a `ProducerContinuation` is created with `ProduceIfEmpty`, `Complete` + // returns `false` if the queue is not empty. + bool Complete(ResourcePtr resource) { + if (!continuation_ || !continuation_(std::move(resource), trace_id_)) { + return false; } + continuation_ = nullptr; + TRACE_EVENT_ASYNC_END0("flutter", "PipelineProduce", trace_id_); + TRACE_FLOW_STEP("flutter", "PipelineItem", trace_id_); + return true; } operator bool() const { return continuation_ != nullptr; } private: friend class Pipeline; - using Continuation = std::function; + using Continuation = std::function; Continuation continuation_; size_t trace_id_; @@ -111,16 +116,20 @@ class Pipeline : public fml::RefCountedThreadSafe> { GetNextPipelineTraceID()}; // trace id } - // Pushes task to the front of the pipeline. - // - // If we exceed the depth completing this continuation, we drop the - // last frame to preserve the depth of the pipeline. - // - // Note: Use |Pipeline::Produce| where possible. This should only be - // used to en-queue high-priority resources. - ProducerContinuation ProduceToFront() { + // Create a `ProducerContinuation` that will only push the task if the queue + // is empty. + ProducerContinuation ProduceIfEmpty() { + if (!empty_.TryWait()) { + return {}; + } + ++inflight_; + FML_TRACE_COUNTER("flutter", "Pipeline Depth", + reinterpret_cast(this), // + "frames in flight", inflight_.load() // + ); + return ProducerContinuation{ - std::bind(&Pipeline::ProducerCommitFront, this, std::placeholders::_1, + std::bind(&Pipeline::ProducerCommitIfEmpty, this, std::placeholders::_1, std::placeholders::_2), // continuation GetNextPipelineTraceID()}; // trace id } @@ -171,7 +180,8 @@ class Pipeline : public fml::RefCountedThreadSafe> { std::mutex queue_mutex_; std::deque> queue_; - void ProducerCommit(ResourcePtr resource, size_t trace_id) { + // Always returns `true` as it is always successful. + bool ProducerCommit(ResourcePtr resource, size_t trace_id) { { std::scoped_lock lock(queue_mutex_); queue_.emplace_back(std::move(resource), trace_id); @@ -179,19 +189,23 @@ class Pipeline : public fml::RefCountedThreadSafe> { // Ensure the queue mutex is not held as that would be a pessimization. available_.Signal(); + return true; } - void ProducerCommitFront(ResourcePtr resource, size_t trace_id) { + // Returns `true` if successful. + bool ProducerCommitIfEmpty(ResourcePtr resource, size_t trace_id) { { std::scoped_lock lock(queue_mutex_); - queue_.emplace_front(std::move(resource), trace_id); - while (queue_.size() > depth_) { - queue_.pop_back(); + if (!queue_.empty()) { + empty_.Signal(); + return false; } + queue_.emplace_back(std::move(resource), trace_id); } // Ensure the queue mutex is not held as that would be a pessimization. available_.Signal(); + return true; } FML_DISALLOW_COPY_AND_ASSIGN(Pipeline); diff --git a/shell/common/pipeline_unittests.cc b/shell/common/pipeline_unittests.cc index ada908953b45a..1b3917a0898a5 100644 --- a/shell/common/pipeline_unittests.cc +++ b/shell/common/pipeline_unittests.cc @@ -89,46 +89,35 @@ TEST(PipelineTest, PushingMultiProcessesInOrder) { ASSERT_EQ(consume_result_2, PipelineConsumeResult::Done); } -TEST(PipelineTest, PushingToFrontOverridesOrder) { +TEST(PipelineTest, ProduceIfEmptyWhenQueueIsNotEmptyDoesNotConsume) { const int depth = 2; fml::RefPtr pipeline = fml::MakeRefCounted(depth); Continuation continuation_1 = pipeline->Produce(); - Continuation continuation_2 = pipeline->ProduceToFront(); + Continuation continuation_2 = pipeline->ProduceIfEmpty(); const int test_val_1 = 1, test_val_2 = 2; continuation_1.Complete(std::make_unique(test_val_1)); - continuation_2.Complete(std::make_unique(test_val_2)); + ASSERT_EQ(continuation_2.Complete(std::make_unique(test_val_2)), false); PipelineConsumeResult consume_result_1 = pipeline->Consume( - [&test_val_2](std::unique_ptr v) { ASSERT_EQ(*v, test_val_2); }); - ASSERT_EQ(consume_result_1, PipelineConsumeResult::MoreAvailable); - - PipelineConsumeResult consume_result_2 = pipeline->Consume( [&test_val_1](std::unique_ptr v) { ASSERT_EQ(*v, test_val_1); }); - ASSERT_EQ(consume_result_2, PipelineConsumeResult::Done); + ASSERT_EQ(consume_result_1, PipelineConsumeResult::Done); } -TEST(PipelineTest, PushingToFrontDropsLastResource) { - const int depth = 2; +TEST(PipelineTest, ProduceIfEmptySuccessfulIf) { + const int depth = 1; fml::RefPtr pipeline = fml::MakeRefCounted(depth); - Continuation continuation_1 = pipeline->Produce(); - Continuation continuation_2 = pipeline->Produce(); - Continuation continuation_3 = pipeline->ProduceToFront(); + Continuation continuation_1 = pipeline->ProduceIfEmpty(); + Continuation continuation_2 = pipeline->ProduceIfEmpty(); - const int test_val_1 = 1, test_val_2 = 2, test_val_3 = 3; + const int test_val_1 = 1; continuation_1.Complete(std::make_unique(test_val_1)); - continuation_2.Complete(std::make_unique(test_val_2)); - continuation_3.Complete(std::make_unique(test_val_3)); PipelineConsumeResult consume_result_1 = pipeline->Consume( - [&test_val_3](std::unique_ptr v) { ASSERT_EQ(*v, test_val_3); }); - ASSERT_EQ(consume_result_1, PipelineConsumeResult::MoreAvailable); - - PipelineConsumeResult consume_result_2 = pipeline->Consume( [&test_val_1](std::unique_ptr v) { ASSERT_EQ(*v, test_val_1); }); - ASSERT_EQ(consume_result_2, PipelineConsumeResult::Done); + ASSERT_EQ(consume_result_1, PipelineConsumeResult::Done); } } // namespace testing diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc index acad5ada8510a..3d87889157196 100644 --- a/shell/common/rasterizer.cc +++ b/shell/common/rasterizer.cc @@ -127,9 +127,12 @@ void Rasterizer::Draw(fml::RefPtr> pipeline) { // if the raster status is to resubmit the frame, we push the frame to the // front of the queue and also change the consume status to more available. if (raster_status == RasterStatus::kResubmit) { - auto front_continuation = pipeline->ProduceToFront(); - front_continuation.Complete(std::move(resubmitted_layer_tree_)); - consume_result = PipelineConsumeResult::MoreAvailable; + auto front_continuation = pipeline->ProduceIfEmpty(); + bool result = + front_continuation.Complete(std::move(resubmitted_layer_tree_)); + if (result) { + consume_result = PipelineConsumeResult::MoreAvailable; + } } else if (raster_status == RasterStatus::kEnqueuePipeline) { consume_result = PipelineConsumeResult::MoreAvailable; } From 552246166a5fd3b2026b9230f146b4f379fa591f Mon Sep 17 00:00:00 2001 From: Chris Yang Date: Fri, 13 Mar 2020 15:39:45 -0700 Subject: [PATCH 2/4] revert complete to return void --- shell/common/pipeline.h | 23 ++++++++++------------- shell/common/pipeline_unittests.cc | 5 ++--- shell/common/rasterizer.cc | 6 +----- 3 files changed, 13 insertions(+), 21 deletions(-) diff --git a/shell/common/pipeline.h b/shell/common/pipeline.h index 54461b46d68e1..ecc3cdbf9e127 100644 --- a/shell/common/pipeline.h +++ b/shell/common/pipeline.h @@ -64,21 +64,20 @@ class Pipeline : public fml::RefCountedThreadSafe> { // // If a `ProducerContinuation` is created with `ProduceIfEmpty`, `Complete` // returns `false` if the queue is not empty. - bool Complete(ResourcePtr resource) { - if (!continuation_ || !continuation_(std::move(resource), trace_id_)) { - return false; + void Complete(ResourcePtr resource) { + if (continuation_) { + continuation_(std::move(resource), trace_id_); + continuation_ = nullptr; + TRACE_EVENT_ASYNC_END0("flutter", "PipelineProduce", trace_id_); + TRACE_FLOW_STEP("flutter", "PipelineItem", trace_id_); } - continuation_ = nullptr; - TRACE_EVENT_ASYNC_END0("flutter", "PipelineProduce", trace_id_); - TRACE_FLOW_STEP("flutter", "PipelineItem", trace_id_); - return true; } operator bool() const { return continuation_ != nullptr; } private: friend class Pipeline; - using Continuation = std::function; + using Continuation = std::function; Continuation continuation_; size_t trace_id_; @@ -181,7 +180,7 @@ class Pipeline : public fml::RefCountedThreadSafe> { std::deque> queue_; // Always returns `true` as it is always successful. - bool ProducerCommit(ResourcePtr resource, size_t trace_id) { + void ProducerCommit(ResourcePtr resource, size_t trace_id) { { std::scoped_lock lock(queue_mutex_); queue_.emplace_back(std::move(resource), trace_id); @@ -189,23 +188,21 @@ class Pipeline : public fml::RefCountedThreadSafe> { // Ensure the queue mutex is not held as that would be a pessimization. available_.Signal(); - return true; } // Returns `true` if successful. - bool ProducerCommitIfEmpty(ResourcePtr resource, size_t trace_id) { + void ProducerCommitIfEmpty(ResourcePtr resource, size_t trace_id) { { std::scoped_lock lock(queue_mutex_); if (!queue_.empty()) { empty_.Signal(); - return false; + return; } queue_.emplace_back(std::move(resource), trace_id); } // Ensure the queue mutex is not held as that would be a pessimization. available_.Signal(); - return true; } FML_DISALLOW_COPY_AND_ASSIGN(Pipeline); diff --git a/shell/common/pipeline_unittests.cc b/shell/common/pipeline_unittests.cc index 1b3917a0898a5..6e500e5bf36a9 100644 --- a/shell/common/pipeline_unittests.cc +++ b/shell/common/pipeline_unittests.cc @@ -98,19 +98,18 @@ TEST(PipelineTest, ProduceIfEmptyWhenQueueIsNotEmptyDoesNotConsume) { const int test_val_1 = 1, test_val_2 = 2; continuation_1.Complete(std::make_unique(test_val_1)); - ASSERT_EQ(continuation_2.Complete(std::make_unique(test_val_2)), false); + continuation_2.Complete(std::make_unique(test_val_2)); PipelineConsumeResult consume_result_1 = pipeline->Consume( [&test_val_1](std::unique_ptr v) { ASSERT_EQ(*v, test_val_1); }); ASSERT_EQ(consume_result_1, PipelineConsumeResult::Done); } -TEST(PipelineTest, ProduceIfEmptySuccessfulIf) { +TEST(PipelineTest, ProduceIfEmptySuccessfulIfQueueIsEmpty) { const int depth = 1; fml::RefPtr pipeline = fml::MakeRefCounted(depth); Continuation continuation_1 = pipeline->ProduceIfEmpty(); - Continuation continuation_2 = pipeline->ProduceIfEmpty(); const int test_val_1 = 1; continuation_1.Complete(std::make_unique(test_val_1)); diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc index 3d87889157196..811898b5a42d2 100644 --- a/shell/common/rasterizer.cc +++ b/shell/common/rasterizer.cc @@ -128,11 +128,7 @@ void Rasterizer::Draw(fml::RefPtr> pipeline) { // front of the queue and also change the consume status to more available. if (raster_status == RasterStatus::kResubmit) { auto front_continuation = pipeline->ProduceIfEmpty(); - bool result = - front_continuation.Complete(std::move(resubmitted_layer_tree_)); - if (result) { - consume_result = PipelineConsumeResult::MoreAvailable; - } + front_continuation.Complete(std::move(resubmitted_layer_tree_)); } else if (raster_status == RasterStatus::kEnqueuePipeline) { consume_result = PipelineConsumeResult::MoreAvailable; } From 6c1e5dc258fb42764f2b06cad4458c661bd78900 Mon Sep 17 00:00:00 2001 From: Chris Yang Date: Fri, 13 Mar 2020 15:41:14 -0700 Subject: [PATCH 3/4] revert comments --- shell/common/pipeline.h | 6 ------ 1 file changed, 6 deletions(-) diff --git a/shell/common/pipeline.h b/shell/common/pipeline.h index ecc3cdbf9e127..8ec8366201db2 100644 --- a/shell/common/pipeline.h +++ b/shell/common/pipeline.h @@ -60,10 +60,6 @@ class Pipeline : public fml::RefCountedThreadSafe> { } } - // Return `true` if successful. - // - // If a `ProducerContinuation` is created with `ProduceIfEmpty`, `Complete` - // returns `false` if the queue is not empty. void Complete(ResourcePtr resource) { if (continuation_) { continuation_(std::move(resource), trace_id_); @@ -179,7 +175,6 @@ class Pipeline : public fml::RefCountedThreadSafe> { std::mutex queue_mutex_; std::deque> queue_; - // Always returns `true` as it is always successful. void ProducerCommit(ResourcePtr resource, size_t trace_id) { { std::scoped_lock lock(queue_mutex_); @@ -190,7 +185,6 @@ class Pipeline : public fml::RefCountedThreadSafe> { available_.Signal(); } - // Returns `true` if successful. void ProducerCommitIfEmpty(ResourcePtr resource, size_t trace_id) { { std::scoped_lock lock(queue_mutex_); From a60d0f9390748b8c11cea48af89f811d2e2ff2b5 Mon Sep 17 00:00:00 2001 From: Chris Yang Date: Thu, 19 Mar 2020 10:15:29 -0700 Subject: [PATCH 4/4] review updates --- shell/common/pipeline.h | 4 ++++ shell/common/pipeline_unittests.cc | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/shell/common/pipeline.h b/shell/common/pipeline.h index 8ec8366201db2..296d9bf4a87ab 100644 --- a/shell/common/pipeline.h +++ b/shell/common/pipeline.h @@ -113,6 +113,8 @@ class Pipeline : public fml::RefCountedThreadSafe> { // Create a `ProducerContinuation` that will only push the task if the queue // is empty. + // Prefer using |Produce|. ProducerContinuation returned by this method + // doesn't guarantee that the frame will be rendered. ProducerContinuation ProduceIfEmpty() { if (!empty_.TryWait()) { return {}; @@ -189,6 +191,8 @@ class Pipeline : public fml::RefCountedThreadSafe> { { std::scoped_lock lock(queue_mutex_); if (!queue_.empty()) { + // Bail if the queue is not empty, opens up spaces to produce other + // frames. empty_.Signal(); return; } diff --git a/shell/common/pipeline_unittests.cc b/shell/common/pipeline_unittests.cc index 6e500e5bf36a9..76fdc85867ee7 100644 --- a/shell/common/pipeline_unittests.cc +++ b/shell/common/pipeline_unittests.cc @@ -89,7 +89,7 @@ TEST(PipelineTest, PushingMultiProcessesInOrder) { ASSERT_EQ(consume_result_2, PipelineConsumeResult::Done); } -TEST(PipelineTest, ProduceIfEmptyWhenQueueIsNotEmptyDoesNotConsume) { +TEST(PipelineTest, ProduceIfEmptyDoesNotConsumeWhenQueueIsNotEmpty) { const int depth = 2; fml::RefPtr pipeline = fml::MakeRefCounted(depth);