Replace Pipeline's ProduceToFront with ProduceIfEmpty to handle thread merging.#17122
Conversation
flar
left a comment
flar
left a comment
There was a problem hiding this comment.
Also, would like to see input from @chinmaygarde
shell/common/rasterizer.cc
Outdated
| front_continuation.Complete(std::move(resubmitted_layer_tree_)); | ||
| if (result) { | ||
| consume_result = PipelineConsumeResult::MoreAvailable; | ||
| } |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Oh, you are right! Reverting.
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.
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. |
Sure! I have updated the description of the PR. Please let me know if there are still things missing :) |
iskakaushik
left a comment
iskakaushik
left a comment
There was a problem hiding this comment.
LGTM modulo requests for doc improvements and a nit about test name.
| // used to en-queue high-priority resources. | ||
| ProducerContinuation ProduceToFront() { | ||
| // Create a `ProducerContinuation` that will only push the task if the queue | ||
| // is empty. |
There was a problem hiding this comment.
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.
shell/common/pipeline_unittests.cc
Outdated
| } | ||
|
|
||
| TEST(PipelineTest, PushingToFrontOverridesOrder) { | ||
| TEST(PipelineTest, ProduceIfEmptyWhenQueueIsNotEmptyDoesNotConsume) { |
There was a problem hiding this comment.
rename test to ProduceIfEmptyDoesNotConsumeWhenQueueIsNotEmpty
| while (queue_.size() > depth_) { | ||
| queue_.pop_back(); | ||
| if (!queue_.empty()) { | ||
| empty_.Signal(); |
There was a problem hiding this comment.
can you add a note here describing why this semaphore needs to be signaled?
cyanglaz
left a comment
cyanglaz
left a comment
There was a problem hiding this comment.
@iskakaushik Updated based on your comments!
| // used to en-queue high-priority resources. | ||
| ProducerContinuation ProduceToFront() { | ||
| // Create a `ProducerContinuation` that will only push the task if the queue | ||
| // is empty. |
| while (queue_.size() > depth_) { | ||
| queue_.pop_back(); | ||
| if (!queue_.empty()) { | ||
| empty_.Signal(); |
shell/common/pipeline_unittests.cc
Outdated
| } | ||
|
|
||
| TEST(PipelineTest, PushingToFrontOverridesOrder) { | ||
| TEST(PipelineTest, ProduceIfEmptyWhenQueueIsNotEmptyDoesNotConsume) { |
…handle thread merging. (flutter/engine#17122)
…handle thread merging. (flutter/engine#17122) (#52972)
6 participants
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