Mozart: Reduce pipeline depth and unify frame queue.

services/gfx/compositor/backend/gpu_output.cc

Index: services/gfx/compositor/backend/gpu_output.cc
diff --git a/services/gfx/compositor/backend/gpu_output.cc b/services/gfx/compositor/backend/gpu_output.cc
index d52ef5a250e7c0f35da9e94f40725465be62dfd9..ad42dc58fd0f3d4b68e76ad01d43219bbd9eee1d 100644
--- a/services/gfx/compositor/backend/gpu_output.cc
+++ b/services/gfx/compositor/backend/gpu_output.cc
@@ -18,9 +18,9 @@
 namespace compositor {
 namespace {
 constexpr const char* kPipelineDepthSwitch = "pipeline-depth";
-constexpr uint32_t kDefaultPipelineDepth = 2u;  // ideally should be 1
+constexpr uint32_t kDefaultPipelineDepth = 1u;
 constexpr uint32_t kMinPipelineDepth = 1u;
-constexpr uint32_t kMaxPipelineDepth = 10u;
+constexpr uint32_t kMaxPipelineDepth = 10u;  // for experimentation
 
 scoped_ptr<base::MessagePump> CreateMessagePumpMojo() {
   return base::MessageLoop::CreateMessagePumpForType(
@@ -86,24 +86,34 @@ void GpuOutput::SubmitFrame(const scoped_refptr<RenderFrame>& frame) {
   TRACE_EVENT0("gfx", "GpuOutput::SubmitFrame");
 
   const int64_t submit_time = MojoGetTimeTicksNow();
-  scoped_refptr<FrameData> frame_data(
+  std::unique_ptr<FrameData> frame_data(
       new FrameData(frame, submit_time));  // drop outside lock
   {
     std::lock_guard<std::mutex> lock(shared_state_.mutex);
 
-    TRACE_EVENT_FLOW_BEGIN0("gfx", "Frame Queued", frame_data.get());
-    shared_state_.current_frame_data.swap(frame_data);
-    if (frame_data && !frame_data->drawn) {
-      // Dropped an undrawn frame.
-      DVLOG(2) << "Rasterizer stalled, dropping frame to catch up.";
+    // Enqueue the frame, ensuring that the queue only contains at most
+    // one undrawn frame.  If the last frame hasn't been drawn by now then
+    // the rasterizer must be falling behind.
+    if (!shared_state_.frames.empty() && shared_state_.frames.front()->finished)

[mikejurka, 2016/05/25 21:11:46]

if you introduce a pinned_for_draw variable, i would do a DCHECK here to make
sure its false.

[jeffbrown, 2016/05/25 23:45:43]

Maybe add a DCHECK here that the queue size is exactly 1.

+      shared_state_.frames.pop();

[mikejurka, 2016/05/25 21:11:46]

should we have a DCHECK after this pop that the front() frame is not finished?

ie 

// Make sure all finished frames have been removed
DCHECK(shared_state_.frames.empty() || !shared_state_.frames.front()->finished)

[mikejurka, 2016/05/25 23:54:06]

actually, on second thought, shouldn't the above "if" clause be completely
unnecessary? doesn't OnRasterizerFinishedDraw set "finished" to true and then
remove from the queue in one swoop?

+    if (shared_state_.frames.empty() || shared_state_.frames.back()->drawn) {
+      shared_state_.frames.emplace(std::move(frame_data));
+    } else {

[jeffbrown, 2016/05/25 23:45:43]

Add a comment here explaining why it's safe to replace the last frame.

+      shared_state_.frames.back().swap(frame_data);

[mikejurka, 2016/05/25 21:11:46]

if you introduce a pinned_for_draw variable, i would do a DCHECK here to make
sure its false.


ie

 DCHECK(!shared_state_.frames.back().pinned_for_draw)

       TRACE_EVENT_FLOW_END1("gfx", "Frame Queued", frame_data.get(), "drawn",
                             false);
+      DVLOG(2) << "Rasterizer stalled, dropped a frame to catch up.";
     }
 
-    // TODO(jeffbrown): If the draw queue is full, we should pause
+    TRACE_EVENT_FLOW_BEGIN0("gfx", "Frame Queued",
+                            shared_state_.frames.back().get());
+
+    if (!shared_state_.rasterizer_ready)
+      return;
+
+    // TODO(jeffbrown): If the draw queue is overfull, we should pause
     // scheduling until the queue drains.
-    if (shared_state_.rasterizer_ready &&
-        shared_state_.drawn_frames_awaiting_finish.size() < pipeline_depth_)
+    if (shared_state_.frames.size() <= pipeline_depth_)
       ScheduleDrawLocked();

[mikejurka, 2016/05/25 21:11:46]

might be useful to track another time here ie draw_scheduled_time

[jeffbrown, 2016/05/25 23:45:43]

I'm going to leave this out.  The other two timestamps are useful for estimating
overall CPU and GPU latency on their own whereas specifically observing
scheduling latency is something we could accomplish through tracing.

   }
 }
@@ -134,15 +144,14 @@ void GpuOutput::OnRasterizerReady(int64_t vsync_timebase,
     if (shared_state_.rasterizer_ready)
       return;
 
-    DCHECK(shared_state_.drawn_frames_awaiting_finish.empty());
     shared_state_.rasterizer_ready = true;
 
-    if (!shared_state_.current_frame_data)
+    if (shared_state_.frames.empty())
       return;
 
-    shared_state_.current_frame_data->Recycle();
+    shared_state_.frames.back()->ClearDrawState();
     TRACE_EVENT_FLOW_BEGIN0("gfx", "Frame Queued",
-                            shared_state_.current_frame_data.get());
+                            shared_state_.frames.back().get());
     ScheduleDrawLocked();
   }
 }
@@ -169,8 +178,9 @@ void GpuOutput::OnRasterizerError() {
 }
 
 void GpuOutput::ScheduleDrawLocked() {
-  DCHECK(shared_state_.current_frame_data);
-  DCHECK(!shared_state_.current_frame_data->drawn);
+  DCHECK(!shared_state_.frames.empty());
+  DCHECK(!shared_state_.frames.back()->drawn);
+  DCHECK(shared_state_.frames.size() <= pipeline_depth_);
 
   if (shared_state_.draw_scheduled)
     return;
@@ -184,30 +194,29 @@ void GpuOutput::OnDraw() {
   DCHECK(rasterizer_task_runner_->RunsTasksOnCurrentThread());
   TRACE_EVENT0("gfx", "GpuOutput::OnDraw");
 
-  scoped_refptr<FrameData> frame_data;  // used outside lock
+  FrameData* frame_data;

[mikejurka, 2016/05/25 21:11:46]

probably a good idea to initialize to null (just for a safeguard for future code
changes)

[jeffbrown, 2016/05/25 23:45:43]

The compiler will yell at me if I try to use an uninitialized variable so I find
it better leaving it that way rather than making the reader wonder whether a
null can ever happen.

   {
     std::lock_guard<std::mutex> lock(shared_state_.mutex);
 
     DCHECK(shared_state_.draw_scheduled);
-    DCHECK(shared_state_.current_frame_data);
-    DCHECK(!shared_state_.current_frame_data->drawn);
+    DCHECK(!shared_state_.frames.empty());
+    DCHECK(!shared_state_.frames.back()->drawn);
 
     shared_state_.draw_scheduled = false;
-
-    if (!shared_state_.rasterizer_ready ||
-        shared_state_.drawn_frames_awaiting_finish.size() >= pipeline_depth_)
+    if (!shared_state_.rasterizer_ready)
       return;
 
-    frame_data = shared_state_.current_frame_data;
+    frame_data = shared_state_.frames.back().get();

[mikejurka, 2016/05/25 21:11:46]

maybe move DCHECK(!frame_data->drawn) to here instead.

[jeffbrown, 2016/05/25 23:45:43]

I'd rather leave it where it is since the invariant must hold true independent
of whether the rasterizer is ready.

     frame_data->drawn = true;
     frame_data->draw_time = MojoGetTimeTicksNow();
-    TRACE_EVENT_FLOW_END1("gfx", "Frame Queued", frame_data.get(), "drawn",
-                          true);
-
-    TRACE_EVENT_ASYNC_BEGIN0("gfx", "Rasterize", frame_data.get());
-    shared_state_.drawn_frames_awaiting_finish.emplace(frame_data);
+    TRACE_EVENT_FLOW_END1("gfx", "Frame Queued", frame_data, "drawn", true);
   }
 

[mikejurka, 2016/05/25 21:11:46]

Do a DCHECK to ensure frame_data is not null here (just for a safeguard for
future code changes)

[jeffbrown, 2016/05/25 23:45:43]

I don't think that's really necessary.  It must obviously be non-null since we
dereferenced it.

+  // It is safe to access |frame_data| outside of the lock here because
+  // it will not be dequeued until |OnRasterizerFinishedDraw| gets posted
+  // to this thread's message loop.  Moreover |SubmitFrame| will not replace
+  // the frame because the |drawn| flag has been set.

[mikejurka, 2016/05/25 21:11:46]

I would put a note in the header file next to the drawn flag that you can never
remove or swap an item where (drawn && !isFinished) is true.

actually, i would consider making another variable called "pinned_for_draw" that
is true during this critical section (ie after "drawn" is set but before
"isFinished" is set"), and check for that in the other methods. and sprinkle
DCHECKs to make sure you are never popping or swapping an item with
"pinned_for_draw" set to true.

[jeffbrown, 2016/05/25 23:45:43]

I don't actually need extra state here.  I could add an inline function but I
think the indirection may make the code even harder to understand.  Note that
this invariant follows from the comments already in the header file.

+  TRACE_EVENT_ASYNC_BEGIN0("gfx", "Rasterize", frame_data);
   rasterizer_->DrawFrame(frame_data->frame);
   frame_data->wait_time = MojoGetTimeTicksNow();
 }
@@ -217,19 +226,19 @@ void GpuOutput::OnRasterizerFinishedDraw(bool presented) {
   TRACE_EVENT0("gfx", "GpuOutput::OnRasterizerFinishedDraw");
 
   const int64_t finish_time = MojoGetTimeTicksNow();
-  scoped_refptr<FrameData> frame_data;  // drop outside lock
+  std::unique_ptr<FrameData> old_frame_data;  // drop outside lock

[mikejurka, 2016/05/25 21:11:46]

change comment for "drop outside lock" to "keep old frame data alive until after
locked section"

[jeffbrown, 2016/05/25 23:45:43]

Done.

   {
     std::lock_guard<std::mutex> lock(shared_state_.mutex);
 
     DCHECK(shared_state_.rasterizer_ready);
-    DCHECK(!shared_state_.drawn_frames_awaiting_finish.empty());
-    size_t draw_queue_depth = shared_state_.drawn_frames_awaiting_finish.size();
-    shared_state_.drawn_frames_awaiting_finish.front().swap(frame_data);
-    shared_state_.drawn_frames_awaiting_finish.pop();
+    DCHECK(!shared_state_.frames.empty());
+
+    FrameData* frame_data = shared_state_.frames.front().get();
     DCHECK(frame_data);
     DCHECK(frame_data->drawn);
-    TRACE_EVENT_ASYNC_END1("gfx", "Rasterize", frame_data.get(), "presented",
+    TRACE_EVENT_ASYNC_END1("gfx", "Rasterize", frame_data, "presented",
                            presented);
+    frame_data->finished = true;

[mikejurka, 2016/05/25 21:11:46]

if you decide to use a "pinned_for_draw" var, i guess you'd set it to false here
as well.

 
     // TODO(jeffbrown): Adjust scheduler behavior based on observed timing.
     // Note: These measurements don't account for systematic downstream delay
@@ -245,6 +254,7 @@ void GpuOutput::OnRasterizerFinishedDraw(bool presented) {
       const int64_t draw_time = frame_data->draw_time;
       const int64_t wait_time = frame_data->wait_time;
       const int64_t submit_time = frame_data->submit_time;
+      const size_t draw_queue_depth = shared_state_.frames.size();
 
       DVLOG(2) << "Presented frame: composition latency "
                << (composition_time - frame_time) << " us, submission latency "
@@ -259,9 +269,12 @@ void GpuOutput::OnRasterizerFinishedDraw(bool presented) {
       DVLOG(2) << "Rasterizer dropped frame.";
     }
 
-    DCHECK(shared_state_.current_frame_data);
-    if (!shared_state_.current_frame_data->drawn)
-      ScheduleDrawLocked();
+    if (shared_state_.frames.size() > 1u) {
+      shared_state_.frames.front().swap(old_frame_data);

[mikejurka, 2016/05/25 21:11:46]

I would add the earlier comment here again to clarify ie

"// keep old frame data alive until after locked section"

[jeffbrown, 2016/05/25 23:45:43]

Done.

+      shared_state_.frames.pop();
+      if (!shared_state_.frames.back()->drawn)
+        ScheduleDrawLocked();
+    }
   }
 }
 
@@ -295,8 +308,9 @@ GpuOutput::FrameData::FrameData(const scoped_refptr<RenderFrame>& frame,
 
 GpuOutput::FrameData::~FrameData() {}
 
-void GpuOutput::FrameData::Recycle() {
+void GpuOutput::FrameData::ClearDrawState() {
   drawn = false;
+  finished = false;
   draw_time = 0;
   wait_time = 0;
 }