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

content/common/gpu/gpu_channel.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.
 
 #if defined(OS_WIN)
 #include <windows.h>
 #endif
 
 #include "content/common/gpu/gpu_channel.h"
 
 #include <algorithm>
 #include <queue>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/location.h"
 #include "base/single_thread_task_runner.h"
 #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"
 #include "content/common/gpu/gpu_channel_manager.h"
 #include "content/common/gpu/gpu_memory_buffer_factory.h"
 #include "content/common/gpu/gpu_messages.h"
 #include "content/common/gpu/media/gpu_jpeg_decode_accelerator.h"
 #include "content/public/common/content_switches.h"
 #include "gpu/command_buffer/common/mailbox.h"
 #include "gpu/command_buffer/common/value_state.h"
@@ skipping 41 matching lines @@
 //   before preempting and we limit the amount of time that we can preempt in
 //   one shot (see constants above).
 // - it handles the GpuCommandBufferMsg_InsertSyncPoint message on the IO
 //   thread, generating the sync point ID and responding immediately, and then
 //   posting a task to insert the GpuCommandBufferMsg_RetireSyncPoint message
 //   into the channel's queue.
 // - it generates mailbox names for clients of the GPU process on the IO thread.
 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) {}
 
   void OnFilterAdded(IPC::Sender* sender) override {
     DCHECK(!sender_);
     sender_ = sender;
     timer_ = make_scoped_ptr(new base::OneShotTimer<GpuChannelMessageFilter>);
   }
 
   void OnFilterRemoved() override {
     DCHECK(sender_);
     sender_ = nullptr;
     timer_ = nullptr;
   }
 
   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_) {
       DLOG(ERROR) << "Untrusted client should not send "
                      "GpuCommandBufferMsg_RetireSyncPoint message";
       return true;
     }
 
     if (message.type() == GpuCommandBufferMsg_InsertSyncPoint::ID) {
       base::Tuple<bool> retire;
       IPC::Message* reply = IPC::SyncMessage::GenerateReply(&message);
       if (!GpuCommandBufferMsg_InsertSyncPoint::ReadSendParam(&message,
                                                               &retire)) {
         reply->set_reply_error();
         Send(reply);
         return true;
       }
       if (!future_sync_points_ && !base::get<0>(retire)) {
         LOG(ERROR) << "Untrusted contexts can't create future sync points";
         reply->set_reply_error();
         Send(reply);
         return true;
       }
       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;
     }
 
     // These are handled by GpuJpegDecodeAccelerator and
     // GpuVideoDecodeAccelerator.
     // TODO(kcwu) Modify GpuChannel::AddFilter to handle additional filters by
     // GpuChannelMessageFilter instead of by IPC::SyncChannel directly. Then we
     // don't need to exclude them one by one here.
     if (message.type() == AcceleratedJpegDecoderMsg_Decode::ID ||
         message.type() == AcceleratedJpegDecoderMsg_Destroy::ID ||
         message.type() == AcceleratedVideoDecoderMsg_Decode::ID) {
       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_));
+    // 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();
   }
 
   void SetPreemptingFlagAndSchedulingState(
       gpu::PreemptionFlag* preempting_flag,
       bool a_stub_is_descheduled) {
     preempting_flag_ = preempting_flag;
     a_stub_is_descheduled_ = a_stub_is_descheduled;
   }
 
@@ skipping 25 matching lines @@
     // We would like to preempt, but some stub is descheduled.
     WOULD_PREEMPT_DESCHEDULED,
   };
 
   PreemptionState preemption_state_;
 
   // Maximum amount of time that we can spend in PREEMPTING.
   // 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:
         // A timer will transition us to CHECKING.
         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;
       case PREEMPTING:
         // A TransitionToIdle() timer should always be running in this state.
         DCHECK(timer_->IsRunning());
         if (a_stub_is_descheduled_)
           TransitionToWouldPreemptDescheduled();
         else
           TransitionToIdleIfCaughtUp();
         break;
       case WOULD_PREEMPT_DESCHEDULED:
         // A TransitionToIdle() timer should never be running in this state.
         DCHECK(!timer_->IsRunning());
         if (!a_stub_is_descheduled_)
           TransitionToPreempting();
         else
           TransitionToIdleIfCaughtUp();
         break;
       default:
         NOTREACHED();
     }
   }
 
   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();
     }
   }
 
   void TransitionToIdle() {
     DCHECK(preemption_state_ == PREEMPTING ||
            preemption_state_ == WOULD_PREEMPT_DESCHEDULED);
     // Stop any outstanding timer set to force us from PREEMPTING to IDLE.
     timer_->Stop();
@@ skipping 65 matching lines @@
       }
     }
 
     preemption_state_ = WOULD_PREEMPT_DESCHEDULED;
     preempting_flag_->Reset();
     TRACE_COUNTER_ID1("gpu", "GpuChannel::Preempting", this, 0);
 
     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_;
 
   bool a_stub_is_descheduled_;
 
   // True if this channel can create future sync points.
   bool future_sync_points_;
 };
 
 GpuChannel::GpuChannel(GpuChannelManager* gpu_channel_manager,
                        GpuWatchdog* watchdog,
                        gfx::GLShareGroup* share_group,
                        gpu::gles2::MailboxManager* mailbox,
                        base::SingleThreadTaskRunner* task_runner,
                        base::SingleThreadTaskRunner* io_task_runner,
                        int client_id,
                        uint64_t client_tracing_id,
                        bool software,
                        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),
       io_task_runner_(io_task_runner),
       share_group_(share_group ? share_group : new gfx::GLShareGroup),
       mailbox_manager_(mailbox
                            ? scoped_refptr<gpu::gles2::MailboxManager>(mailbox)
                            : gpu::gles2::MailboxManager::Create()),
       subscription_ref_set_(new gpu::gles2::SubscriptionRefSet),
       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) {
   DCHECK(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);
 }
 
 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();
 }
 
 IPC::ChannelHandle GpuChannel::Init(base::WaitableEvent* shutdown_event,
                                     IPC::AttachmentBroker* attachment_broker) {
   DCHECK(shutdown_event);
   DCHECK(!channel_);
 
@@ skipping 13 matching lines @@
 
   channel_->AddFilter(filter_.get());
 
   return channel_handle;
 }
 
 base::ProcessId GpuChannel::GetClientPID() const {
   return channel_->GetPeerPID();
 }
 
