Process pending delayed tasks in kMsgHaveWork

base/message_loop/message_pump_win.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.
 
 #include "base/message_loop/message_pump_win.h"
 
 #include <limits>
 #include <math.h>
 
 #include "base/message_loop/message_loop.h"
@@ skipping 128 matching lines @@
   //
   // We use a single SetTimer corresponding to the timer that will expire
   // soonest.  As new timers are created and destroyed, we update SetTimer.
   // Getting a spurrious SetTimer event firing is benign, as we'll just be
   // processing an empty timer queue.
   //
   delayed_work_time_ = delayed_work_time;
 
   int delay_msec = GetCurrentDelay();
   DCHECK_GE(delay_msec, 0);
-  if (delay_msec < USER_TIMER_MINIMUM)
-    delay_msec = USER_TIMER_MINIMUM;
+  if (delay_msec == 0) {
+    ScheduleWork();
+  } else {
+    if (delay_msec < USER_TIMER_MINIMUM)
+      delay_msec = USER_TIMER_MINIMUM;
 
-  // Create a WM_TIMER event that will wake us up to check for any pending
-  // timers (in case we are running within a nested, external sub-pump).
-  BOOL ret = SetTimer(message_hwnd_, reinterpret_cast<UINT_PTR>(this),
-                      delay_msec, NULL);
-  if (ret)
-    return;
-  // If we can't set timers, we are in big trouble... but cross our fingers for
-  // now.
-  // TODO(jar): If we don't see this error, use a CHECK() here instead.
-  UMA_HISTOGRAM_ENUMERATION("Chrome.MessageLoopProblem", SET_TIMER_ERROR,
-                            MESSAGE_LOOP_PROBLEM_MAX);
+    // Create a WM_TIMER event that will wake us up to check for any pending
+    // timers (in case we are running within a nested, external sub-pump).
+    BOOL ret = SetTimer(message_hwnd_, reinterpret_cast<UINT_PTR>(this),
+                        delay_msec, NULL);
+    if (ret)
+      return;
+    // If we can't set timers, we are in big trouble... but cross our fingers
+    // for now.
+    // TODO(jar): If we don't see this error, use a CHECK() here instead.
+    UMA_HISTOGRAM_ENUMERATION("Chrome.MessageLoopProblem", SET_TIMER_ERROR,
+                              MESSAGE_LOOP_PROBLEM_MAX);
+  }
 }
 
 //-----------------------------------------------------------------------------
 // MessagePumpForUI private:
 
 // static
 LRESULT CALLBACK MessagePumpForUI::WndProcThunk(
     HWND hwnd, UINT message, WPARAM wparam, LPARAM lparam) {
   // TODO(vadimt): Remove ScopedTracker below once crbug.com/440919 is fixed.
   tracked_objects::ScopedTracker tracking_profile1(
@@ skipping 138 matching lines @@
 
   // Let whatever would have run had we not been putting messages in the queue
   // run now.  This is an attempt to make our dummy message not starve other
   // messages that may be in the Windows message queue.
   ProcessPumpReplacementMessage();
 
   // Now give the delegate a chance to do some work.  He'll let us know if he
   // needs to do more work.
   if (state_->delegate->DoWork())
     ScheduleWork();
+  state_->delegate->DoDelayedWork(&delayed_work_time_);

[jar (doing other things), 2015/05/13 20:01:21]

Note that you *might* effectively double the amount of work (number of tasks)
done during each HandleWorkMessage.  Worse yet, it might actually swell up the
regular task queue during an "overload," rather than "do less work" when we fall
behind.  The cases where this is crucial are indeed very sensitive to resulting
jank (due to CPU usage).

The only reason (I think) for this HandleWorkMessage method to run is that we're
in a nested (native?) message loop, and not getting to run our traditional list
of chores (which do include both a shot at regular tasks, as well as delayed
tasks).  In that nested-loop situation, it is commonly the case that the OS
really wants to tightly control responsiveness (which is why they ran a tight
native loop)... so we only very intermittently run a Chrome Task.  We
deliberately allow them to get progressively more cycles (proportionately) if
they are processing a pile of native queue messages.

Perhaps we should change lines 323/324 to:

 if (state_->delegate->DoWork()) {
   ScheduleWork();
   return;  // We did absolutely minimal amount of work.
  }
  // Fallthrough to see if DelayedWork needs to be done.


You could argue that we should randomly try DoDelayedWork vs DoWork... and
perhaps that could be crafted in here.

IMO, a more common case would be that a delayed task fires, and it posts a bunch
of tasks (or tasks that post tasks), and it is probably better to handle most
all of those "first" (or at least ASAP), rather than doing some more delayed
tasks "sooner" and perhaps REALLY growing our regular task queue.

+  if (!delayed_work_time_.is_null()) {
+    // A bit gratuitous to set delayed_work_time_ again, but oh well.
+    ScheduleDelayedWork(delayed_work_time_);
+  }
 }
 
 void MessagePumpForUI::HandleTimerMessage() {
   KillTimer(message_hwnd_, reinterpret_cast<UINT_PTR>(this));
 
   // If we are being called outside of the context of Run, then don't do
   // anything.  This could correspond to a MessageBox call or something of
   // that sort.
   if (!state_)
     return;
 
   state_->delegate->DoDelayedWork(&delayed_work_time_);
   if (!delayed_work_time_.is_null()) {
     // A bit gratuitous to set delayed_work_time_ again, but oh well.

[jar (doing other things), 2015/05/13 20:01:21]

The CL is plausible.... but I'm not a fan of replicated code (line 327 and 343).

I'm not sure of how or why to refactor... but certainly copying a confusing
comment is questionable.  

Can you clarify the meaning of the comment?

[jbauman, 2015/05/20 01:31:35]

Ok, refactored out the timer rescheduling logic to a separate function so
there's less duplication.

     ScheduleDelayedWork(delayed_work_time_);
   }
 }
 
 bool MessagePumpForUI::ProcessNextWindowsMessage() {
   // TODO(vadimt): Remove ScopedTracker below once crbug.com/440919 is fixed.
   tracked_objects::ScopedTracker tracking_profile1(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "440919 MessagePumpForUI::ProcessNextWindowsMessage1"));
 
@@ skipping 345 matching lines @@
 
 // static
 MessagePumpForIO::IOHandler* MessagePumpForIO::KeyToHandler(
     ULONG_PTR key,
     bool* has_valid_io_context) {
   *has_valid_io_context = ((key & 1) == 0);
   return reinterpret_cast<IOHandler*>(key & ~static_cast<ULONG_PTR>(1));
 }
 
 }  // namespace base