Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(807)

Issue 1181263008: Revert of Don't peek messages in the MessagePumpForUI class when we receive our kMsgHaveWork message (Closed)

Created:
5 years, 6 months ago by scottmg
Modified:
5 years, 6 months ago
CC:
chromium-reviews, jam, erikwright+watch_chromium.org, sadrul, piman+watch_chromium.org, darin-cc_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Revert of Don't peek messages in the MessagePumpForUI class when we receive our kMsgHaveWork message. (patchset #1 id:1 of https://codereview.chromium.org/1173193002/) Reason for revert: 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/ Original issue's description: > 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} TBR=cpu@chromium.org,ananta@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=492837, 500749 Committed: https://crrev.com/4d306ecabfa83ed5e7ee61a97e30e96064bc100a Cr-Commit-Position: refs/heads/master@{#335082}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+59 lines, -148 lines) Patch
M base/message_loop/incoming_task_queue.h View 1 chunk +0 lines, -3 lines 0 comments Download
M base/message_loop/incoming_task_queue.cc View 1 chunk +0 lines, -5 lines 0 comments Download
M base/message_loop/message_loop.h View 1 chunk +0 lines, -1 line 0 comments Download
M base/message_loop/message_loop.cc View 1 chunk +0 lines, -4 lines 0 comments Download
M base/message_loop/message_pump.h View 1 chunk +0 lines, -3 lines 0 comments Download
M base/message_loop/message_pump_win.h View 3 chunks +0 lines, -31 lines 0 comments Download
M base/message_loop/message_pump_win.cc View 4 chunks +58 lines, -87 lines 0 comments Download
M content/gpu/gpu_main.cc View 1 chunk +1 line, -14 lines 0 comments Download

Messages

Total messages: 6 (2 generated)
scottmg
Created Revert of Don't peek messages in the MessagePumpForUI class when we receive our kMsgHaveWork ...
5 years, 6 months ago (2015-06-18 18:14:43 UTC) #1
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1181263008/1
5 years, 6 months ago (2015-06-18 18:17:56 UTC) #4
commit-bot: I haz the power
Committed patchset #1 (id:1)
5 years, 6 months ago (2015-06-18 18:21:48 UTC) #5
commit-bot: I haz the power
5 years, 6 months ago (2015-06-18 18:22:28 UTC) #6
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/4d306ecabfa83ed5e7ee61a97e30e96064bc100a
Cr-Commit-Position: refs/heads/master@{#335082}

Powered by Google App Engine
This is Rietveld 408576698