+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);
+
+  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_);
+  channel_messages_.push_back(msg);
+  ScheduleHandleMessageLocked();
+}
+
 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() {
   gpu_channel_manager_->RemoveChannel(client_id_);
 }
 
 bool GpuChannel::Send(IPC::Message* message) {
   // The GPU process must never send a synchronous IPC message to the renderer
   // process. This could result in deadlock.
   DCHECK(!message->is_sync());
@@ skipping 12 matching lines @@
 void GpuChannel::OnAddSubscription(unsigned int target) {
   gpu_channel_manager()->Send(
       new GpuHostMsg_AddSubscription(client_id_, target));
 }
 
 void GpuChannel::OnRemoveSubscription(unsigned int target) {
   gpu_channel_manager()->Send(
       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_++;
   }
   DCHECK_LE(num_stubs_descheduled_, stubs_.size());
   bool a_stub_is_descheduled = num_stubs_descheduled_ > 0;
 
   if (a_stub_is_descheduled != a_stub_was_descheduled) {
     if (preempting_flag_.get()) {
       io_task_runner_->PostTask(
           FROM_HERE,
@@ skipping 105 matching lines @@
                         OnDestroyCommandBuffer)
     IPC_MESSAGE_HANDLER_DELAY_REPLY(GpuMsg_CreateJpegDecoder,
                                     OnCreateJpegDecoder)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   DCHECK(handled) << msg.type();
   return handled;
 }
 
 void GpuChannel::HandleMessage() {
-  handle_messages_scheduled_ = false;
-  if (deferred_messages_.empty())
-    return;
-
-  IPC::Message* m = NULL;
-  GpuCommandBufferStub* stub = NULL;
-
-  m = deferred_messages_.front();
-  stub = stubs_.get(m->routing_id());
-  if (stub) {
-    if (!stub->IsScheduled())
-      return;
-    if (stub->IsPreempted()) {
-      OnScheduled();
+  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());
+
+    // 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;
+      }
+    }
+
+    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.
     if (message->is_sync()) {
       IPC::Message* reply = IPC::SyncMessage::GenerateReply(&*message);
       reply->set_reply_error();
       Send(reply);
     }
   } else {
     // If the command buffer becomes unscheduled as a result of handling the
     // message but still has more commands to process, synthesize an IPC
     // 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(
     const gfx::Size& size,
     const GPUCreateCommandBufferConfig& init_params,
     int32 route_id,
     bool* succeeded) {
   TRACE_EVENT1("gpu", "GpuChannel::OnCreateOffscreenCommandBuffer", "route_id",
                route_id);
 
@@ skipping 48 matching lines @@
   }
 }
 
 void GpuChannel::OnCreateJpegDecoder(int32 route_id, IPC::Message* reply_msg) {
   if (!jpeg_decoder_) {
     jpeg_decoder_.reset(new GpuJpegDecodeAccelerator(this, io_task_runner_));
   }
   jpeg_decoder_->AddClient(route_id, reply_msg);
 }
 
-void GpuChannel::MessageProcessed() {
-  messages_processed_++;
-  if (preempting_flag_.get()) {
-    io_task_runner_->PostTask(
-        FROM_HERE, base::Bind(&GpuChannelMessageFilter::MessageProcessed,
-                              filter_, 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::OnMessageProcessed,
+                              filter_));
+  }
+}
+
 void GpuChannel::CacheShader(const std::string& key,
                              const std::string& shader) {
   gpu_channel_manager_->Send(
       new GpuHostMsg_CacheShader(client_id_, key, shader));
 }
 
 void GpuChannel::AddFilter(IPC::MessageFilter* filter) {
   channel_->AddFilter(filter);
 }
 
@@ skipping 46 matching lines @@
     }
   }
 }
 
 void GpuChannel::HandleUpdateValueState(
     unsigned int target, const gpu::ValueState& state) {
   pending_valuebuffer_state_->UpdateState(target, state);
 }
 
 }  // namespace content