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..909cc4fe2874199fc9cb8db80b4a7c1d715f14a5 100644
--- a/content/common/gpu/gpu_channel.cc
+++ b/content/common/gpu/gpu_channel.cc
@@ -9,7 +9,7 @@
 #include "content/common/gpu/gpu_channel.h"
 
 #include <algorithm>
-#include <queue>
+#include <deque>
 #include <vector>
 
 #include "base/bind.h"
@@ -18,8 +18,8 @@
 #include "base/single_thread_task_runner.h"
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
+#include "base/synchronization/lock.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"
@@ -67,6 +67,162 @@ const int64 kStopPreemptThresholdMs = kVsyncIntervalMs;
 
 }  // anonymous namespace
 
+struct ChannelMessage {
+  uint32_t order_number;
+  base::TimeTicks time_received;
+  IPC::Message message;
+
+  // TODO(dyen): Temporary sync point data, remove once new sync point lands.
+  bool retire_sync_point;
+  uint32 sync_point_number;
+
+  ChannelMessage(uint32_t order_num, const IPC::Message& msg)
+    : order_number(order_num),
+      time_received(base::TimeTicks::Now()),
+      message(msg),
+      retire_sync_point(false),
+      sync_point_number(0) {}
+};
+
+class GpuChannelMessageQueue : public base::RefCounted<GpuChannelMessageQueue> {

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

Needs to be RefCountedThreadSafe

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

Done.

+ public:
+  static scoped_refptr<GpuChannelMessageQueue> Create(
+      GpuChannelManager* gpu_channel_manager,
+      base::WeakPtr<GpuChannel> gpu_channel,
+      scoped_refptr<base::SingleThreadTaskRunner> task_runner) {
+    return new GpuChannelMessageQueue(gpu_channel_manager, gpu_channel,
+                                     task_runner);
+  }
+
+  void PushBackMessage(uint32_t order_number,
+                                   const IPC::Message& message) {

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

nit: indent, here and below (run git cl format)

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

Done.

+    base::subtle::Release_Store(&unprocessed_order_num_, order_number);
+
+    base::AutoLock auto_lock(channel_messages_lock_);
+    const bool was_empty = channel_messages_.empty();
+    channel_messages_.push_back(new ChannelMessage(order_number, message));
+    if (was_empty)
+      ScheduleHandleMessageLocked();
+  }
+
+  void 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_);
+    const bool was_empty = channel_messages_.empty();
+    channel_messages_.push_front(new ChannelMessage(static_cast<uint32_t>(-1),

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

nit: maybe define a const uint32_t kOutOfOrderNumer = -1u; and use here and
below.

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

Done.

+                                                    message));
+    if (was_empty)
+      ScheduleHandleMessageLocked();
+  }
+
+  void PushSyncPointMessage(uint32_t order_number,
+                                        const IPC::Message& message,
+                                        bool retire_sync_point,
+                                        uint32_t sync_point_num) {
+    DCHECK(message.type() == GpuCommandBufferMsg_InsertSyncPoint::ID);
+
+    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;
+
+    base::AutoLock auto_lock(channel_messages_lock_);
+    const bool was_empty = channel_messages_.empty();
+    channel_messages_.push_back(msg);
+    if (was_empty)
+      ScheduleHandleMessageLocked();
+  }

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

nit: all 3 functions (and PushUnfinishedMessage) could maybe use a helper:

void PushMessageAndSchedule(ChannelMessage* message) {
  base::AutoLock auto_lock(channel_messages_lock_);
  const bool was_empty = channel_messages_.empty();
  channel_messages_.push_back(message);
  if (was_empty)
    ScheduleHandleMessageLocked();
}

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

Done.

