Avoid iterating over all handles in MessagePumpMojo on every iteration to calculate deadlines.

mojo/message_pump/message_pump_mojo.cc

Index: mojo/message_pump/message_pump_mojo.cc
diff --git a/mojo/message_pump/message_pump_mojo.cc b/mojo/message_pump/message_pump_mojo.cc
index a24349be3d721dcc418d50abd2645d959f593fbb..7248720b53b6d98214763a4680be93df0116fe35 100644
--- a/mojo/message_pump/message_pump_mojo.cc
+++ b/mojo/message_pump/message_pump_mojo.cc
@@ -91,10 +91,15 @@ void MessagePumpMojo::AddHandler(MessagePumpMojoHandler* handler,
   handler_data.deadline = deadline;
   handler_data.id = next_handler_id_++;
   handlers_[handle] = handler_data;
+  if (!deadline.is_null()) {
+    bool inserted = deadline_handles_.insert(handle).second;
+    DCHECK(inserted);
+  }
 }
 
 void MessagePumpMojo::RemoveHandler(const Handle& handle) {
   handlers_.erase(handle);
+  deadline_handles_.erase(handle);
 }
 
 void MessagePumpMojo::AddObserver(Observer* observer) {
@@ -207,21 +212,31 @@ bool MessagePumpMojo::DoInternalWork(const RunState& run_state, bool block) {
     }
   }
 
-  // Notify and remove any handlers whose time has expired. Make a copy in case
-  // someone tries to add/remove new handlers from notification.
-  const HandleToHandler cloned_handlers(handlers_);
+  // Notify and remove any handlers whose time has expired. First, iterate over
+  // the set of handles that have a deadline, and add the expired handles to a
+  // map of <Handle, id>. Then, iterate over those expired handles and remove
+  // them. The two-step process is because a handler can add/remove new
+  // handlers.
+  std::map<Handle, int> expired_handles;
   const base::TimeTicks now(internal::NowTicks());
-  for (HandleToHandler::const_iterator i = cloned_handlers.begin();
-       i != cloned_handlers.end(); ++i) {
-    // Since we're iterating over a clone of the handlers, verify the handler is
-    // still valid before notifying.
-    if (!i->second.deadline.is_null() && i->second.deadline < now &&
-        handlers_.find(i->first) != handlers_.end() &&
-        handlers_[i->first].id == i->second.id) {
+  for (const Handle handle : deadline_handles_) {
+    const auto it = handlers_.find(handle);
+    // Expect any handle in |deadline_handles_| to also be in |handlers_| since
+    // the two are modified in lock-step.
+    DCHECK(it != handlers_.end());
+    if (!it->second.deadline.is_null() && it->second.deadline < now)
+      expired_handles[handle] = it->second.id;
+  }
+  for (auto h : expired_handles) {

[sky, 2015/11/17 18:35:11]

auto& . And naming what is a pair 'h' is rather confusing and misleading. Use
pair.

[Anand Mistry (off Chromium), 2015/11/17 23:54:39]

Done.

+    auto it = handlers_.find(h.first);
+    // Don't need to check deadline again since it can't change if id hasn't
+    // changed.
+    if (it != handlers_.end() && it->second.id == h.second) {
+      MessagePumpMojoHandler* handler = handlers_[h.first].handler;
+      RemoveHandler(h.first);
       WillSignalHandler();
-      i->second.handler->OnHandleError(i->first, MOJO_RESULT_DEADLINE_EXCEEDED);
+      handler->OnHandleError(h.first, MOJO_RESULT_DEADLINE_EXCEEDED);
       DidSignalHandler();
-      handlers_.erase(i->first);
       did_work = true;
     }
   }
@@ -241,7 +256,9 @@ void MessagePumpMojo::RemoveInvalidHandle(const WaitState& wait_state,
   CHECK(handlers_.find(wait_state.handles[index]) != handlers_.end());
   MessagePumpMojoHandler* handler =
       handlers_[wait_state.handles[index]].handler;
-  handlers_.erase(wait_state.handles[index]);
+  Handle handle = wait_state.handles[index];
+  handlers_.erase(handle);

[sky, 2015/11/17 18:35:11]

Replace these two lines with RemoveHandler.

[Anand Mistry (off Chromium), 2015/11/17 23:54:39]

Done.

+  deadline_handles_.erase(handle);
   WillSignalHandler();
   handler->OnHandleError(wait_state.handles[index], result);
   DidSignalHandler();
@@ -275,10 +292,11 @@ MojoDeadline MessagePumpMojo::GetDeadlineForWait(
   const base::TimeTicks now(internal::NowTicks());
   MojoDeadline deadline = TimeTicksToMojoDeadline(run_state.delayed_work_time,
                                                   now);
-  for (HandleToHandler::const_iterator i = handlers_.begin();
-       i != handlers_.end(); ++i) {
+  for (const Handle handle : deadline_handles_) {
+    auto it = handlers_.find(handle);
+    DCHECK(it != handlers_.end());
     deadline = std::min(
-        TimeTicksToMojoDeadline(i->second.deadline, now), deadline);
+        TimeTicksToMojoDeadline(it->second.deadline, now), deadline);
   }
   return deadline;
 }