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

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

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

Description

Don't peek messages in the MessagePumpForUI class when we receive our kMsgHaveWork message. Relanding this patch with fixes for the performance issues reported with the worker thread hogging the CPU continuosly. (https://codereview.chromium.org/1181263008/) 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. To prevent the UI worker thread from continuously posting the timer we use a flag to control the same. The flag ensures that the worker thread posts the timer once for each iteration. This patch also contains a fix for the crash reported in bug 501602 which occurs due to the GetNewlyAddedTaskDelay function being called from the WndProc which does not have the incoming queue lock. Fix is to move some of the logic in the ScheduleWork function to the newly added ScheduleWorkHelper function. BUG=492837, 501602 TBR=cpu Committed: https://crrev.com/4d5496cde5b81436954e31a9fe82e35ccfe5de49 Cr-Commit-Position: refs/heads/master@{#335475}

Patch Set 1 #

Patch Set 2 : Fix the crash reported in bug 501602. The patch description has been updated with comments about th… #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+172 lines, -57 lines) Patch
M base/message_loop/incoming_task_queue.h View 1 chunk +3 lines, -0 lines 0 comments Download
M base/message_loop/incoming_task_queue.cc View 1 chunk +5 lines, -0 lines 0 comments Download
M base/message_loop/message_loop.h View 1 chunk +1 line, -0 lines 0 comments Download
M base/message_loop/message_loop.cc View 1 chunk +4 lines, -0 lines 0 comments Download
M base/message_loop/message_pump.h View 1 chunk +3 lines, -0 lines 0 comments Download
M base/message_loop/message_pump_win.h View 1 3 chunks +40 lines, -0 lines 0 comments Download
M base/message_loop/message_pump_win.cc View 1 6 chunks +102 lines, -56 lines 1 comment Download
M content/gpu/gpu_main.cc View 1 chunk +14 lines, -1 line 0 comments Download

Messages

Total messages: 26 (10 generated)
ananta
5 years, 6 months ago (2015-06-18 18:59:55 UTC) #2
scottmg
The added flag looks ok to me. Maybe +Bruce can confirm that this doesn't cause ...
5 years, 6 months ago (2015-06-18 20:40:45 UTC) #4
brucedawson1
I'm at the airport now but I will definitely check it out tomorrow. It looks ...
5 years, 6 months ago (2015-06-18 21:12:19 UTC) #5
cpu_(ooo_6.6-7.5)
adding kbr unless somehow we can get jbauman to take a peek, but seems OOO.
5 years, 6 months ago (2015-06-19 21:35:37 UTC) #7
ananta
Applied brucedawson's comments from the previous patch https://codereview.chromium.org/1189133003 to this patch
5 years, 6 months ago (2015-06-19 22:20:06 UTC) #8
Ken Russell (switch to Gerrit)
content/gpu change LGTM +jmadill and geofflang as FYI geofflang has done more experimentation and we ...
5 years, 6 months ago (2015-06-19 22:28:23 UTC) #9
scottmg
lgtm
5 years, 6 months ago (2015-06-19 22:46:02 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1194673004/20001
5 years, 6 months ago (2015-06-20 23:12:25 UTC) #13
commit-bot: I haz the power
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator_ninja/builds/37327)
5 years, 6 months ago (2015-06-21 01:38:31 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1194673004/20001
5 years, 6 months ago (2015-06-21 04:25:50 UTC) #17
commit-bot: I haz the power
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator_ninja/builds/37342)
5 years, 6 months ago (2015-06-21 06:23:46 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1194673004/20001
5 years, 6 months ago (2015-06-22 04:37:26 UTC) #21
commit-bot: I haz the power
Committed patchset #2 (id:20001)
5 years, 6 months ago (2015-06-22 06:10:51 UTC) #22
commit-bot: I haz the power
Patchset 2 (id:??) landed as https://crrev.com/4d5496cde5b81436954e31a9fe82e35ccfe5de49 Cr-Commit-Position: refs/heads/master@{#335475}
5 years, 6 months ago (2015-06-22 06:11:41 UTC) #23
jbauman
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/1222013004/ by jbauman@chromium.org. ...
5 years, 5 months ago (2015-07-01 21:16:08 UTC) #24
dcheng
5 years, 5 months ago (2015-07-01 22:50:47 UTC) #26
Message was sent while issue was closed.
https://codereview.chromium.org/1194673004/diff/20001/base/message_loop/messa...
File base/message_loop/message_pump_win.cc (right):

https://codereview.chromium.org/1194673004/diff/20001/base/message_loop/messa...
base/message_loop/message_pump_win.cc:480: LARGE_INTEGER due_time = {0};
FYI, when you reland this, please use {} instead of {0} to avoid tickling a
Clang warning.

Using {} instead of {0} also leads to slightly better codegen for VC:
https://randomascii.wordpress.com/2011/08/15/visual-c-code-gen-deficiencies/#3

Powered by Google App Engine
This is Rietveld 408576698