+
+  bool HasQueuedMessages() {
+    base::AutoLock auto_lock(channel_messages_lock_);
+    return !channel_messages_.empty();
+  }
+
+  base::TimeTicks 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;
+  }
+
+ protected:
+  virtual ~GpuChannelMessageQueue() {

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

This logic needs to happen on the main thread.

Since this is refcounted by both the filter and the GpuChannel, you have no
guarantee where the destructor is run.


Instead, cleaning up the sync point messages should probably happen by an
explicit call from GpuChannel in its destructor. This could also set a flag so
that the filter stops creating sync point numbers, and allocating
ChannelMessages.

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

Ah, I just noticed that too. I've moved it to the main thread and added an
"enabled_" boolean. I also had to move the sync point generating code to be
behind the lock to or else there would be a race condition, it is all handled
within the GpuChannelMessageQueue::GenerateSyncPointMessage() now.

+    base::AutoLock auto_lock(channel_messages_lock_);
+    for (const auto& msg : channel_messages_) {

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

I'd suggest making this ChannelMessage* msg for clarity. auto& makes it sound
like it's a ChannelMessage reference.

Also, you can simply delete msg in this loop, rather than running a second loop
in STLDeleteElements.

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

Done.

+      const uint32_t sync_point = msg->sync_point_number;
+      gpu_channel_manager_->sync_point_manager()->RetireSyncPoint(sync_point);

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

if (sync_point)
  gpu_channel_manager_->sync_point_manager()->RetireSyncPoint(sync_point);

(RetireSyncPoint is non-trivial, you don't want to run it on 0 for every
non-InsertSyncPoint message).

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

Done.

+    }
+    STLDeleteElements(&channel_messages_);
+  }
+
+ private:
+  friend class GpuChannel;
+  friend class base::RefCounted<GpuChannelMessageQueue>;
+
+  GpuChannelMessageQueue(
+      GpuChannelManager* gpu_channel_manager,
+      base::WeakPtr<GpuChannel> gpu_channel,
+      scoped_refptr<base::SingleThreadTaskRunner> task_runner)
+      : unprocessed_order_num_(0),
+        handle_messages_scheduled_(false),
+        gpu_channel_manager_(gpu_channel_manager),
+        gpu_channel_(gpu_channel),
+        task_runner_(task_runner) {
+  }
+
+  void PushUnfinishedMessage(uint32_t order_number,
+                             const IPC::Message& message) {
+    // This is pushed only if it was unfinished, so order number is kept.
+    base::AutoLock auto_lock(channel_messages_lock_);
+    const bool was_empty = channel_messages_.empty();
+    channel_messages_.push_front(new ChannelMessage(order_number, message));
+    if (was_empty)
+      ScheduleHandleMessageLocked();
+  }
+
+  void ScheduleHandleMessage() {
+    base::AutoLock auto_lock(channel_messages_lock_);
+    ScheduleHandleMessageLocked();
+  }
+
+  void ScheduleHandleMessageLocked() {
+    channel_messages_lock_.AssertAcquired();
+    if (!handle_messages_scheduled_) {
+      task_runner_->PostTask(
+          FROM_HERE,
+          base::Bind(&GpuChannel::HandleMessage, gpu_channel_));
+      handle_messages_scheduled_ = true;
+    }
+  }
+
+  // Highest IPC order number seen, set when queued on the IO thread.
+  base::subtle::Atomic32 unprocessed_order_num_;

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

If we really need this, I'd prefer it to be a uint32_t, and accessed under the
lock. Other access patterns are incorrect unless proven correct - it encodes the
highest global order in the queue, but that's racy if you don't have the lock
held.

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

Done.

+
+  std::deque<ChannelMessage*> channel_messages_;
+  bool handle_messages_scheduled_;
+
+  // This lock protects both handle_messages_scheduled_ and channel_messages_.
+  base::Lock channel_messages_lock_;
+
+  GpuChannelManager* gpu_channel_manager_;
+  base::WeakPtr<GpuChannel> gpu_channel_;
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(GpuChannelMessageQueue);
+};
+
 // This filter does three things:
 // - it counts and timestamps each message forwarded to the channel
 //   so that we can preempt other channels if a message takes too long to
