GPU Channel's now maintain a global order number for each processed IPC.

content/common/gpu/gpu_channel.cc

Index: content/common/gpu/gpu_channel.cc
diff --git a/content/common/gpu/gpu_channel.cc b/content/common/gpu/gpu_channel.cc
index 636029cdd4765ede69dd2083c684ae54624867bd..a5eafa6067bf09aa787b457f835714c8b6ed7a4c 100644
--- a/content/common/gpu/gpu_channel.cc
+++ b/content/common/gpu/gpu_channel.cc
@@ -19,7 +19,6 @@
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "base/thread_task_runner_handle.h"
-#include "base/timer/timer.h"
 #include "base/trace_event/memory_dump_manager.h"
 #include "base/trace_event/process_memory_dump.h"
 #include "base/trace_event/trace_event.h"
@@ -81,16 +80,17 @@ const int64 kStopPreemptThresholdMs = kVsyncIntervalMs;
 class GpuChannelMessageFilter : public IPC::MessageFilter {
  public:
   GpuChannelMessageFilter(
+      GpuChannel* gpu_channel_io,
       base::WeakPtr<GpuChannel> gpu_channel,
       gpu::SyncPointManager* sync_point_manager,
       scoped_refptr<base::SingleThreadTaskRunner> task_runner,
       bool future_sync_points)
       : preemption_state_(IDLE),
+        gpu_channel_io_(gpu_channel_io),
         gpu_channel_(gpu_channel),
         sender_(nullptr),
         sync_point_manager_(sync_point_manager),
         task_runner_(task_runner),
-        messages_forwarded_to_channel_(0),
         a_stub_is_descheduled_(false),
         future_sync_points_(future_sync_points) {}
 
@@ -109,6 +109,8 @@ class GpuChannelMessageFilter : public IPC::MessageFilter {
   bool OnMessageReceived(const IPC::Message& message) override {
     DCHECK(sender_);
 
+    const uint32_t order_number =
+        gpu_channel_io_->gpu_channel_manager()->GenerateGlobalOrderNumber();

[piman, 2015/08/31 23:15:04]

I don't think gpu_channel_io_ is safe to use, because the filter may outlive the
GpuChannel.

My suggestion is to move the message queue (and the lock) to the filter, and use
the existing weak pointer to post the task when needed.

For the GenerateGlobalOrderNumber, I think it's fine to use a
StaticAtomicSequenceNumber, global in this file. There's other options if we
need it to be owned by the GpuChannelManager (e.g. a thread safe refcounted
class).

[David Yen, 2015/09/01 02:01:52]

Having the filter own the messages didn't feel right since most of the
operations are done on the GpuChannel side. I made it a separate object instead
which is shared by both, having the GpuChannel specific functions be private
while GpuChannel is a friend.

The global order number doesn't really need to be owned by the GpuChannelManager
and it's only accessed on the IO thread so I think a regular static number in
GpuChannelMessageFilter is perfect.

     bool handled = false;
     if ((message.type() == GpuCommandBufferMsg_RetireSyncPoint::ID) &&
         !future_sync_points_) {
@@ -135,11 +137,9 @@ class GpuChannelMessageFilter : public IPC::MessageFilter {
       uint32 sync_point = sync_point_manager_->GenerateSyncPoint();
       GpuCommandBufferMsg_InsertSyncPoint::WriteReplyParams(reply, sync_point);
       Send(reply);
-      task_runner_->PostTask(
-          FROM_HERE,
-          base::Bind(&GpuChannelMessageFilter::InsertSyncPointOnMainThread,
-                     gpu_channel_, sync_point_manager_, message.routing_id(),
-                     base::get<0>(retire), sync_point));
+
+      gpu_channel_io_->PushSyncPointMessage(order_number, message,
+                                            base::get<0>(retire), sync_point);
       handled = true;
     }
 
@@ -154,19 +154,24 @@ class GpuChannelMessageFilter : public IPC::MessageFilter {
       return false;
     }
 
-    // All other messages get processed by the GpuChannel.
-    messages_forwarded_to_channel_++;
-    if (preempting_flag_.get())
-      pending_messages_.push(PendingMessage(messages_forwarded_to_channel_));
-    UpdatePreemptionState();
+    // Forward all other messages to the GPU Channel.
+    if (!handled && !message.is_reply() && !message.should_unblock()) {
+      if (message.type() == GpuCommandBufferMsg_WaitForTokenInRange::ID ||
+          message.type() == GpuCommandBufferMsg_WaitForGetOffsetInRange::ID) {
+        // Move Wait commands to the head of the queue, so the renderer
+        // doesn't have to wait any longer than necessary.
+        gpu_channel_io_->PushFrontMessage(message);
+      } else {
+        gpu_channel_io_->PushBackMessage(order_number, message);
+      }
+      handled = true;
+    }
 
+    UpdatePreemptionState();
     return handled;
   }
 
-  void MessageProcessed(uint64 messages_processed) {
-    while (!pending_messages_.empty() &&
-           pending_messages_.front().message_number <= messages_processed)
-      pending_messages_.pop();
+  void OnMessageProcessed() {
     UpdatePreemptionState();
   }
 
@@ -212,20 +217,10 @@ class GpuChannelMessageFilter : public IPC::MessageFilter {
   // It is reset when we transition to IDLE.
   base::TimeDelta max_preemption_time_;
 
-  struct PendingMessage {
-    uint64 message_number;
-    base::TimeTicks time_received;
-
-    explicit PendingMessage(uint64 message_number)
-        : message_number(message_number),
-          time_received(base::TimeTicks::Now()) {
-    }
-  };
-
   void UpdatePreemptionState() {
     switch (preemption_state_) {
       case IDLE:
-        if (preempting_flag_.get() && !pending_messages_.empty())
+        if (preempting_flag_.get() && gpu_channel_io_->HasQueuedMessages())
           TransitionToWaiting();
         break;
       case WAITING:
@@ -233,21 +228,23 @@ class GpuChannelMessageFilter : public IPC::MessageFilter {
         DCHECK(timer_->IsRunning());
         break;
       case CHECKING:
-        if (!pending_messages_.empty()) {
-          base::TimeDelta time_elapsed =
-              base::TimeTicks::Now() - pending_messages_.front().time_received;
-          if (time_elapsed.InMilliseconds() < kPreemptWaitTimeMs) {
-            // Schedule another check for when the IPC may go long.
-            timer_->Start(
-                FROM_HERE,
-                base::TimeDelta::FromMilliseconds(kPreemptWaitTimeMs) -
-                    time_elapsed,
-                this, &GpuChannelMessageFilter::UpdatePreemptionState);
-          } else {
-            if (a_stub_is_descheduled_)
-              TransitionToWouldPreemptDescheduled();
-            else
-              TransitionToPreempting();
+        {
+          base::TimeTicks time_tick = gpu_channel_io_->GetNextMessageTimeTick();
+          if (!time_tick.is_null()) {
+            base::TimeDelta time_elapsed = base::TimeTicks::Now() - time_tick;
+            if (time_elapsed.InMilliseconds() < kPreemptWaitTimeMs) {
+              // Schedule another check for when the IPC may go long.
+              timer_->Start(
+                  FROM_HERE,
+                  base::TimeDelta::FromMilliseconds(kPreemptWaitTimeMs) -
+                      time_elapsed,
+                  this, &GpuChannelMessageFilter::UpdatePreemptionState);
+            } else {
+              if (a_stub_is_descheduled_)
+                TransitionToWouldPreemptDescheduled();
+              else
+                TransitionToPreempting();
+            }
           }
         }
         break;
@@ -275,11 +272,11 @@ class GpuChannelMessageFilter : public IPC::MessageFilter {
   void TransitionToIdleIfCaughtUp() {
     DCHECK(preemption_state_ == PREEMPTING ||
            preemption_state_ == WOULD_PREEMPT_DESCHEDULED);
-    if (pending_messages_.empty()) {
+    base::TimeTicks next_tick = gpu_channel_io_->GetNextMessageTimeTick();
+    if (next_tick.is_null()) {
       TransitionToIdle();
     } else {
-      base::TimeDelta time_elapsed =
-          base::TimeTicks::Now() - pending_messages_.front().time_received;
+      base::TimeDelta time_elapsed = base::TimeTicks::Now() - next_tick;
       if (time_elapsed.InMilliseconds() < kStopPreemptThresholdMs)
         TransitionToIdle();
     }
@@ -365,46 +362,16 @@ class GpuChannelMessageFilter : public IPC::MessageFilter {
     UpdatePreemptionState();
   }
 
-  static void InsertSyncPointOnMainThread(
-      base::WeakPtr<GpuChannel> gpu_channel,
-      gpu::SyncPointManager* manager,
-      int32 routing_id,
-      bool retire,
-      uint32 sync_point) {
-    // This function must ensure that the sync point will be retired. Normally
-    // we'll find the stub based on the routing ID, and associate the sync point
-    // with it, but if that fails for any reason (channel or stub already
-    // deleted, invalid routing id), we need to retire the sync point
-    // immediately.
-    if (gpu_channel) {
-      GpuCommandBufferStub* stub = gpu_channel->LookupCommandBuffer(routing_id);
-      if (stub) {
-        stub->AddSyncPoint(sync_point);
-        if (retire) {
-          GpuCommandBufferMsg_RetireSyncPoint message(routing_id, sync_point);
-          gpu_channel->OnMessageReceived(message);
-        }
-        return;
-      } else {
-        gpu_channel->MessageProcessed();
-      }
-    }
-    manager->RetireSyncPoint(sync_point);
-  }
-
-  // NOTE: this weak pointer is never dereferenced on the IO thread, it's only
-  // passed through - therefore the WeakPtr assumptions are respected.
+  // NOTE: The gpu_channel_io_ pointer should only be used for thread-safe
+  // function calls. The gpu_channel_ weak pointer is only dereferenced on the
+  // main thread - therefore the WeakPtr assumptions are respected.
+  GpuChannel* gpu_channel_io_;
   base::WeakPtr<GpuChannel> gpu_channel_;
   IPC::Sender* sender_;
   gpu::SyncPointManager* sync_point_manager_;
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
   scoped_refptr<gpu::PreemptionFlag> preempting_flag_;
 
-  std::queue<PendingMessage> pending_messages_;
-
-  // Count of the number of IPCs forwarded to the GpuChannel.
-  uint64 messages_forwarded_to_channel_;
-
   // This timer is created and destroyed on the IO thread.
   scoped_ptr<base::OneShotTimer<GpuChannelMessageFilter>> timer_;
 
@@ -426,7 +393,7 @@ GpuChannel::GpuChannel(GpuChannelManager* gpu_channel_manager,
                        bool allow_future_sync_points)
     : gpu_channel_manager_(gpu_channel_manager),
       channel_id_(IPC::Channel::GenerateVerifiedChannelID("gpu")),
-      messages_processed_(0),
+      handle_messages_scheduled_(false),
       client_id_(client_id),
       client_tracing_id_(client_tracing_id),
       task_runner_(task_runner),
@@ -439,8 +406,9 @@ GpuChannel::GpuChannel(GpuChannelManager* gpu_channel_manager,
       pending_valuebuffer_state_(new gpu::ValueStateMap),
       watchdog_(watchdog),
       software_(software),
-      handle_messages_scheduled_(false),
-      currently_processing_message_(nullptr),
+      current_order_num_(0),
+      processed_order_num_(0),
+      unprocessed_order_num_(0),
       num_stubs_descheduled_(0),
       allow_future_sync_points_(allow_future_sync_points),
       weak_factory_(this) {
@@ -448,7 +416,8 @@ GpuChannel::GpuChannel(GpuChannelManager* gpu_channel_manager,
   DCHECK(client_id);
 
   filter_ = new GpuChannelMessageFilter(
-      weak_factory_.GetWeakPtr(), gpu_channel_manager_->sync_point_manager(),
+      this, weak_factory_.GetWeakPtr(),
+      gpu_channel_manager_->sync_point_manager(),
       task_runner_, allow_future_sync_points_);
 
   subscription_ref_set_->AddObserver(this);
@@ -458,7 +427,10 @@ GpuChannel::~GpuChannel() {
   // Clear stubs first because of dependencies.
   stubs_.clear();
 
-  STLDeleteElements(&deferred_messages_);
+  {
+    base::AutoLock auto_lock(channel_messages_lock_);
+    STLDeleteElements(&channel_messages_);

[piman, 2015/08/31 23:15:04]

We need to handle unprocessed InsertSyncPoint here, otherwise they'll never get
retired.

[David Yen, 2015/09/01 02:01:52]

Done. Although isn't this the same as how it was done before? sync points were
always retired within a task on the main thread. I admit I am not familiar with
what guarantees we have about queued tasks that are run by the time the
destructor is called.

[piman, 2015/09/01 03:55:26]

Before, InsertSyncPointOnMainThread would always run (static method),
out-of-order, and retire the sync point immediately if the stub doesn't exist.
If it did, the sync point would be added to the stub, and
GpuCommandBufferStub::Destroy would retire the sync point if it hasn't been
retired.

Now, it is handled in-order (that's ok), but that supposes HandleMessage gets to
it. Nothing guarantees that if the channel is destroyed early. In particular,
the fact that we use a weak pointer means the task is not run if the GpuChannel
is destroyed.

(note that GpuCommandBufferStub::AddSyncPoint has almost no purpose now, since
you retire immediately after it, except for "future" sync points. If we remove
those, then we don't need that logic in GpuCommandBufferStub).

[David Yen, 2015/09/01 19:08:02]

Oh I see, I didn't notice that it was static before. That makes sense.

+  }
   subscription_ref_set_->RemoveObserver(this);
   if (preempting_flag_.get())
     preempting_flag_->Reset();
@@ -492,22 +464,67 @@ base::ProcessId GpuChannel::GetClientPID() const {
   return channel_->GetPeerPID();
 }
 
-bool GpuChannel::OnMessageReceived(const IPC::Message& message) {
-  DVLOG(1) << "received message @" << &message << " on channel @" << this
-           << " with type " << message.type();
-
-  if (message.type() == GpuCommandBufferMsg_WaitForTokenInRange::ID ||
-      message.type() == GpuCommandBufferMsg_WaitForGetOffsetInRange::ID) {
-    // Move Wait commands to the head of the queue, so the renderer
-    // doesn't have to wait any longer than necessary.
-    deferred_messages_.push_front(new IPC::Message(message));
-  } else {
-    deferred_messages_.push_back(new IPC::Message(message));
+bool GpuChannel::HasQueuedMessages() {
+  base::AutoLock auto_lock(channel_messages_lock_);
+  return !channel_messages_.empty();
+}
+
+base::TimeTicks GpuChannel::GetNextMessageTimeTick() {
+  // We have to account for messages that are pushed out of order, the out
+  // of order messages are pushed back to front and have order numbers of -1.
+  base::TimeTicks next_time_tick;
+  base::AutoLock auto_lock(channel_messages_lock_);
+  for (const auto& msg : channel_messages_) {
+    if (msg->order_number != static_cast<uint32_t>(-1)) {
+      // Return the earliest time tick if we have some out of order ones.
+      return next_time_tick.is_null() ?
+             msg->time_received :
+             std::min(msg->time_received, next_time_tick);
+    } else {
+      // Store the last out of order message in next_time_tick.
+      next_time_tick = msg->time_received;
+    }
   }
+  return next_time_tick;
+}
+
+void GpuChannel::PushBackMessage(uint32_t order_number,
+                                 const IPC::Message& message) {
+  base::subtle::Release_Store(&unprocessed_order_num_, order_number);
+
+  base::AutoLock auto_lock(channel_messages_lock_);
+  channel_messages_.push_back(new ChannelMessage(order_number, message));
+  ScheduleHandleMessageLocked();
+}
+
+void GpuChannel::PushFrontMessage(const IPC::Message& message) {
+  // These are pushed out of order so should not have any order messages.
+  base::AutoLock auto_lock(channel_messages_lock_);
+  channel_messages_.push_front(new ChannelMessage(static_cast<uint32_t>(-1),
+                                                  message));
+  ScheduleHandleMessageLocked();
+}
+
+void GpuChannel::PushSyncPointMessage(uint32_t order_number,
+                                      const IPC::Message& message,
+                                      bool retire_sync_point,
+                                      uint32_t sync_point_num) {
+  DCHECK(message.type() == GpuCommandBufferMsg_InsertSyncPoint::ID);
 
-  OnScheduled();
+  base::subtle::Release_Store(&unprocessed_order_num_, order_number);
+  ChannelMessage* msg = new ChannelMessage(order_number, message);
+  msg->retire_sync_point = retire_sync_point;
+  msg->sync_point_number = sync_point_num;
 
-  return true;
+  base::AutoLock auto_lock(channel_messages_lock_);
+  channel_messages_.push_back(msg);
+  ScheduleHandleMessageLocked();
+}
+
+bool GpuChannel::OnMessageReceived(const IPC::Message& message) {
+  // All messages should be pushed to channel_messages_ and handled separately.
+  NOTREACHED();
+  return false;
 }
 
 void GpuChannel::OnChannelError() {
@@ -540,32 +557,11 @@ void GpuChannel::OnRemoveSubscription(unsigned int target) {
       new GpuHostMsg_RemoveSubscription(client_id_, target));
 }
 
-void GpuChannel::RequeueMessage() {
-  DCHECK(currently_processing_message_);
-  deferred_messages_.push_front(
-      new IPC::Message(*currently_processing_message_));
-  messages_processed_--;
-  currently_processing_message_ = NULL;
-}
-
-void GpuChannel::OnScheduled() {
-  if (handle_messages_scheduled_)
-    return;
-  // Post a task to handle any deferred messages. The deferred message queue is
-  // not emptied here, which ensures that OnMessageReceived will continue to
-  // defer newly received messages until the ones in the queue have all been
-  // handled by HandleMessage. HandleMessage is invoked as a
-  // task to prevent reentrancy.
-  task_runner_->PostTask(FROM_HERE, base::Bind(&GpuChannel::HandleMessage,
-                                               weak_factory_.GetWeakPtr()));
-  handle_messages_scheduled_ = true;
-}
-
 void GpuChannel::StubSchedulingChanged(bool scheduled) {
   bool a_stub_was_descheduled = num_stubs_descheduled_ > 0;
   if (scheduled) {
     num_stubs_descheduled_--;
-    OnScheduled();
+    ScheduleHandleMessage();
   } else {
     num_stubs_descheduled_++;
   }
@@ -691,35 +687,76 @@ bool GpuChannel::OnControlMessageReceived(const IPC::Message& msg) {
 }
 
 void GpuChannel::HandleMessage() {
-  handle_messages_scheduled_ = false;
-  if (deferred_messages_.empty())
-    return;
+  ChannelMessage* m = nullptr;
+  GpuCommandBufferStub* stub = nullptr;
+  {
+    base::AutoLock auto_lock(channel_messages_lock_);
+    handle_messages_scheduled_ = false;
+    if (channel_messages_.empty()) {
+      return;
+    }
+    m = channel_messages_.front();

[piman, 2015/08/31 23:15:04]

Can we pop_front and release the lock?
RetireSyncPoint could cause another stub to become scheduled and call
StubSchedulingChanged, which could call ScheduleHandleMessage that takes the
lock again. It would be better to make sure RetireSyncPoint is only called with
the lock not held.

We may need to take again the lock and pushing back the message at the front if
needed, or alternatively figure out whether the stub is scheduled/preempted or
not first, pop the message, then release the lock, and only then handle the
Retire.

[David Yen, 2015/09/01 02:01:52]

I would have to think about it a bit if we can pop in the beginning and push at
the front if we are being pre-empted. It is probably okay but I currently have
an invariant that only "out of order messages" are pushed to the front to make
reasoning about the state of the queue easier (which is why pushing are all
handled by functions).

The simplest solution is simply move the retire sync point handling to after we
have released the lock though. I am not too concerned about the old sync point
code because the new wait/release based on global order will be in shortly after
this. I have moved it now.

[piman, 2015/09/01 03:55:26]

That's fine.

A possibility for out-of-order messages is to keep them in a separate queue.
That way, special handling may be easier (in particular, GetNextMessageTimeTick
could simply look at the front of both queues).

[David Yen, 2015/09/01 19:08:02]

Done.

+    stub = stubs_.get(m->message.routing_id());
 
-  IPC::Message* m = NULL;
-  GpuCommandBufferStub* stub = NULL;
+    // TODO(dyen): Temporary handling of old sync points.
+    // This must ensure that the sync point will be retired. Normally we'll
+    // find the stub based on the routing ID, and associate the sync point
+    // with it, but if that fails for any reason (channel or stub already
+    // deleted, invalid routing id), we need to retire the sync point
+    // immediately.
+    if (m->message.type() == GpuCommandBufferMsg_InsertSyncPoint::ID) {
+      const bool retire = m->retire_sync_point;
+      const uint32_t sync_point = m->sync_point_number;
+      const int32_t routing_id = m->message.routing_id();
+      if (stub) {
+        stub->AddSyncPoint(sync_point);
+        if (retire) {
+          m->message = GpuCommandBufferMsg_RetireSyncPoint(routing_id,
+                                                           sync_point);
+        }
+      } else {
+        current_order_num_ = m->order_number;
+        gpu_channel_manager_->sync_point_manager()->RetireSyncPoint(
+            sync_point);
+        channel_messages_.pop_front();
+        MessageProcessed(m->order_number);
+        return;
+      }
+    }
 
-  m = deferred_messages_.front();
-  stub = stubs_.get(m->routing_id());
-  if (stub) {
-    if (!stub->IsScheduled())
-      return;
-    if (stub->IsPreempted()) {
-      OnScheduled();
-      return;
+    if (stub) {
+      if (!stub->IsScheduled())
+        return;
+      if (stub->IsPreempted()) {
+        ScheduleHandleMessage();
+        return;
+      }
     }
+
+    channel_messages_.pop_front();
   }
 
-  scoped_ptr<IPC::Message> message(m);
-  deferred_messages_.pop_front();
+  scoped_ptr<ChannelMessage> scoped_message(m);
+  const uint32_t order_number = m->order_number;
+  IPC::Message* message = &m->message;
+
+  DVLOG(1) << "received message @" << message << " on channel @" << this
+           << " with type " << message->type();
+
   bool message_processed = true;
 
-  currently_processing_message_ = message.get();
+  if (order_number != static_cast<uint32_t>(-1)) {
+    // Make sure this is a valid unprocessed order number.
+    DCHECK(order_number <= GetUnprocessedOrderNum() &&
+           order_number >= GetProcessedOrderNum());
+
+    current_order_num_ = order_number;
+  }
   bool result;
   if (message->routing_id() == MSG_ROUTING_CONTROL)
     result = OnControlMessageReceived(*message);
   else
     result = router_.RouteMessage(*message);
-  currently_processing_message_ = NULL;
 
   if (!result) {
     // Respond to sync messages even if router failed to route.
@@ -734,18 +771,18 @@ void GpuChannel::HandleMessage() {
     // message to flush that command buffer.
     if (stub) {
       if (stub->HasUnprocessedCommands()) {
-        deferred_messages_.push_front(new GpuCommandBufferMsg_Rescheduled(
-            stub->route_id()));
+        PushUnfinishedMessage(
+            order_number, GpuCommandBufferMsg_Rescheduled(stub->route_id()));
         message_processed = false;
       }
     }
   }
   if (message_processed)
-    MessageProcessed();
+    MessageProcessed(order_number);
 
-  if (!deferred_messages_.empty()) {
-    OnScheduled();
-  }
+  base::AutoLock auto_lock(channel_messages_lock_);
+  if (!channel_messages_.empty())
+    ScheduleHandleMessageLocked();

[piman, 2015/08/31 23:15:04]

Arguably we could do this while the lock is taken the first time, to avoid
taking the lock a second time. The only place where channel_messages_.empty()
may change after that is if we push GpuCommandBufferMsg_Rescheduled, which will
call ScheduleHandleMessageLocked anyway.

[David Yen, 2015/09/01 02:01:52]

Good point, I guess I was trying to replicate old code and didn't think this
through. Done.

 }
 
 void GpuChannel::OnCreateOffscreenCommandBuffer(
@@ -814,12 +851,38 @@ void GpuChannel::OnCreateJpegDecoder(int32 route_id, IPC::Message* reply_msg) {
   jpeg_decoder_->AddClient(route_id, reply_msg);
 }
 
-void GpuChannel::MessageProcessed() {
-  messages_processed_++;
+void GpuChannel::PushUnfinishedMessage(uint32_t order_number,
+                                       const IPC::Message& message) {
+  // This is pushed in front only if it was unfinished, so order number is kept.
+  base::AutoLock auto_lock(channel_messages_lock_);
+  channel_messages_.push_front(new ChannelMessage(order_number, message));
+  ScheduleHandleMessageLocked();
+}
+
+void GpuChannel::ScheduleHandleMessage() {
+  base::AutoLock auto_lock(channel_messages_lock_);
+  ScheduleHandleMessageLocked();
+}
+
+void GpuChannel::ScheduleHandleMessageLocked() {
+  channel_messages_lock_.AssertAcquired();
+  if (!handle_messages_scheduled_) {

[piman, 2015/08/31 23:15:04]

You really only need to post a task if the queue was empty. If the queue is not
empty, the main thread is responsible for processing all messages, posting tasks
if needed. Because you're adding the message with the lock taken, it's either
one of 2 things:
- either the queue was empty, the main thread is idle, so we need to post a task
- or the queue was not empty, so the main thread is either descheduled or it
will have posted a task to handle the next message. Either way it will continue
processing, including this new message, if/when it is scheduled.

If we post a message here anyway, we may end up posting 2 tasks per message
(once here, and once on the main thread after processing the previous message),
which is inefficient (lots of tasks will end up early exiting).

That way, you can even remove handle_messages_scheduled_.

[David Yen, 2015/09/01 02:01:52]

Done. Although we cannot get rid of handle_messages_scheduled_ because the
descheduler uses it.

[piman, 2015/09/01 03:55:26]

I think the only place this is used is GpuCommandBufferStub::PollWork, and you
could use HasQueuedMessages() for it instead.

[David Yen, 2015/09/01 19:08:02]

Actually looking at GpuCommandBufferStub::PollWork(), we can now detect if
anything has been processed or queued by simply comparing
UnprocessedOrderNumber(). I'm modified the function to use that instead and
removed "handle_messages_scheduled_".

This could potentially be faster if it just accessed the global order number
instead, but I believe the scheduling code is going to change a lot after
streams goes in right?

+    task_runner_->PostTask(
+          FROM_HERE,
+          base::Bind(&GpuChannel::HandleMessage, weak_factory_.GetWeakPtr()));
+    handle_messages_scheduled_ = true;
+  }
+}
+
+void GpuChannel::MessageProcessed(uint32_t order_number) {
+  if (order_number != static_cast<uint32_t>(-1)) {
+    DCHECK(current_order_num_ == order_number);
+    processed_order_num_ = order_number;
+  }
   if (preempting_flag_.get()) {
     io_task_runner_->PostTask(
-        FROM_HERE, base::Bind(&GpuChannelMessageFilter::MessageProcessed,
-                              filter_, messages_processed_));
+        FROM_HERE, base::Bind(&GpuChannelMessageFilter::OnMessageProcessed,
+                              filter_));
   }
 }