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

content/common/gpu/gpu_command_buffer_stub.cc

Index: content/common/gpu/gpu_command_buffer_stub.cc
diff --git a/content/common/gpu/gpu_command_buffer_stub.cc b/content/common/gpu/gpu_command_buffer_stub.cc
index 407ac03b1fbfb92011eb4bb31de74c4a0a855b09..98fdc1b55ba1e46337711adad61e9490d38eb491 100644
--- a/content/common/gpu/gpu_command_buffer_stub.cc
+++ b/content/common/gpu/gpu_command_buffer_stub.cc
@@ -199,7 +199,6 @@ GpuCommandBufferStub::GpuCommandBufferStub(
       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) {
@@ -313,9 +312,12 @@ bool GpuCommandBufferStub::OnMessageReceived(const IPC::Message& message) {
 
   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);
@@ -330,13 +332,25 @@ 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;
@@ -363,8 +377,12 @@ void GpuCommandBufferStub::PollWork() {
       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() {
@@ -376,22 +394,28 @@ bool GpuCommandBufferStub::HasUnprocessedCommands() {
   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
@@ -402,12 +426,13 @@ void GpuCommandBufferStub::ScheduleDelayedWork(int64 delay) {
   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() {