gpu: Reduce GL context switches used to check pending queries.

content/common/gpu/gpu_command_buffer_stub.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/hash.h"
 #include "base/json/json_writer.h"
 #include "base/memory/shared_memory.h"
@@ skipping 181 matching lines @@
       gpu_preference_(gpu_preference),
       use_virtualized_gl_context_(use_virtualized_gl_context),
       stream_id_(stream_id),
       route_id_(route_id),
       surface_id_(surface_id),
       software_(software),
       last_flush_count_(0),
       last_memory_allocation_valid_(false),
       watchdog_(watchdog),
       sync_point_wait_count_(0),
-      delayed_work_scheduled_(false),
       previous_messages_processed_(0),
       active_url_(active_url),
       total_gpu_memory_(0) {
   active_url_hash_ = base::Hash(active_url.possibly_invalid_spec());
   FastSetActiveURL(active_url_, active_url_hash_);
 
   gpu::gles2::ContextCreationAttribHelper attrib_parser;
   attrib_parser.Parse(requested_attribs_);
 
   if (share_group) {
@@ skipping 93 matching lines @@
         OnSetClientHasMemoryAllocationChangedCallback)
     IPC_MESSAGE_HANDLER(GpuCommandBufferMsg_CreateImage, OnCreateImage);
     IPC_MESSAGE_HANDLER(GpuCommandBufferMsg_DestroyImage, OnDestroyImage);
     IPC_MESSAGE_HANDLER(GpuCommandBufferMsg_CreateStreamTexture,
                         OnCreateStreamTexture)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
 
   CheckCompleteWaits();
 
+  // Ensure that any delayed work that was created will be handled.
   if (have_context) {
-    // Ensure that any delayed work that was created will be handled.
-    ScheduleDelayedWork(kHandleMoreWorkPeriodMs);
+    if (scheduler_)
+      scheduler_->ProcessPendingQueries();
+    ScheduleDelayedWork(
+        base::TimeDelta::FromMilliseconds(kHandleMoreWorkPeriodMs));
   }
 
   DCHECK(handled);
   return handled;
 }
 
 bool GpuCommandBufferStub::Send(IPC::Message* message) {
   return channel_->Send(message);
 }
 
 bool GpuCommandBufferStub::IsScheduled() {
   return (!scheduler_.get() || scheduler_->IsScheduled());
 }
 
