Let GpuChannel handle deferred IPC messages of SwapBuffer in batch

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 0759e5d00f961569894377ec1e49bd3a7d2267fa..1f7974c21aed49c049919d29d67b36d8d6c85fdc 100644
--- a/content/common/gpu/gpu_channel.cc
+++ b/content/common/gpu/gpu_channel.cc
@@ -662,16 +662,66 @@ bool GpuChannel::OnControlMessageReceived(const IPC::Message& msg) {
   return handled;
 }
 
+size_t GpuChannel::MatchSwapBufferMessagesPattern(
+    IPC::Message* current_message) {
+  DCHECK(current_message);
+  if (deferred_messages_.empty() || !current_message)
+    return 0;
+  // Only care about SetLatencyInfo and AsyncFlush message.
+  if (current_message->type() != GpuCommandBufferMsg_SetLatencyInfo::ID &&
+      current_message->type() != GpuCommandBufferMsg_AsyncFlush::ID)
+    return 0;
+
+  size_t index = 0;
+  int32 routing_id = current_message->routing_id();
+
+  // In case of the current message is SetLatencyInfo, we try to look ahead one
+  // more deferred messages.
+  IPC::Message *first_message = NULL;
+  IPC::Message *second_message = NULL;
+
+  // Fetch the first message and move index to point to the second message.
+  first_message = deferred_messages_[index++];
+
+  // If the current message is AsyncFlush, the expected message sequence for
+  // SwapBuffer should be AsyncFlush->Echo. We only try to match Echo message.
+  if (current_message->type() == GpuCommandBufferMsg_AsyncFlush::ID &&
+      first_message->type() == GpuCommandBufferMsg_Echo::ID &&
+      first_message->routing_id() == routing_id) {
+    return 1;
+  }
+
+  // If the current message is SetLatencyInfo, the expected message sequence
+  // for SwapBuffer should be SetLatencyInfo->AsyncFlush->Echo (optional).
+  if (current_message->type() == GpuCommandBufferMsg_SetLatencyInfo::ID &&
+      first_message->type() == GpuCommandBufferMsg_AsyncFlush::ID &&
+      first_message->routing_id() == routing_id) {
+    if (deferred_messages_.size() >= 2)
+      second_message = deferred_messages_[index];
+    if (!second_message)
+      return 1;
+    if (second_message->type() == GpuCommandBufferMsg_Echo::ID &&
+        second_message->routing_id() == routing_id) {
+      return 2;
+    }
+  }
+  // No matched message is found.
+  return 0;
+}
+
 void GpuChannel::HandleMessage() {
   handle_messages_scheduled_ = false;
   if (deferred_messages_.empty())
     return;
 
-  bool should_fast_track_ack = false;
-  IPC::Message* m = deferred_messages_.front();
-  GpuCommandBufferStub* stub = stubs_.Lookup(m->routing_id());
+  size_t matched_messages_num = 0;
+  bool should_handle_swapbuffer_msgs_immediate = false;
+  IPC::Message* m = NULL;
+  GpuCommandBufferStub* stub = NULL;
 
   do {
+    m = deferred_messages_.front();
+    stub = stubs_.Lookup(m->routing_id());
     if (stub) {
       if (!stub->IsScheduled())
         return;
@@ -715,17 +765,29 @@ void GpuChannel::HandleMessage() {
     if (message_processed)
       MessageProcessed();
 
-    // We want the EchoACK following the SwapBuffers to be sent as close as
-    // possible, avoiding scheduling other channels in the meantime.
-    should_fast_track_ack = false;
-    if (!deferred_messages_.empty()) {
-      m = deferred_messages_.front();
-      stub = stubs_.Lookup(m->routing_id());
-      should_fast_track_ack =
-          (m->type() == GpuCommandBufferMsg_Echo::ID) &&
-          stub && stub->IsScheduled();
+    if (deferred_messages_.empty())
+      break;
+
+    // We process the pending messages immediately if these messages matches
+    // the pattern of SwapBuffers, for example, GLRenderer always issues
+    // SwapBuffers calls with a specific IPC message patterns, for example,
+    // it should be SetLatencyInfo->AsyncFlush->Echo sequence.
+    //
+    // Instead of posting a task to message loop, it could avoid the possibility
+    // of being blocked by other channels, and make SwapBuffers executed as soon
+    // as possible.
+    if (!should_handle_swapbuffer_msgs_immediate) {
+      // Start from the current processing message to match SwapBuffer pattern.
+      matched_messages_num = MatchSwapBufferMessagesPattern(message.get());

[Ken Russell (switch to Gerrit), 2014/08/26 18:18:55]

This isn't the same logic as in the previous patch set nor the original code.
Note that in the previous code deferred_messages_.pop_front() is called higher
up (line 685) before deferred_messages_.front() is called on line 722, so this
used to start the scan for GpuCommandBufferMsg_Echo::ID from the message after
the one currently processed. Now it'll start the look-ahead from whatever the
current message is, so it won't necessarily fast-track a SwapBuffers following
an AsyncFlush (think AsyncFlush -> SetLatencyInfo -> AsyncFlush -> Echo). How
did you intend this to work?

How was this change tested? There's a lack of unit tests for the GpuChannel.

[Hongbo Min, 2014/08/27 01:54:00]

Russell, thanks for your time.

On 2014/08/26 18:18:55, Ken Russell wrote:
Such a change would not change the logics. The parameter |message| passed to
MatchSwapBufferMessagesPattern is initialized from |m| which is assigned to the
front of |deferred_messages_|, it is always pointing to the message currently
being processed, no matter whether pop_front is called on |deferred_message_|.

During the process of MatchSwapBufferMessagesPattern, the first message of
|deferred_messages_| is actually the next message of the current processing
message |message|, so it can look ahead by fetching the first element of
|deferred_messages_|.

Take AsyncFlush -> SetLatencyInfo -> AsyncFlush -> Echo sequence for example.
Assume the current processing message is AsyncFlush, the
MatchSwapBufferMessagesPattern would return 0 indicating no SwapBufers found for
this AsyncFlush message, and let message loop schedule HandleMessage next time.
The next time of HandleMessage is called, the current processing message is
SetLatencyInfo, and MatchSwapBufferMessagesPattern would return 2, meaning it is
followed by 2 more messages which are made up of a SwapBuffers, and we will go
to next loop iteration instead of scheduling by message loop, and at the begin
of loop, it will fetch the first message of deferred message and repeat again
util no more pattern message.

For the original code, If the current processing message is AsyncFlush followed
by Echo, the MatchSwapBufferMessagesPattern will return 1, meaning the Echo
message should be fast-tracked, it is the same as the origin code.
Seems there is no any unittest for GpuChannel, no gpu_channel_unittest.cc file
is found. Is there any other place to add test for GpuChannel or write a new one
from scatch?

[Ken Russell (switch to Gerrit), 2014/08/27 22:16:56]

Thanks for pointing that out. I now see that message.get(), not m, was being
passed to MatchSwapBufferMessagesPattern in patch set 1. Agree that the new
structure is the same.
A new unit test file would be needed. I won't block this CL on the addition of
new tests, but if you would consider starting to add some as future work, that
would be very helpful.

+      should_handle_swapbuffer_msgs_immediate =
+          matched_messages_num > 0 && stub && stub->IsScheduled();

[jbauman, 2014/08/27 20:46:41]

I don't think you need to check for "stub && stub->IsScheduled()" here or in the
else branch.

[Hongbo Min, 2014/08/29 01:11:10]

Seems stub->IsScheduled is not needed since the above if clause could ensure it
is true, but we still need to check stub is null.

+    } else {
+      DCHECK_GT(matched_messages_num, 0);
+      --matched_messages_num;
+      if (!stub || !stub->IsScheduled() || matched_messages_num == 0)
+        should_handle_swapbuffer_msgs_immediate = false;
     }
-  } while (should_fast_track_ack);
+  } while (should_handle_swapbuffer_msgs_immediate);
 
   if (!deferred_messages_.empty()) {
     OnScheduled();