@@ -81,16 +237,17 @@ const int64 kStopPreemptThresholdMs = kVsyncIntervalMs;
 class GpuChannelMessageFilter : public IPC::MessageFilter {
  public:
   GpuChannelMessageFilter(
+      scoped_refptr<GpuChannelMessageQueue> message_queue,
       base::WeakPtr<GpuChannel> gpu_channel,
       gpu::SyncPointManager* sync_point_manager,
       scoped_refptr<base::SingleThreadTaskRunner> task_runner,
       bool future_sync_points)
       : preemption_state_(IDLE),
+        message_queue_(message_queue),
         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 +266,7 @@ class GpuChannelMessageFilter : public IPC::MessageFilter {
   bool OnMessageReceived(const IPC::Message& message) override {
     DCHECK(sender_);
 
+    const uint32_t order_number = global_order_counter_++;
     bool handled = false;
     if ((message.type() == GpuCommandBufferMsg_RetireSyncPoint::ID) &&
         !future_sync_points_) {
@@ -135,11 +293,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));
+
+      message_queue_->PushSyncPointMessage(order_number, message,
+                                           base::get<0>(retire), sync_point);
       handled = true;
     }
 
@@ -154,19 +310,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.
+        message_queue_->PushFrontMessage(message);
+      } else {
+        message_queue_->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 +373,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() && message_queue_->HasQueuedMessages())
           TransitionToWaiting();
         break;
       case WAITING:
@@ -233,21 +384,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 = message_queue_->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 +428,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 = message_queue_->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 +518,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 message_queue_ is used to handle messages on the main thread.
+  // The gpu_channel_ weak pointer is only dereferenced on the
+  // main thread - therefore the WeakPtr assumptions are respected.
+  scoped_refptr<GpuChannelMessageQueue> message_queue_;
   base::WeakPtr<GpuChannel> gpu_channel_;

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

I don't think you need this any more.

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

Done.

   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_;
 
@@ -412,8 +535,13 @@ class GpuChannelMessageFilter : public IPC::MessageFilter {
 
   // True if this channel can create future sync points.
   bool future_sync_points_;
+
+  // This number is only ever incremented/read on the IO thread.
+  static uint32_t global_order_counter_;
 };
 
+uint32_t GpuChannelMessageFilter::global_order_counter_ = 0;
+
 GpuChannel::GpuChannel(GpuChannelManager* gpu_channel_manager,
                        GpuWatchdog* watchdog,
                        gfx::GLShareGroup* share_group,
@@ -426,7 +554,6 @@ 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),
       client_id_(client_id),
       client_tracing_id_(client_tracing_id),
       task_runner_(task_runner),
@@ -439,16 +566,21 @@ 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),
       num_stubs_descheduled_(0),
       allow_future_sync_points_(allow_future_sync_points),
       weak_factory_(this) {
   DCHECK(gpu_channel_manager);
   DCHECK(client_id);
 
+  message_queue_ = GpuChannelMessageQueue::Create(gpu_channel_manager,
+                                                  weak_factory_.GetWeakPtr(),
+                                                  task_runner);
+
   filter_ = new GpuChannelMessageFilter(
-      weak_factory_.GetWeakPtr(), gpu_channel_manager_->sync_point_manager(),
+      message_queue_, weak_factory_.GetWeakPtr(),
+      gpu_channel_manager_->sync_point_manager(),
       task_runner_, allow_future_sync_points_);
 
   subscription_ref_set_->AddObserver(this);
@@ -458,7 +590,6 @@ GpuChannel::~GpuChannel() {
   // Clear stubs first because of dependencies.
   stubs_.clear();
 
-  STLDeleteElements(&deferred_messages_);
   subscription_ref_set_->RemoveObserver(this);
   if (preempting_flag_.get())
     preempting_flag_->Reset();
@@ -493,21 +624,9 @@ base::ProcessId GpuChannel::GetClientPID() const {
 }
 
 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));
