Don't peek messages in the MessagePumpForUI class when we receive our kMsgHaveWork message.

Don't peek messages in the MessagePumpForUI class when we receive our kMsgHaveWork message.

Relanding this patch with fixes for content_unittests failures. We need to wait for the UI worker thread to start.

Currently the MessagePumpForUI class peeks Windows messages when we receive the kMsgHaveWork message in
our main message loop and in nested modal loops. This is because the posted message starves the message loop
of input and other lower priority messages. While this is ok for the main message loop our peeking and dispatching
messages in nested loops is wrong and violates the silent contract where in the nested loop should be the one peeking
and dispatching messages.

To fix this the approach we are taking is to create a worker thread which uses a waitable timer of 3 ms which posts
the kMsgHaveWork message to the main loop when the timer fires. As a result we can safely delete all the code
in the MessagePumpForUI::ScheduleWork function and simplify the ProcessPumpReplacementMessage function.

The MessagePumpForUI::ScheduleWork function now checks the delay for the task at the head of the queue
If it is 0 then we post the message right away as it is a regular task. Added functions (GetNewlyAddedTaskDelay) to the MessagePump::Delegate class and the IncomingTaskQueue for this.

The other change here is to change the GPU process to use the IO message pump by default and use the UI pump only
if we are using opengl. Reason being this patch causes a delay in processing tasks due to the worker thread which
causes tests like webgl_conformance to fail. We will continue working on addressing this over the coming days.

BUG=492837
TBR=cpu, scottmg

Committed: https://crrev.com/29a20657bb92335c4e671cc1ee4c5e36ee1ffa1b
Cr-Commit-Position: refs/heads/master@{#333831}

[scottmg, 2015-06-10 22:32:18]

lgtm

[ananta, 2015-06-10 22:49:32]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2015-06-10 22:49:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1173193002/1

[commit-bot: I haz the power, 2015-06-10 23:08:53]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-06-10 23:10:48]