-bool GpuCommandBufferStub::HasMoreWork() {
-  return scheduler_.get() && scheduler_->HasMoreWork();
+void GpuCommandBufferStub::PollWork() {
+  // Post another delayed task if we have not yet reached the time at which
+  // we should process delayed work.
+  base::TimeTicks current_time = base::TimeTicks::Now();
+  DCHECK(!process_delayed_work_time_.is_null());
+  if (process_delayed_work_time_ > current_time) {
+    task_runner_->PostDelayedTask(

[dshwang, 2015/09/01 08:56:10]

do we have to delay one more?
PollWork() is already called via PostDelayedTask considering
|process_delayed_work_time_|.
In addition, I'm afraid that twice post-tasks delays to perform pending tasks
for unnecessarily long time.

[reveman, 2015/09/01 15:54:11]

That's what this patch is supposed to improve. If "process_delayed_work_time_ >
current_time" then that means we've checked for pending queries since the task
was posted so performing work here would cause us to do that more frequently
than desired. We need to delay the PerformWork more to avoid that. 

[dshwang, 2015/09/01 16:15:50]

How is it possible |process_delayed_work_time_ > current_time|?
Only GpuCommandBufferStub::ScheduleDelayedWork posts this method with |delay|,
which is set to |process_delayed_work_time_|.

[reveman, 2015/09/01 16:25:17]

ScheduleDelayedWork is called as a result of performing work. A call to that
function means that we should re-schedule the time at which we should perform
delayed. At line 409 we update |process_delayed_work_time_| to the new time at
which we should perform delayed work and if a delayed task is already in flight
then "process_delayed_work_time_ > current_time" is likely going to be true when
that task runs.

[dshwang, 2015/09/01 16:32:17]

I understand, thx for explanation.

[dshwang, 2015/09/01 16:37:00]

Is it possible for idle tasks and pending query to be starved? If
GpuCommandBufferStub get messages continuously, it looks possible.

[reveman, 2015/09/01 17:02:07]

Pending queries are processed each time we process messages so they can't be
starved. Idle work can be starved but I'm not sure if that's a problem or the
behavior we prefer. It was a bit of a problem with async uploads but with that
removed it might be better to only do idle work when actually idle.

+        FROM_HERE, base::Bind(&GpuCommandBufferStub::PollWork, AsWeakPtr()),
+        process_delayed_work_time_ - current_time);
+    return;
+  }
+  process_delayed_work_time_ = base::TimeTicks();
+
+  PerformWork();
 }
 
-void GpuCommandBufferStub::PollWork() {
-  TRACE_EVENT0("gpu", "GpuCommandBufferStub::PollWork");
-  delayed_work_scheduled_ = false;
+void GpuCommandBufferStub::PerformWork() {
+  TRACE_EVENT0("gpu", "GpuCommandBufferStub::PerformWork");
+
   FastSetActiveURL(active_url_, active_url_hash_);
   if (decoder_.get() && !MakeCurrent())
     return;
 
   if (scheduler_) {
     uint64 current_messages_processed =
         channel()->gpu_channel_manager()->MessagesProcessed();
     // We're idle when no messages were processed or scheduled.
     bool is_idle =
         (previous_messages_processed_ == current_messages_processed) &&
         !channel()->gpu_channel_manager()->HandleMessagesScheduled();
     if (!is_idle && !last_idle_time_.is_null()) {
       base::TimeDelta time_since_idle =
           base::TimeTicks::Now() - last_idle_time_;
       base::TimeDelta max_time_since_idle =
           base::TimeDelta::FromMilliseconds(kMaxTimeSinceIdleMs);
 
       // Force idle when it's been too long since last time we were idle.
       if (time_since_idle > max_time_since_idle)
         is_idle = true;
     }
 
     if (is_idle) {
       last_idle_time_ = base::TimeTicks::Now();
       scheduler_->PerformIdleWork();
     }
+
+    scheduler_->ProcessPendingQueries();

[no sievers, 2015/09/16 21:04:35]

Why do we really treat pending queries different from idle work?
Apart from the fact that we already check queries after each handled message and
after flush and finish.

I'm wondering if we can even avoid the MakeCurrent() above in line 355 if we are
not idle. It means we will handle a message very soon and check pending queries
anyhow.

[reveman, 2015/09/16 23:12:08]

I'm not that familiar with the current use cases for idle work. It was created
for async uploads but is used for other things today. Would be nice to remove
the idle work concept if possible.
I'm not sure processing of queries as idle work is going to work well. I think
that would result in even worse stalls when running out of staging buffers than
we have today.

While flush and finish will often be enough to process pending queries, it's
important that we check them frequently in situations where the compositor
depends on them to make progress when initializing tiles. e.g. when raster is
fast enough that we run out of staging buffers.

My plan for reducing processing of queries when not needed but increasing the
interval when needed is to use SignalQuery API from the compositor when we run
out of staging buffers to tell the GPU service that it's now important to
process queries. I'd prefer to keep PerformIdleWork and ProcessPendingQueries
separated for now.

[no sievers, 2015/09/17 00:34:38]

ok sounds good. i was just thinking that because we already process queries
above after each message being handled, that i was wondering how much value it
adds to do it here for |!is_idle|. it seems that would occur when we have
messages pending (!is_idle) but are not processing them because we are
descheduled. so probably important enough to consider.

   }
-  ScheduleDelayedWork(kHandleMoreWorkPeriodBusyMs);
+
+  ScheduleDelayedWork(
+      base::TimeDelta::FromMilliseconds(kHandleMoreWorkPeriodBusyMs));
 }
 
 bool GpuCommandBufferStub::HasUnprocessedCommands() {
   if (command_buffer_) {
     gpu::CommandBuffer::State state = command_buffer_->GetLastState();
     return command_buffer_->GetPutOffset() != state.get_offset &&
         !gpu::error::IsError(state.error);
   }
   return false;
 }
 
-void GpuCommandBufferStub::ScheduleDelayedWork(int64 delay) {
-  if (!HasMoreWork()) {
+void GpuCommandBufferStub::ScheduleDelayedWork(base::TimeDelta delay) {
+  bool has_more_work = scheduler_.get() && (scheduler_->HasPendingQueries() ||
+                                            scheduler_->HasMoreIdleWork());

[dshwang, 2015/09/01 08:56:10]

very nice :)

+  if (!has_more_work) {
     last_idle_time_ = base::TimeTicks();
     return;
   }
 
-  if (delayed_work_scheduled_)
+  base::TimeTicks current_time = base::TimeTicks::Now();
+  // |process_delayed_work_time_| is set if processing of delayed work is
+  // already scheduled. Just update the time if already scheduled.
+  if (!process_delayed_work_time_.is_null()) {
+    process_delayed_work_time_ = current_time + delay;

[no sievers, 2015/09/16 21:04:35]

This keeps pushing out the time though if called repeatedly, so it's a bit
counterintuitive if you just look at ScheduleDelayedWork() in isolation (the
previous implementation has an early return if already scheduled). Is this
because calling this *should* imply that you also probably just did PollWork()
or weren't idle (i.e. handled a message and also at least processed queries
then)?

I'm just ever so slightly worried that it could be misconceived and somebody
adds a call to this somewhere with the intention of 'making sure work is
scheduled'. Maybe a comment above the function is all it needs.

[reveman, 2015/09/16 23:12:08]

Improved the comment in the header file to make this more clear.

     return;
-  delayed_work_scheduled_ = true;
+  }
 
   // Idle when no messages are processed between now and when
   // PollWork is called.
   previous_messages_processed_ =
       channel()->gpu_channel_manager()->MessagesProcessed();
   if (last_idle_time_.is_null())
-    last_idle_time_ = base::TimeTicks::Now();
+    last_idle_time_ = current_time;
 
   // IsScheduled() returns true after passing all unschedule fences
   // and this is when we can start performing idle work. Idle work
   // is done synchronously so we can set delay to 0 and instead poll
   // for more work at the rate idle work is performed. This also ensures
   // that idle work is done as efficiently as possible without any
   // unnecessary delays.
   if (scheduler_.get() &&
       scheduler_->IsScheduled() &&
       scheduler_->HasMoreIdleWork()) {

[dshwang, 2015/09/01 08:56:10]

Don't we need to check (scheduler_->HasPendingQueries() ||
scheduler_->HasMoreIdleWork())?

[reveman, 2015/09/01 15:54:11]

This code is only for handling idle work. Pending queries should not affect it.

-    delay = 0;
+    delay = base::TimeDelta();
   }
 
+  process_delayed_work_time_ = current_time + delay;
   task_runner_->PostDelayedTask(
       FROM_HERE, base::Bind(&GpuCommandBufferStub::PollWork, AsWeakPtr()),
-      base::TimeDelta::FromMilliseconds(delay));
+      delay);
 }
 
 bool GpuCommandBufferStub::MakeCurrent() {
   if (decoder_->MakeCurrent())
     return true;
   DLOG(ERROR) << "Context lost because MakeCurrent failed.";
   command_buffer_->SetContextLostReason(decoder_->GetContextLostReason());
   command_buffer_->SetParseError(gpu::error::kLostContext);
   CheckContextLost();
   return false;
@@ skipping 759 matching lines @@
                                                     result));
 }
 
 void GpuCommandBufferStub::SendUpdateVSyncParameters(base::TimeTicks timebase,
                                                      base::TimeDelta interval) {
   Send(new GpuCommandBufferMsg_UpdateVSyncParameters(route_id_, timebase,
                                                      interval));
 }
 
 }  // namespace content