Bug 1677509 - Use GetQueuedCompletionStatusEx in win message pump r=h…

…andyman This is almost certainly a very small optimization, and likely won't address the frequency of hangs from bug 1677509. However, it still should be an improvement, and will work on anything Vista or later. We observed as part of trying to diagnose the somewhat mysterious bug 1677509 that sometimes multiple messages would be queued, and yet the IPC I/O thread would go idle in between servicing them in GetQueuedCompletionStatusEx. Given that we send frequent sync ipc messages for mouse move events, it seems prudent to be able to service multiple at a time. Differential Revision: https://phabricator.services.mozilla.com/D114287
Brli · May 19, 2021 · 015a338 · 015a338
1 parent 6ff37f3
commit 015a338
Show file tree

Hide file tree

Showing 4 changed files with 50 additions and 38 deletions.
diff --git a/ipc/chromium/src/base/message_pump_win.cc b/ipc/chromium/src/base/message_pump_win.cc
@@ -452,45 +452,55 @@ void MessagePumpForIO::WaitForWork() {
 }
 
 bool MessagePumpForIO::WaitForIOCompletion(DWORD timeout, IOHandler* filter) {
-  IOItem item;
-  if (completed_io_.empty() || !MatchCompletedIOItem(filter, &item)) {
-    // We have to ask the system for another IO completion.
-    if (!GetIOItem(timeout, &item)) return false;
-
-    if (ProcessInternalIOItem(item)) return true;
+  IOItemChunk items;
+  if (completed_io_.empty() || !MatchCompletedIOItem(filter, items.values)) {
+    if (!GetIOItems(timeout, &items)) return false;
+  } else {
+    items.count = 1;
   }
 
-  if (item.context->handler) {
-    if (filter && item.handler != filter) {
-      // Save this item for later
-      completed_io_.push_back(item);
+  for (ULONG i = 0; i < items.count; ++i) {
+    IOItem& item = items.values[i];
+    if (ProcessInternalIOItem(item)) {
+      continue;
+    }
+
+    if (item.context->handler) {
+      if (filter && item.handler != filter) {
+        // Save this item for later
+        completed_io_.push_back(item);
+      } else {
+        DCHECK(item.context->handler == item.handler);
+        item.handler->OnIOCompleted(item.context, item.bytes_transfered);
+      }
     } else {
-      DCHECK(item.context->handler == item.handler);
-      item.handler->OnIOCompleted(item.context, item.bytes_transfered,
-                                  item.error);
+      // The handler must be gone by now, just cleanup the mess.
+      delete item.context;
     }
-  } else {
-    // The handler must be gone by now, just cleanup the mess.
-    delete item.context;
   }
+
   return true;
 }
 
 // Asks the OS for another IO completion result.
-bool MessagePumpForIO::GetIOItem(DWORD timeout, IOItem* item) {
-  memset(item, 0, sizeof(*item));
-  ULONG_PTR key = 0;
-  OVERLAPPED* overlapped = NULL;
+bool MessagePumpForIO::GetIOItems(DWORD timeout, IOItemChunk* items) {
+  memset(items, 0, sizeof(*items));
+  OVERLAPPED_ENTRY entries[arraysize(items->values)];
   AUTO_PROFILER_LABEL("MessagePumpForIO::GetIOItem::Wait", IDLE);
-  if (!GetQueuedCompletionStatus(port_.Get(), &item->bytes_transfered, &key,
-                                 &overlapped, timeout)) {
-    if (!overlapped) return false;  // Nothing in the queue.
-    item->error = GetLastError();
-    item->bytes_transfered = 0;
+  if (!GetQueuedCompletionStatusEx(port_.Get(), entries,
+                                   arraysize(items->values), &items->count,
+                                   timeout, FALSE)) {
+    return false;  // Nothing in the queue.
+  }
+
+  for (int i = 0; i < (int)items->count; ++i) {
+    items->values[i].handler =
+        reinterpret_cast<IOHandler*>(entries[i].lpCompletionKey);
+    items->values[i].bytes_transfered = entries[i].dwNumberOfBytesTransferred;
+    items->values[i].context =
+        reinterpret_cast<IOContext*>(entries[i].lpOverlapped);
   }
 
-  item->handler = reinterpret_cast<IOHandler*>(key);
-  item->context = reinterpret_cast<IOContext*>(overlapped);
   return true;
 }
 

diff --git a/ipc/chromium/src/base/message_pump_win.h b/ipc/chromium/src/base/message_pump_win.h
@@ -222,8 +222,8 @@ class MessagePumpForIO : public MessagePumpWin {
   //         delete context_;
   //      }
   //     }
-  //     virtual void OnIOCompleted(IOContext* context, DWORD bytes_transfered,
-  //                                DWORD error) {
+  //     virtual void OnIOCompleted(IOContext* context, DWORD bytes_transfered)
+  //     {
   //         pending_ = false;
   //     }
   //     void DoSomeIo() {
@@ -248,8 +248,8 @@ class MessagePumpForIO : public MessagePumpWin {
   //     // while there are pending IO operations.
   //     ~MyFile() {
   //     }
-  //     virtual void OnIOCompleted(IOContext* context, DWORD bytes_transfered,
-  //                                DWORD error) {
+  //     virtual void OnIOCompleted(IOContext* context, DWORD bytes_transfered)
+  //     {
   //       ...
   //       delete context;
   //     }
@@ -280,8 +280,7 @@ class MessagePumpForIO : public MessagePumpWin {
     // |context| completes. |error| is the Win32 error code of the IO operation
     // (ERROR_SUCCESS if there was no error). |bytes_transfered| will be zero
     // on error.
-    virtual void OnIOCompleted(IOContext* context, DWORD bytes_transfered,
-                               DWORD error) = 0;
+    virtual void OnIOCompleted(IOContext* context, DWORD bytes_transfered) = 0;
   };
 
   // The extended context that should be used as the base structure on every
@@ -326,13 +325,16 @@ class MessagePumpForIO : public MessagePumpWin {
     IOHandler* handler;
     IOContext* context;
     DWORD bytes_transfered;
-    DWORD error;
+  };
+  struct IOItemChunk {
+    IOItem values[8];
+    ULONG count;
   };
 
   virtual void DoRunLoop();
   void WaitForWork();
   bool MatchCompletedIOItem(IOHandler* filter, IOItem* item);
-  bool GetIOItem(DWORD timeout, IOItem* item);
+  bool GetIOItems(DWORD timeout, IOItemChunk* items);
   bool ProcessInternalIOItem(const IOItem& item);
 
   // The completion port associated with this thread.

diff --git a/ipc/chromium/src/chrome/common/ipc_channel_win.cc b/ipc/chromium/src/chrome/common/ipc_channel_win.cc
@@ -286,7 +286,7 @@ bool Channel::ChannelImpl::Connect() {
     // to true, we indicate to OnIOCompleted that this is the special
     // initialization signal.
     MessageLoopForIO::current()->PostTask(factory_.NewRunnableMethod(
-        &Channel::ChannelImpl::OnIOCompleted, &input_state_.context, 0, 0));
+        &Channel::ChannelImpl::OnIOCompleted, &input_state_.context, 0));
   }
 
   if (!waiting_connect_) ProcessOutgoingMessages(NULL, 0);
@@ -556,7 +556,7 @@ bool Channel::ChannelImpl::ProcessOutgoingMessages(
 }
 
 void Channel::ChannelImpl::OnIOCompleted(MessageLoopForIO::IOContext* context,
-                                         DWORD bytes_transfered, DWORD error) {
+                                         DWORD bytes_transfered) {
   bool ok;
   ASSERT_OWNINGTHREAD(ChannelImpl);
   if (context == &input_state_.context) {

diff --git a/ipc/chromium/src/chrome/common/ipc_channel_win.h b/ipc/chromium/src/chrome/common/ipc_channel_win.h
@@ -68,7 +68,7 @@ class Channel::ChannelImpl : public MessageLoopForIO::IOHandler {
 
   // MessageLoop::IOHandler implementation.
   virtual void OnIOCompleted(MessageLoopForIO::IOContext* context,
-                             DWORD bytes_transfered, DWORD error);
+                             DWORD bytes_transfered);
 
  private:
   struct State {