Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1714263002:
Version of MessagePumpForUI optimized for GPU process (Closed)
|
Created:
4 years, 10 months ago by stanisc Modified:
4 years, 8 months ago CC:
chromium-reviews, jam, darin-cc_chromium.org, sadrul, piman+watch_chromium.org, vmpstr+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionVersion of MessagePumpForUI optimized for GPU process.
The purpose of this change is to reduce overhead of
MessagePump in GPU process. According to my testing on
Windows 10 Dell XPS laptop this change improved the
power usage by 0.15 Wt in one test and 0.12 Wt in another.
The test was a windowed playback of 1080p @ 60 Hz H.264
video.
Based on ETW sample profile the overhead of the message
pump is reduced from 13-15% to 7-10%. The overhead is
calculated as a number of inclusive samples in DoRunLoop
that are not part of DoWork and DoDelayedWork.
The change eliminates some system calls that shouldn't be needed
in the context of GPU process such as GetQueueStatus,
SetTimer, KillTimer, and minimizes other system calls that
came up high in the sampling profile - PeekMessage,
MsgWaitForMultipleObjectsEx, PostMessage.
While this version of MessagePump is still capable to peek
and dispatch Windows messages which apparently is needed
in the GPU process on Windows version above Win7,
based on the usage pattern analysis, the emphasis gets
shifted to handling the work (vs. handling Windows messages in MessagePumpForUI).
See the bug for more details.
If this work well in a limited context of the GPU process
some of these ideas might later be applied to other
types of MessagePump.
BUG=588798
Committed: https://crrev.com/f71e6d2cbb90cacc88ce41a48475c47c6b7a3543
Cr-Commit-Position: refs/heads/master@{#386898}
Patch Set 1 : #
Total comments: 2
Patch Set 2 : Got rid of MessageBaseForUIBase and used PostThreadMessage #Patch Set 3 : Handle messages for other threads without exiting WaitForWork #
Total comments: 26
Patch Set 4 : Addressed CR feedback #
Total comments: 8
Patch Set 5 : Addressed CR feedback #Patch Set 6 : Added <memory> include #Patch Set 7 : Two more comparisons to flip #
Total comments: 8
Patch Set 8 : Added CallMsgFilter call #
Messages
Total messages: 40 (19 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Experiment: GPU optimized version of MessagePumpForUI BUG= ========== to ========== Version of MessagePumpForUI optimized for GPU process. The purpose of this change is to reduce overhead of MessagePump in GPU process and improve power usage when in video playback scenarios. According to my testing on Windows 10 Dell XPS laptop with Battor device this change improves the overall power usage by 0.15 Wt (6.3 to 6.15) when doing a windowed playback of 1080p @ 60 Hz H.264 video. If this work well in a limited context of the GPU process some of these ideas might later be applied to other types of MessagePump. The change eliminates some system calls that shouldn't be needed in the context of GPU process such as GetQueueStatus, SetTimer, KillTimer, and minimizes other system calls that came up high in the sampling profile - PeekMessage, MsgWaitForMultipleObjectsEx, PostMessage. While this version of MessagePump is still capable to peek and dispatch Windows messages which apparently is needed in the GPU process, bases on the usage pattern analysis, the emphasis is shifted to handling the work. BUG=588798 ==========
Description was changed from ========== Version of MessagePumpForUI optimized for GPU process. The purpose of this change is to reduce overhead of MessagePump in GPU process and improve power usage when in video playback scenarios. According to my testing on Windows 10 Dell XPS laptop with Battor device this change improves the overall power usage by 0.15 Wt (6.3 to 6.15) when doing a windowed playback of 1080p @ 60 Hz H.264 video. If this work well in a limited context of the GPU process some of these ideas might later be applied to other types of MessagePump. The change eliminates some system calls that shouldn't be needed in the context of GPU process such as GetQueueStatus, SetTimer, KillTimer, and minimizes other system calls that came up high in the sampling profile - PeekMessage, MsgWaitForMultipleObjectsEx, PostMessage. While this version of MessagePump is still capable to peek and dispatch Windows messages which apparently is needed in the GPU process, bases on the usage pattern analysis, the emphasis is shifted to handling the work. BUG=588798 ========== to ========== Version of MessagePumpForUI optimized for GPU process. The purpose of this change is to reduce overhead of MessagePump in GPU process. According to my testing on Windows 10 Dell XPS laptop with Battor device this change improves the overall power usage by 0.15 Wt (6.3 to 6.15) when doing a windowed playback of 1080p @ 60 Hz H.264 video. If this work well in a limited context of the GPU process some of these ideas might later be applied to other types of MessagePump. The change eliminates some system calls that shouldn't be needed in the context of GPU process such as GetQueueStatus, SetTimer, KillTimer, and minimizes other system calls that came up high in the sampling profile - PeekMessage, MsgWaitForMultipleObjectsEx, PostMessage. While this version of MessagePump is still capable to peek and dispatch Windows messages which apparently is needed in the GPU process, bases on the usage pattern analysis, the emphasis is shifted to handling the work. BUG=588798 ==========
Description was changed from ========== Version of MessagePumpForUI optimized for GPU process. The purpose of this change is to reduce overhead of MessagePump in GPU process. According to my testing on Windows 10 Dell XPS laptop with Battor device this change improves the overall power usage by 0.15 Wt (6.3 to 6.15) when doing a windowed playback of 1080p @ 60 Hz H.264 video. If this work well in a limited context of the GPU process some of these ideas might later be applied to other types of MessagePump. The change eliminates some system calls that shouldn't be needed in the context of GPU process such as GetQueueStatus, SetTimer, KillTimer, and minimizes other system calls that came up high in the sampling profile - PeekMessage, MsgWaitForMultipleObjectsEx, PostMessage. While this version of MessagePump is still capable to peek and dispatch Windows messages which apparently is needed in the GPU process, bases on the usage pattern analysis, the emphasis is shifted to handling the work. BUG=588798 ========== to ========== Version of MessagePumpForUI optimized for GPU process. The purpose of this change is to reduce overhead of MessagePump in GPU process. According to my testing on Windows 10 Dell XPS laptop with Battor device this change improves the overall power usage by 0.15 Wt (6.3 to 6.15) when doing a windowed playback of 1080p @ 60 Hz H.264 video. If this work well in a limited context of the GPU process some of these ideas might later be applied to other types of MessagePump. The change eliminates some system calls that shouldn't be needed in the context of GPU process such as GetQueueStatus, SetTimer, KillTimer, and minimizes other system calls that came up high in the sampling profile - PeekMessage, MsgWaitForMultipleObjectsEx, PostMessage. While this version of MessagePump is still capable to peek and dispatch Windows messages which apparently is needed in the GPU process, bases on the usage pattern analysis, the emphasis is shifted to handling the work. BUG=588798 ==========
Description was changed from ========== Version of MessagePumpForUI optimized for GPU process. The purpose of this change is to reduce overhead of MessagePump in GPU process. According to my testing on Windows 10 Dell XPS laptop with Battor device this change improves the overall power usage by 0.15 Wt (6.3 to 6.15) when doing a windowed playback of 1080p @ 60 Hz H.264 video. If this work well in a limited context of the GPU process some of these ideas might later be applied to other types of MessagePump. The change eliminates some system calls that shouldn't be needed in the context of GPU process such as GetQueueStatus, SetTimer, KillTimer, and minimizes other system calls that came up high in the sampling profile - PeekMessage, MsgWaitForMultipleObjectsEx, PostMessage. While this version of MessagePump is still capable to peek and dispatch Windows messages which apparently is needed in the GPU process, bases on the usage pattern analysis, the emphasis is shifted to handling the work. BUG=588798 ========== to ========== Version of MessagePumpForUI optimized for GPU process. The purpose of this change is to reduce overhead of MessagePump in GPU process. According to my testing on Windows 10 Dell XPS laptop with Battor device this change improves the overall power usage by 0.15 Wt (6.3 to 6.15) when doing a windowed playback of 1080p @ 60 Hz H.264 video. The change eliminates some system calls that shouldn't be needed in the context of GPU process such as GetQueueStatus, SetTimer, KillTimer, and minimizes other system calls that came up high in the sampling profile - PeekMessage, MsgWaitForMultipleObjectsEx, PostMessage. While this version of MessagePump is still capable to peek and dispatch Windows messages which apparently is needed in the GPU process, bases on the usage pattern analysis, the emphasis is shifted to handling the work. If this work well in a limited context of the GPU process some of these ideas might later be applied to other types of MessagePump. BUG=588798 ==========
stanisc@chromium.org changed reviewers: + jbauman@chromium.org, thakis@chromium.org
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Version of MessagePumpForUI optimized for GPU process. The purpose of this change is to reduce overhead of MessagePump in GPU process. According to my testing on Windows 10 Dell XPS laptop with Battor device this change improves the overall power usage by 0.15 Wt (6.3 to 6.15) when doing a windowed playback of 1080p @ 60 Hz H.264 video. The change eliminates some system calls that shouldn't be needed in the context of GPU process such as GetQueueStatus, SetTimer, KillTimer, and minimizes other system calls that came up high in the sampling profile - PeekMessage, MsgWaitForMultipleObjectsEx, PostMessage. While this version of MessagePump is still capable to peek and dispatch Windows messages which apparently is needed in the GPU process, bases on the usage pattern analysis, the emphasis is shifted to handling the work. If this work well in a limited context of the GPU process some of these ideas might later be applied to other types of MessagePump. BUG=588798 ========== to ========== Version of MessagePumpForUI optimized for GPU process. The purpose of this change is to reduce overhead of MessagePump in GPU process. According to my testing on Windows 10 Dell XPS laptop with Battor device this change improves the overall power usage by 0.15 Wt (6.3 to 6.15) when doing a windowed playback of 1080p @ 60 Hz H.264 video. The change eliminates some system calls that shouldn't be needed in the context of GPU process such as GetQueueStatus, SetTimer, KillTimer, and minimizes other system calls that came up high in the sampling profile - PeekMessage, MsgWaitForMultipleObjectsEx, PostMessage. While this version of MessagePump is still capable to peek and dispatch Windows messages which apparently is needed in the GPU process on Windows version above Win7, based on the usage pattern analysis, the emphasis gets shifted to handling the work (vs. handling Windows messages in MessagePumpForUI). If this work well in a limited context of the GPU process some of these ideas might later be applied to other types of MessagePump. BUG=588798 ==========
Description was changed from ========== Version of MessagePumpForUI optimized for GPU process. The purpose of this change is to reduce overhead of MessagePump in GPU process. According to my testing on Windows 10 Dell XPS laptop with Battor device this change improves the overall power usage by 0.15 Wt (6.3 to 6.15) when doing a windowed playback of 1080p @ 60 Hz H.264 video. The change eliminates some system calls that shouldn't be needed in the context of GPU process such as GetQueueStatus, SetTimer, KillTimer, and minimizes other system calls that came up high in the sampling profile - PeekMessage, MsgWaitForMultipleObjectsEx, PostMessage. While this version of MessagePump is still capable to peek and dispatch Windows messages which apparently is needed in the GPU process on Windows version above Win7, based on the usage pattern analysis, the emphasis gets shifted to handling the work (vs. handling Windows messages in MessagePumpForUI). If this work well in a limited context of the GPU process some of these ideas might later be applied to other types of MessagePump. BUG=588798 ========== to ========== Version of MessagePumpForUI optimized for GPU process. The purpose of this change is to reduce overhead of MessagePump in GPU process. According to my testing on Windows 10 Dell XPS laptop with Battor device this change improves the overall power usage by 0.15 Wt (6.3 to 6.15) when doing a windowed playback of 1080p @ 60 Hz H.264 video. The change eliminates some system calls that shouldn't be needed in the context of GPU process such as GetQueueStatus, SetTimer, KillTimer, and minimizes other system calls that came up high in the sampling profile - PeekMessage, MsgWaitForMultipleObjectsEx, PostMessage. While this version of MessagePump is still capable to peek and dispatch Windows messages which apparently is needed in the GPU process on Windows version above Win7, based on the usage pattern analysis, the emphasis gets shifted to handling the work (vs. handling Windows messages in MessagePumpForUI). See the bug for more details. If this work well in a limited context of the GPU process some of these ideas might later be applied to other types of MessagePump. BUG=588798 ==========
Description was changed from ========== Version of MessagePumpForUI optimized for GPU process. The purpose of this change is to reduce overhead of MessagePump in GPU process. According to my testing on Windows 10 Dell XPS laptop with Battor device this change improves the overall power usage by 0.15 Wt (6.3 to 6.15) when doing a windowed playback of 1080p @ 60 Hz H.264 video. The change eliminates some system calls that shouldn't be needed in the context of GPU process such as GetQueueStatus, SetTimer, KillTimer, and minimizes other system calls that came up high in the sampling profile - PeekMessage, MsgWaitForMultipleObjectsEx, PostMessage. While this version of MessagePump is still capable to peek and dispatch Windows messages which apparently is needed in the GPU process on Windows version above Win7, based on the usage pattern analysis, the emphasis gets shifted to handling the work (vs. handling Windows messages in MessagePumpForUI). See the bug for more details. If this work well in a limited context of the GPU process some of these ideas might later be applied to other types of MessagePump. BUG=588798 ========== to ========== Version of MessagePumpForUI optimized for GPU process. The purpose of this change is to reduce overhead of MessagePump in GPU process. According to my testing on Windows 10 Dell XPS laptop with Battor device this change improves the overall power usage by 0.15 Wt (6.3 to 6.15) when doing a windowed playback of 1080p @ 60 Hz H.264 video. Based on ETW sample profile the overhead of the message pump is reduced from 15% to 7%. The overhead is calculated as a number of inclusive samples in DoRunLoop that are not part of DoWork and DoDelayedWork. The change eliminates some system calls that shouldn't be needed in the context of GPU process such as GetQueueStatus, SetTimer, KillTimer, and minimizes other system calls that came up high in the sampling profile - PeekMessage, MsgWaitForMultipleObjectsEx, PostMessage. While this version of MessagePump is still capable to peek and dispatch Windows messages which apparently is needed in the GPU process on Windows version above Win7, based on the usage pattern analysis, the emphasis gets shifted to handling the work (vs. handling Windows messages in MessagePumpForUI). See the bug for more details. If this work well in a limited context of the GPU process some of these ideas might later be applied to other types of MessagePump. BUG=588798 ==========
PTAL!
https://codereview.chromium.org/1714263002/diff/60001/base/message_loop/messa... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/1714263002/diff/60001/base/message_loop/messa... base/message_loop/message_pump_win.cc:87: InitMessageWnd(); We could essentially get rid of MessagePumpForUIBase as a separate class and avoid complicating MessagePumpForUI even more by either using an event object to schedule MsgWaitForMultipleObjectsEx (this may even help power usage), or by using PostThreadMessage so we don't have to bother with the message window, etc.
https://codereview.chromium.org/1714263002/diff/60001/base/message_loop/messa... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/1714263002/diff/60001/base/message_loop/messa... base/message_loop/message_pump_win.cc:87: InitMessageWnd(); On 2016/03/18 01:00:50, jbauman wrote: > We could essentially get rid of MessagePumpForUIBase as a separate class and > avoid complicating MessagePumpForUI even more by either using an event object to > schedule MsgWaitForMultipleObjectsEx (this may even help power usage), or by > using PostThreadMessage so we don't have to bother with the message window, etc. Thanks for the feedback! I've tried a separate class with an event. It worked fine on Windows 7 but would hang on Windows 10. I guess a Windows message loop is still needed. One issue with SetEvent is that it temporarily boosts receiving thread priority and might result in preempting the producer thread on a machine with a small number of CPU cores. I think in case of GPU process that would just result in an increasing the number of context switches and doing more work, not less. I'll go ahead and rewrite this to use a completely separate class and PostThreadMessage.
I've removed the base class and reimplemented MessagePumpForGpu to derive from MessagePumpWin and to use PostThreadMessage. PTAL!
lgtm
Nico, could you take a look or recommend someone else to review this?
Patchset #3 (id:100001) has been deleted
Description was changed from ========== Version of MessagePumpForUI optimized for GPU process. The purpose of this change is to reduce overhead of MessagePump in GPU process. According to my testing on Windows 10 Dell XPS laptop with Battor device this change improves the overall power usage by 0.15 Wt (6.3 to 6.15) when doing a windowed playback of 1080p @ 60 Hz H.264 video. Based on ETW sample profile the overhead of the message pump is reduced from 15% to 7%. The overhead is calculated as a number of inclusive samples in DoRunLoop that are not part of DoWork and DoDelayedWork. The change eliminates some system calls that shouldn't be needed in the context of GPU process such as GetQueueStatus, SetTimer, KillTimer, and minimizes other system calls that came up high in the sampling profile - PeekMessage, MsgWaitForMultipleObjectsEx, PostMessage. While this version of MessagePump is still capable to peek and dispatch Windows messages which apparently is needed in the GPU process on Windows version above Win7, based on the usage pattern analysis, the emphasis gets shifted to handling the work (vs. handling Windows messages in MessagePumpForUI). See the bug for more details. If this work well in a limited context of the GPU process some of these ideas might later be applied to other types of MessagePump. BUG=588798 ========== to ========== Version of MessagePumpForUI optimized for GPU process. The purpose of this change is to reduce overhead of MessagePump in GPU process. According to my testing on Windows 10 Dell XPS laptop this change improved the power usage by 0.15 Wt in one test and 0.12 Wt in another. The test was a windowed playback of 1080p @ 60 Hz H.264 video. Based on ETW sample profile the overhead of the message pump is reduced from 13-15% to 7-10%. The overhead is calculated as a number of inclusive samples in DoRunLoop that are not part of DoWork and DoDelayedWork. The change eliminates some system calls that shouldn't be needed in the context of GPU process such as GetQueueStatus, SetTimer, KillTimer, and minimizes other system calls that came up high in the sampling profile - PeekMessage, MsgWaitForMultipleObjectsEx, PostMessage. While this version of MessagePump is still capable to peek and dispatch Windows messages which apparently is needed in the GPU process on Windows version above Win7, based on the usage pattern analysis, the emphasis gets shifted to handling the work (vs. handling Windows messages in MessagePumpForUI). See the bug for more details. If this work well in a limited context of the GPU process some of these ideas might later be applied to other types of MessagePump. BUG=588798 ==========
stanisc@chromium.org changed reviewers: + ananta@chromium.org, thestig@chromium.org - thakis@chromium.org
Removed Nico from the review and added thestig@ and ananta@. I've done a second round of testing that confirmed the improvement in power usage. The last iteration contains a few minor changes: - Modified WaitForWork do deal with messages that belong to another thread. - Removed MWMO_INPUTAVAILABLE argument from MsgWaitForMultipleObjectsEx. It shouldn't be needed because all messages are peeked and removed only once and it should prevent the loop from spinning when there are messages that belong to another thread.
https://codereview.chromium.org/1714263002/diff/120001/base/message_loop/mess... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/1714263002/diff/120001/base/message_loop/mess... base/message_loop/message_pump_win.cc:97: if (READY != InterlockedExchange(&work_state_, HAVE_WORK)) nit: Just write it normally with "READY" on the right side, ditto below. https://codereview.chromium.org/1714263002/diff/120001/base/message_loop/mess... base/message_loop/message_pump_win.cc:424: PeekMessage(&msg, NULL, 0, 0, PM_NOREMOVE); nullptr in new code please. https://codereview.chromium.org/1714263002/diff/120001/base/message_loop/mess... base/message_loop/message_pump_win.cc:432: MessageLoop::InitMessagePumpForUIFactory( DCHECK() the return result? https://codereview.chromium.org/1714263002/diff/120001/base/message_loop/mess... base/message_loop/message_pump_win.cc:438: return scoped_ptr<MessagePump>(new MessagePumpForGpu); Use base::WrapUnique https://codereview.chromium.org/1714263002/diff/120001/base/message_loop/mess... base/message_loop/message_pump_win.cc:450: // We have failed to insert a have-work message, so there is a chance that we Do you really want to repeat this paragraph from MessagePumpForUI::ScheduleWork() ? https://codereview.chromium.org/1714263002/diff/120001/base/message_loop/mess... base/message_loop/message_pump_win.cc:461: UMA_HISTOGRAM_ENUMERATION("Chrome.MessageLoopProblem", MESSAGE_POST_ERROR, I'm not familiar with this histogram, but do you care that this uses the same enum, or should it use a new enum to indicate this is a GPU message loop problem? https://codereview.chromium.org/1714263002/diff/120001/base/message_loop/mess... base/message_loop/message_pump_win.cc:524: while((delay = GetCurrentDelay()) != 0) { nit: space after "while" - remember to run: git cl lint https://codereview.chromium.org/1714263002/diff/120001/base/message_loop/mess... File base/message_loop/message_pump_win.h (right): https://codereview.chromium.org/1714263002/diff/120001/base/message_loop/mess... base/message_loop/message_pump_win.h:13: #include "base/memory/scoped_ptr.h" No new scoped_ptr please. use std::unique_ptr. https://codereview.chromium.org/1714263002/diff/120001/base/message_loop/mess... base/message_loop/message_pump_win.h:43: // State used with work_state_ variable. nit: work_state_ variable -> |work_state_| https://codereview.chromium.org/1714263002/diff/120001/base/message_loop/mess... base/message_loop/message_pump_win.h:167: // MessagePump methods: MessagePumpWin, actually. https://codereview.chromium.org/1714263002/diff/120001/base/message_loop/mess... base/message_loop/message_pump_win.h:168: void DoRunLoop() override; Add a blank line below to separate this from MessagePumpForGpu's private methods. https://codereview.chromium.org/1714263002/diff/120001/base/message_loop/mess... base/message_loop/message_pump_win.h:172: DWORD thread_id_; const?
Addressed CR feedback. https://codereview.chromium.org/1714263002/diff/120001/base/message_loop/mess... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/1714263002/diff/120001/base/message_loop/mess... base/message_loop/message_pump_win.cc:97: if (READY != InterlockedExchange(&work_state_, HAVE_WORK)) On 2016/04/07 21:32:26, Lei Zhang wrote: > nit: Just write it normally with "READY" on the right side, ditto below. Done. https://codereview.chromium.org/1714263002/diff/120001/base/message_loop/mess... base/message_loop/message_pump_win.cc:424: PeekMessage(&msg, NULL, 0, 0, PM_NOREMOVE); On 2016/04/07 21:32:26, Lei Zhang wrote: > nullptr in new code please. Done. https://codereview.chromium.org/1714263002/diff/120001/base/message_loop/mess... base/message_loop/message_pump_win.cc:432: MessageLoop::InitMessagePumpForUIFactory( On 2016/04/07 21:32:26, Lei Zhang wrote: > DCHECK() the return result? Done. https://codereview.chromium.org/1714263002/diff/120001/base/message_loop/mess... base/message_loop/message_pump_win.cc:438: return scoped_ptr<MessagePump>(new MessagePumpForGpu); On 2016/04/07 21:32:26, Lei Zhang wrote: > Use base::WrapUnique Done. https://codereview.chromium.org/1714263002/diff/120001/base/message_loop/mess... base/message_loop/message_pump_win.cc:450: // We have failed to insert a have-work message, so there is a chance that we On 2016/04/07 21:32:26, Lei Zhang wrote: > Do you really want to repeat this paragraph from > MessagePumpForUI::ScheduleWork() ? Replaced the comment with a short one that references the comment in MessagePumpForUI::ScheduleWork() https://codereview.chromium.org/1714263002/diff/120001/base/message_loop/mess... base/message_loop/message_pump_win.cc:461: UMA_HISTOGRAM_ENUMERATION("Chrome.MessageLoopProblem", MESSAGE_POST_ERROR, On 2016/04/07 21:32:26, Lei Zhang wrote: > I'm not familiar with this histogram, but do you care that this uses the same > enum, or should it use a new enum to indicate this is a GPU message loop > problem? Good question. I think it should be OK to reuse the same error. GPU and Browser message pumps are currently in the same bucket. I'll monitor the value and if I see a change in any direction I'll introduce a new value in a separate change. But I am glad you've pointed out at this. Based on the comment we don't really expect hitting this error. Looking at UMA stats there are actually some hits but they look to be coming from a very small number of unique users. Perhaps this is one of the reasons for GPU hangs where the loop is waiting in MsgWaitForMultipleObjectsEx. https://codereview.chromium.org/1714263002/diff/120001/base/message_loop/mess... base/message_loop/message_pump_win.cc:524: while((delay = GetCurrentDelay()) != 0) { On 2016/04/07 21:32:26, Lei Zhang wrote: > nit: space after "while" - remember to run: git cl lint Done. https://codereview.chromium.org/1714263002/diff/120001/base/message_loop/mess... File base/message_loop/message_pump_win.h (right): https://codereview.chromium.org/1714263002/diff/120001/base/message_loop/mess... base/message_loop/message_pump_win.h:13: #include "base/memory/scoped_ptr.h" On 2016/04/07 21:32:26, Lei Zhang wrote: > No new scoped_ptr please. use std::unique_ptr. Done. https://codereview.chromium.org/1714263002/diff/120001/base/message_loop/mess... base/message_loop/message_pump_win.h:43: // State used with work_state_ variable. On 2016/04/07 21:32:26, Lei Zhang wrote: > nit: work_state_ variable -> |work_state_| Done. https://codereview.chromium.org/1714263002/diff/120001/base/message_loop/mess... base/message_loop/message_pump_win.h:167: // MessagePump methods: On 2016/04/07 21:32:26, Lei Zhang wrote: > MessagePumpWin, actually. Done. https://codereview.chromium.org/1714263002/diff/120001/base/message_loop/mess... base/message_loop/message_pump_win.h:168: void DoRunLoop() override; On 2016/04/07 21:32:26, Lei Zhang wrote: > Add a blank line below to separate this from MessagePumpForGpu's private > methods. Done. https://codereview.chromium.org/1714263002/diff/120001/base/message_loop/mess... base/message_loop/message_pump_win.h:172: DWORD thread_id_; On 2016/04/07 21:32:26, Lei Zhang wrote: > const? Done.
https://codereview.chromium.org/1714263002/diff/120001/base/message_loop/mess... File base/message_loop/message_pump_win.h (right): https://codereview.chromium.org/1714263002/diff/120001/base/message_loop/mess... base/message_loop/message_pump_win.h:13: #include "base/memory/scoped_ptr.h" On 2016/04/08 01:07:12, stanisc wrote: > On 2016/04/07 21:32:26, Lei Zhang wrote: > > No new scoped_ptr please. use std::unique_ptr. > > Done. #include <memory> instead. https://codereview.chromium.org/1714263002/diff/140001/base/message_loop/mess... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/1714263002/diff/140001/base/message_loop/mess... base/message_loop/message_pump_win.cc:430: DCHECK(MessageLoop::InitMessagePumpForUIFactory( You can't put this in a DCHECK. It won't run in release mode. https://codereview.chromium.org/1714263002/diff/140001/base/message_loop/mess... base/message_loop/message_pump_win.cc:497: if (HAVE_WORK == InterlockedCompareExchange(&work_state_, READY, WORKING)) More comparsions to flip. Also line 518, 532. https://codereview.chromium.org/1714263002/diff/140001/base/message_loop/mess... base/message_loop/message_pump_win.cc:531: while (PeekMessage(&msg, nullptr, 0, 0, PM_REMOVE) != FALSE) { You can just omit " != FALSE" https://codereview.chromium.org/1714263002/diff/140001/base/message_loop/mess... base/message_loop/message_pump_win.cc:540: if (msg.hwnd == NULL && msg.message == kMsgHaveWork) { nullptr or !msg.hwnd.
Addressed the second round of CR comments. PTAL! https://codereview.chromium.org/1714263002/diff/120001/base/message_loop/mess... File base/message_loop/message_pump_win.h (right): https://codereview.chromium.org/1714263002/diff/120001/base/message_loop/mess... base/message_loop/message_pump_win.h:13: #include "base/memory/scoped_ptr.h" On 2016/04/08 19:10:21, Lei Zhang wrote: > On 2016/04/08 01:07:12, stanisc wrote: > > On 2016/04/07 21:32:26, Lei Zhang wrote: > > > No new scoped_ptr please. use std::unique_ptr. > > > > Done. > > #include <memory> instead. Done. https://codereview.chromium.org/1714263002/diff/140001/base/message_loop/mess... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/1714263002/diff/140001/base/message_loop/mess... base/message_loop/message_pump_win.cc:430: DCHECK(MessageLoop::InitMessagePumpForUIFactory( On 2016/04/08 19:10:21, Lei Zhang wrote: > You can't put this in a DCHECK. It won't run in release mode. What I was thinking! Fixed. https://codereview.chromium.org/1714263002/diff/140001/base/message_loop/mess... base/message_loop/message_pump_win.cc:497: if (HAVE_WORK == InterlockedCompareExchange(&work_state_, READY, WORKING)) On 2016/04/08 19:10:21, Lei Zhang wrote: > More comparsions to flip. Also line 518, 532. Sorry, overlooked this one. Done. https://codereview.chromium.org/1714263002/diff/140001/base/message_loop/mess... base/message_loop/message_pump_win.cc:531: while (PeekMessage(&msg, nullptr, 0, 0, PM_REMOVE) != FALSE) { On 2016/04/08 19:10:21, Lei Zhang wrote: > You can just omit " != FALSE" Done. https://codereview.chromium.org/1714263002/diff/140001/base/message_loop/mess... base/message_loop/message_pump_win.cc:540: if (msg.hwnd == NULL && msg.message == kMsgHaveWork) { On 2016/04/08 19:10:21, Lei Zhang wrote: > nullptr or !msg.hwnd. Done.
Thanks for getting to all the nits, lgtm.
iyengar@google.com changed reviewers: + iyengar@google.com
https://codereview.chromium.org/1714263002/diff/200001/base/message_loop/mess... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/1714263002/diff/200001/base/message_loop/mess... base/message_loop/message_pump_win.cc:423: PeekMessage(&msg, nullptr, 0, 0, PM_NOREMOVE); Is this needed? https://codereview.chromium.org/1714263002/diff/200001/base/message_loop/mess... base/message_loop/message_pump_win.cc:475: bool more_work_is_plausible = state_->delegate->DoWork(); This function has a lot in common with the MessagePumpForUI::DoRunLoop function. Perhaps put a TODO here to refactor it out? https://codereview.chromium.org/1714263002/diff/200001/base/message_loop/mess... base/message_loop/message_pump_win.cc:533: if (msg.message == WM_QUIT) { We should be calling the message filters if any in this function. Perhaps just call ProcessMessageHelper here after your check for kMsgHaveWork?
https://codereview.chromium.org/1714263002/diff/200001/base/message_loop/mess... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/1714263002/diff/200001/base/message_loop/mess... base/message_loop/message_pump_win.cc:423: PeekMessage(&msg, nullptr, 0, 0, PM_NOREMOVE); On 2016/04/11 22:18:21, iyengar wrote: > Is this needed? Yes, this is needed to ensure that the message queue is created for the thread. Otherwise PostThreadMessage would fail. There is a comment above. https://codereview.chromium.org/1714263002/diff/200001/base/message_loop/mess... base/message_loop/message_pump_win.cc:475: bool more_work_is_plausible = state_->delegate->DoWork(); On 2016/04/11 22:18:21, iyengar wrote: > This function has a lot in common with the MessagePumpForUI::DoRunLoop function. > Perhaps put a TODO here to refactor it out? I tried an approach with a common base class - see the first iteration. It was suggested to me to keep this completely separate from MessagePumpForUI for now. I hope to refactor later. https://codereview.chromium.org/1714263002/diff/200001/base/message_loop/mess... base/message_loop/message_pump_win.cc:533: if (msg.message == WM_QUIT) { On 2016/04/11 22:18:21, iyengar wrote: > We should be calling the message filters if any in this function. Perhaps just > call ProcessMessageHelper here after your check for kMsgHaveWork? Do you think calling CallMsgFilter would do any good in GPU process? From CallMsgFilter documentation: Passes the specified message and hook code to the hook procedures associated with the WH_SYSMSGFILTER and WH_MSGFILTER hooks. A WH_SYSMSGFILTER or WH_MSGFILTER hook procedure is an application-defined callback function that examines and, optionally, modifies messages for a dialog box, message box, menu, or scroll bar.
sg https://codereview.chromium.org/1714263002/diff/200001/base/message_loop/mess... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/1714263002/diff/200001/base/message_loop/mess... base/message_loop/message_pump_win.cc:533: if (msg.message == WM_QUIT) { On 2016/04/11 23:57:32, stanisc wrote: > On 2016/04/11 22:18:21, iyengar wrote: > > We should be calling the message filters if any in this function. Perhaps just > > call ProcessMessageHelper here after your check for kMsgHaveWork? > > Do you think calling CallMsgFilter would do any good in GPU process? > > From CallMsgFilter documentation: > Passes the specified message and hook code to the hook procedures associated > with the WH_SYSMSGFILTER and WH_MSGFILTER hooks. A WH_SYSMSGFILTER or > WH_MSGFILTER hook procedure is an application-defined callback function that > examines and, optionally, modifies messages for a dialog box, message box, menu, > or scroll bar. You never know who could inject into your process. Perhaps a global hook, etc. In any case the common convention is if you have a message loop you typically do this. I think calling ProcessMessageHelper here might be better.
Added a call to the message filters and did another round of testing. https://codereview.chromium.org/1714263002/diff/200001/base/message_loop/mess... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/1714263002/diff/200001/base/message_loop/mess... base/message_loop/message_pump_win.cc:533: if (msg.message == WM_QUIT) { On 2016/04/12 00:00:47, ananta wrote: > On 2016/04/11 23:57:32, stanisc wrote: > > On 2016/04/11 22:18:21, iyengar wrote: > > > We should be calling the message filters if any in this function. Perhaps > just > > > call ProcessMessageHelper here after your check for kMsgHaveWork? > > > > Do you think calling CallMsgFilter would do any good in GPU process? > > > > From CallMsgFilter documentation: > > Passes the specified message and hook code to the hook procedures associated > > with the WH_SYSMSGFILTER and WH_MSGFILTER hooks. A WH_SYSMSGFILTER or > > WH_MSGFILTER hook procedure is an application-defined callback function that > > examines and, optionally, modifies messages for a dialog box, message box, > menu, > > or scroll bar. > > You never know who could inject into your process. Perhaps a global hook, etc. > In any case the common convention is if you have a message loop you typically do > this. I think calling ProcessMessageHelper here might be better. OK, I've added a call to CallMsgFilter below. I didn't use ProcessMessageHelper for two reasons: 1) It isn't shared with this class 2) The implementation is slightly different. msg.hwnd is expected to be nullptr in this version vs. a specific window in MessagePumpForUI.
The CQ bit was checked by stanisc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbauman@chromium.org, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/1714263002/#ps220001 (title: "Added CallMsgFilter call")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1714263002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1714263002/220001
lgtm
Message was sent while issue was closed.
Description was changed from ========== Version of MessagePumpForUI optimized for GPU process. The purpose of this change is to reduce overhead of MessagePump in GPU process. According to my testing on Windows 10 Dell XPS laptop this change improved the power usage by 0.15 Wt in one test and 0.12 Wt in another. The test was a windowed playback of 1080p @ 60 Hz H.264 video. Based on ETW sample profile the overhead of the message pump is reduced from 13-15% to 7-10%. The overhead is calculated as a number of inclusive samples in DoRunLoop that are not part of DoWork and DoDelayedWork. The change eliminates some system calls that shouldn't be needed in the context of GPU process such as GetQueueStatus, SetTimer, KillTimer, and minimizes other system calls that came up high in the sampling profile - PeekMessage, MsgWaitForMultipleObjectsEx, PostMessage. While this version of MessagePump is still capable to peek and dispatch Windows messages which apparently is needed in the GPU process on Windows version above Win7, based on the usage pattern analysis, the emphasis gets shifted to handling the work (vs. handling Windows messages in MessagePumpForUI). See the bug for more details. If this work well in a limited context of the GPU process some of these ideas might later be applied to other types of MessagePump. BUG=588798 ========== to ========== Version of MessagePumpForUI optimized for GPU process. The purpose of this change is to reduce overhead of MessagePump in GPU process. According to my testing on Windows 10 Dell XPS laptop this change improved the power usage by 0.15 Wt in one test and 0.12 Wt in another. The test was a windowed playback of 1080p @ 60 Hz H.264 video. Based on ETW sample profile the overhead of the message pump is reduced from 13-15% to 7-10%. The overhead is calculated as a number of inclusive samples in DoRunLoop that are not part of DoWork and DoDelayedWork. The change eliminates some system calls that shouldn't be needed in the context of GPU process such as GetQueueStatus, SetTimer, KillTimer, and minimizes other system calls that came up high in the sampling profile - PeekMessage, MsgWaitForMultipleObjectsEx, PostMessage. While this version of MessagePump is still capable to peek and dispatch Windows messages which apparently is needed in the GPU process on Windows version above Win7, based on the usage pattern analysis, the emphasis gets shifted to handling the work (vs. handling Windows messages in MessagePumpForUI). See the bug for more details. If this work well in a limited context of the GPU process some of these ideas might later be applied to other types of MessagePump. BUG=588798 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Version of MessagePumpForUI optimized for GPU process. The purpose of this change is to reduce overhead of MessagePump in GPU process. According to my testing on Windows 10 Dell XPS laptop this change improved the power usage by 0.15 Wt in one test and 0.12 Wt in another. The test was a windowed playback of 1080p @ 60 Hz H.264 video. Based on ETW sample profile the overhead of the message pump is reduced from 13-15% to 7-10%. The overhead is calculated as a number of inclusive samples in DoRunLoop that are not part of DoWork and DoDelayedWork. The change eliminates some system calls that shouldn't be needed in the context of GPU process such as GetQueueStatus, SetTimer, KillTimer, and minimizes other system calls that came up high in the sampling profile - PeekMessage, MsgWaitForMultipleObjectsEx, PostMessage. While this version of MessagePump is still capable to peek and dispatch Windows messages which apparently is needed in the GPU process on Windows version above Win7, based on the usage pattern analysis, the emphasis gets shifted to handling the work (vs. handling Windows messages in MessagePumpForUI). See the bug for more details. If this work well in a limited context of the GPU process some of these ideas might later be applied to other types of MessagePump. BUG=588798 ========== to ========== Version of MessagePumpForUI optimized for GPU process. The purpose of this change is to reduce overhead of MessagePump in GPU process. According to my testing on Windows 10 Dell XPS laptop this change improved the power usage by 0.15 Wt in one test and 0.12 Wt in another. The test was a windowed playback of 1080p @ 60 Hz H.264 video. Based on ETW sample profile the overhead of the message pump is reduced from 13-15% to 7-10%. The overhead is calculated as a number of inclusive samples in DoRunLoop that are not part of DoWork and DoDelayedWork. The change eliminates some system calls that shouldn't be needed in the context of GPU process such as GetQueueStatus, SetTimer, KillTimer, and minimizes other system calls that came up high in the sampling profile - PeekMessage, MsgWaitForMultipleObjectsEx, PostMessage. While this version of MessagePump is still capable to peek and dispatch Windows messages which apparently is needed in the GPU process on Windows version above Win7, based on the usage pattern analysis, the emphasis gets shifted to handling the work (vs. handling Windows messages in MessagePumpForUI). See the bug for more details. If this work well in a limited context of the GPU process some of these ideas might later be applied to other types of MessagePump. BUG=588798 Committed: https://crrev.com/f71e6d2cbb90cacc88ce41a48475c47c6b7a3543 Cr-Commit-Position: refs/heads/master@{#386898} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/f71e6d2cbb90cacc88ce41a48475c47c6b7a3543 Cr-Commit-Position: refs/heads/master@{#386898}
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:220001) has been created in https://codereview.chromium.org/1889293003/ by stanisc@chromium.org. The reason for reverting is: It looks like this is causing issues with UI responsiveness due to shared message queue.. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