Patchset 1 (id:??) landed as
https://crrev.com/29a20657bb92335c4e671cc1ee4c5e36ee1ffa1b
Cr-Commit-Position: refs/heads/master@{#333831}

[brucedawson, 2015-06-18 00:28:30]

This change is problematic. Depending on the timer frequency this has been seen
to cause the watcher process to wake up 64 to 300 times per second, communicate
a few times with the browser process, and then go back to sleep. The net result
is up to 7,200 additional context switches per second (many context switches
each time) and frequent enough CPU usage to prevent CPUs from going into low
power mode.

I noticed this accidentally when doing some profiling of stable when I had
canary sitting idle in the background at about:blank. The profile showed that
the canary browser was doing ~4,800 context switches/sec and the watcher was
doing ~2,400 context switches/sec - more than any other processes on the system.
I then verified the bug with a hit-count breakpoint on
SetWakeupTimer(kDefaultUIWorkerThreadWakeupTimerMs) in
MessagePumpForUI::DoWorkerThreadRunLoop() which was hit 1,000 times in ~9
seconds.

I have not looked enough to know what the correct fix - presumably only setting
the wakeup timer when it is needed.

[brucedawson, 2015-06-18 00:36:36]

CPU usage of the browser and watcher processes is approximately 7% and 3.5% of a
core.

[scottmg, 2015-06-18 03:33:23]

On 2015/06/18 00:36:36, brucedawson wrote:
> CPU usage of the browser and watcher processes is approximately 7% and 3.5% of
a
> core.

Also https://code.google.com/p/chromium/issues/detail?id=500749 indicates power
regression, so I think we should revert this. I'm not clear if it was fixing an
existing bug or not?

Maybe cpu knows?

[ananta, 2015-06-18 12:29:51]

On 2015/06/18 03:33:23, scottmg wrote:
> On 2015/06/18 00:36:36, brucedawson wrote:
> > CPU usage of the browser and watcher processes is approximately 7% and 3.5%
of
> a
> > core.
> 
> Also https://code.google.com/p/chromium/issues/detail?id=500749 indicates
power
> regression, so I think we should revert this. I'm not clear if it was fixing
an
> existing bug or not?
> 
> Maybe cpu knows?

Our UI message pump prior to this patch used to peek and dispatch messages even
in the context of
nested modal loops like sizing/dialog boxes etc which basically breaks the
contract in the sense that
the modal loop should peek and dispatch messages. 

We can look into fixing the worker thread waking up frequently to process tasks
by setting the waitable timer
in the thread only once for each iteration i.e. ScheduleWork sets the timer ->
the thread wakes up -> posts the wake
up message -> sets the waitable timer once and then waits for schedulework to do
it again. The second part can be done
via a bool flag or something. scottmg if you have cycles can you look into
that?.

[ananta, 2015-06-18 13:21:26]

On 2015/06/18 12:29:51, ananta wrote:
> On 2015/06/18 03:33:23, scottmg wrote:
> > On 2015/06/18 00:36:36, brucedawson wrote:
> > > CPU usage of the browser and watcher processes is approximately 7% and
3.5%
> of
> > a
> > > core.
> > 
> > Also https://code.google.com/p/chromium/issues/detail?id=500749 indicates
> power
> > regression, so I think we should revert this. I'm not clear if it was fixing
> an
> > existing bug or not?
> > 
> > Maybe cpu knows?
> 
> Our UI message pump prior to this patch used to peek and dispatch messages
even
> in the context of
> nested modal loops like sizing/dialog boxes etc which basically breaks the
> contract in the sense that
> the modal loop should peek and dispatch messages. 
> 
> We can look into fixing the worker thread waking up frequently to process
tasks
> by setting the waitable timer
> in the thread only once for each iteration i.e. ScheduleWork sets the timer ->
> the thread wakes up -> posts the wake
> up message -> sets the waitable timer once and then waits for schedulework to
do
> it again. The second part can be done
> via a bool flag or something. scottmg if you have cycles can you look into
> that?.

Hi scottmg

I will try and send out a patch to address this problem soon. If that does not
address it we can
revert this patch.

Thanks
Ananta

[scottmg, 2015-06-18 18:12:58]

On 2015/06/18 13:21:26, ananta wrote:
> On 2015/06/18 12:29:51, ananta wrote:
> > On 2015/06/18 03:33:23, scottmg wrote:
> > > On 2015/06/18 00:36:36, brucedawson wrote:
> > > > CPU usage of the browser and watcher processes is approximately 7% and
> 3.5%
> > of
> > > a
> > > > core.
> > > 
> > > Also https://code.google.com/p/chromium/issues/detail?id=500749 indicates
> > power
> > > regression, so I think we should revert this. I'm not clear if it was
fixing
> > an
> > > existing bug or not?
> > > 
> > > Maybe cpu knows?
> > 
> > Our UI message pump prior to this patch used to peek and dispatch messages
> even
> > in the context of
> > nested modal loops like sizing/dialog boxes etc which basically breaks the
> > contract in the sense that
> > the modal loop should peek and dispatch messages. 
> > 
> > We can look into fixing the worker thread waking up frequently to process
> tasks
> > by setting the waitable timer
> > in the thread only once for each iteration i.e. ScheduleWork sets the timer
->
> > the thread wakes up -> posts the wake
> > up message -> sets the waitable timer once and then waits for schedulework
to
> do
> > it again. The second part can be done
> > via a bool flag or something. scottmg if you have cycles can you look into
> > that?.
> 
> Hi scottmg
> 
> I will try and send out a patch to address this problem soon. If that does not
> address it we can
> revert this patch.
> 
> Thanks
> Ananta

Thanks, the other patch looks a bit subtle. I don't think this is an urgent fix?
So I think we should revert this CL, and then discuss and reland it with the fix
when you return. No point in being worried about it while on vacation.

Having the overall change as a single patch is better for seeing what's
happening on perf bots and if we end up having to do any merges to branches.

[scottmg, 2015-06-18 18:13:16]

On 2015/06/18 18:12:58, scottmg wrote:
> On 2015/06/18 13:21:26, ananta wrote:
> > On 2015/06/18 12:29:51, ananta wrote:
> > > On 2015/06/18 03:33:23, scottmg wrote:
> > > > On 2015/06/18 00:36:36, brucedawson wrote:
> > > > > CPU usage of the browser and watcher processes is approximately 7% and
> > 3.5%
> > > of
> > > > a
> > > > > core.
> > > > 
> > > > Also https://code.google.com/p/chromium/issues/detail?id=500749
indicates
> > > power
> > > > regression, so I think we should revert this. I'm not clear if it was
> fixing
> > > an
> > > > existing bug or not?
> > > > 
> > > > Maybe cpu knows?
> > > 
> > > Our UI message pump prior to this patch used to peek and dispatch messages
> > even
> > > in the context of
> > > nested modal loops like sizing/dialog boxes etc which basically breaks the
> > > contract in the sense that
> > > the modal loop should peek and dispatch messages. 
> > > 
> > > We can look into fixing the worker thread waking up frequently to process
> > tasks
> > > by setting the waitable timer
> > > in the thread only once for each iteration i.e. ScheduleWork sets the
timer
> ->
> > > the thread wakes up -> posts the wake
> > > up message -> sets the waitable timer once and then waits for schedulework
> to
> > do
> > > it again. The second part can be done
> > > via a bool flag or something. scottmg if you have cycles can you look into
> > > that?.
> > 
> > Hi scottmg
> > 
> > I will try and send out a patch to address this problem soon. If that does
not
> > address it we can
> > revert this patch.
> > 
> > Thanks
> > Ananta
> 
> Thanks, the other patch looks a bit subtle. I don't think this is an urgent
fix?
> So I think we should revert this CL, and then discuss and reland it with the
fix
> when you return. No point in being worried about it while on vacation.
> 
> Having the overall change as a single patch is better for seeing what's
> happening on perf bots and if we end up having to do any merges to branches.

(Other patch here, for xref: https://codereview.chromium.org/1189133003/)

[scottmg, 2015-06-18 18:14:43]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/1181263008/ by scottmg@chromium.org.

The reason for reverting is: Causing frequent wakeups (especially with watcher
process?) and shows power regression on perf bots
https://code.google.com/p/chromium/issues/detail?id=500749.

Potential fix at https://codereview.chromium.org/1189133003/.