-  }
-
-  OnScheduled();
-
-  return true;
+  // All messages should be pushed to channel_messages_ and handled separately.
+  NOTREACHED();
+  return false;
 }
 
 void GpuChannel::OnChannelError() {
@@ -540,32 +659,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();
+    message_queue_->ScheduleHandleMessage();
   } else {
     num_stubs_descheduled_++;
   }
@@ -662,6 +760,11 @@ gpu::PreemptionFlag* GpuChannel::GetPreemptionFlag() {
   return preempting_flag_.get();
 }
 
+bool GpuChannel::handle_messages_scheduled() const {
+  base::AutoLock auto_lock(message_queue_->channel_messages_lock_);
+  return message_queue_->handle_messages_scheduled_;
+}
+
 void GpuChannel::SetPreemptByFlag(
     scoped_refptr<gpu::PreemptionFlag> preempted_flag) {
   preempted_flag_ = preempted_flag;
@@ -691,35 +794,83 @@ 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;
+  uint32_t sync_point_retired = 0;
+  {
+    base::AutoLock auto_lock(message_queue_->channel_messages_lock_);
+    message_queue_->handle_messages_scheduled_ = false;
+    if (message_queue_->channel_messages_.empty()) {
+      return;
+    }
+    m = message_queue_->channel_messages_.front();
+    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) {

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

Why not move that part of the logic after the lock release too?

In particular, if unscheduled/preempted, we'll go through this multiple times,
which means we'll do AddSyncPoint multiple times - that's bad.

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

Done.

+      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 {
+        sync_point_retired = sync_point;
+      }
+    }
 
-  m = deferred_messages_.front();
-  stub = stubs_.get(m->routing_id());
-  if (stub) {
-    if (!stub->IsScheduled())
-      return;
-    if (stub->IsPreempted()) {
-      OnScheduled();
-      return;
+    if (!sync_point_retired && stub) {
+      if (!stub->IsScheduled())
+        return;
+      if (stub->IsPreempted()) {
+        message_queue_->ScheduleHandleMessageLocked();
+        return;
+      }
     }
+
+    message_queue_->channel_messages_.pop_front();
+    if (!message_queue_->channel_messages_.empty())
+      message_queue_->ScheduleHandleMessageLocked();
+  }
+
+  // TODO(dyen): Temporary handling of old sync points.
+  if (sync_point_retired) {
+    current_order_num_ = m->order_number;
+    gpu_channel_manager_->sync_point_manager()->RetireSyncPoint(
+        sync_point_retired);
+    MessageProcessed(m->order_number);
+    return;
   }
 
-  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 +885,14 @@ void GpuChannel::HandleMessage() {
     // message to flush that command buffer.
     if (stub) {
       if (stub->HasUnprocessedCommands()) {
-        deferred_messages_.push_front(new GpuCommandBufferMsg_Rescheduled(
-            stub->route_id()));
+        message_queue_->PushUnfinishedMessage(
+            order_number, GpuCommandBufferMsg_Rescheduled(stub->route_id()));
         message_processed = false;
       }
     }
   }
   if (message_processed)
-    MessageProcessed();
-
-  if (!deferred_messages_.empty()) {
-    OnScheduled();
-  }
+    MessageProcessed(order_number);
 }
 
 void GpuChannel::OnCreateOffscreenCommandBuffer(
@@ -814,12 +961,15 @@ void GpuChannel::OnCreateJpegDecoder(int32 route_id, IPC::Message* reply_msg) {
   jpeg_decoder_->AddClient(route_id, reply_msg);
 }
 
-void GpuChannel::MessageProcessed() {
-  messages_processed_++;
+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_));
   }
 }
 
@@ -888,4 +1038,8 @@ void GpuChannel::HandleUpdateValueState(
   pending_valuebuffer_state_->UpdateState(target, state);
 }
 
+uint32_t GpuChannel::GetUnprocessedOrderNum() const {
+  return base::subtle::Acquire_Load(&message_queue_->unprocessed_order_num_);
+}
+
 }  // namespace content