Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1943063002:
Version of MessagePumpForUI optimized for GPU (Closed)
|
Created:
4 years, 7 months ago by stanisc Modified:
4 years, 6 months ago CC:
chromium-reviews, jam, 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. |
DescriptionVersion of MessagePumpForUI optimized for GPU
This is re-landing of https://codereview.chromium.org/1714263002.
The original patch was reverted due to responsiveness issues with the
browser main window on Window 10 during high GPU load. It turned out
not pumping Windows messages promptly enough in the GPU process resulted
in blocking mouse messages in the browser process due to the
shared nature of the underlying message queue (and the fact that DXVA
window is a disabled child of the main browser window covering its
entire client area).
The original patch is patch-set 1 and patch-set 2 shows the revised
version. There are two main differences:
- Message Pumping is now done on every loop iteration just like in
MessagePumpForUI.
- At the same time I moved away from using a work message to signal
the consumer thread and used an event instead. This should mostly
eliminate the message traffic in the main GPU thread message queue
and hopefully reduce interference between the browser main thread
and the GPU main thread.
See the original change for the description of the efficiency
improvements. The secondary goal for this change is to see whether
stopping using work messages altogether would affect the rate
of GPU hangs (crbug/596190). Hangs in KillTimer account
for about 2-2.5% of all GPU hangs. I hope that eliminating
any timer related code altogether might help to reduce
the rate. The previously used signalling mechanism based
on PostMessage could fail as well when the message queue
was overfilled. The new mechanism based on SetEvent
should be more reliable.
BUG=588798
Committed: https://crrev.com/100d03d7637f9ec1f31ae012a7712a44d195c483
Cr-Commit-Position: refs/heads/master@{#396240}
Patch Set 1 : The original patch #Patch Set 2 : Changes to the original patch #
Total comments: 2
Patch Set 3 : Addressed CR feedback #
Total comments: 2
Patch Set 4 : Merged with recent changes #Patch Set 5 : Addressed CR feedback #
Messages
Total messages: 26 (12 generated)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Description was changed from ========== Applied MessagePump patch BUG= ========== to ========== Version of MessagePumpForUI optimized for GPU This is re-landing of https://codereview.chromium.org/1714263002. The original patch was reverted due to responsiveness issues with the browser main window on Window 10 during high GPU load. It turned out not pumping Windows messages promptly enough in the GPU process resulted in blocking mouse messages in the browser process due to the shared nature of the underlying message queue (and the fact that DXVA window is a disabled child of the main browser window covering its entire client area). The original patch is patch-set 1 and patch-set 2 shows the revised version. There are two main differences: - Message Pumping is now done on every loop iteration just like in MessagePumpForUI. - At the same time I moved away from using a work message to signal the consumer thread and used an event instead. This should mostly eliminate the message traffic in the main GPU thread message queue and hopefully reduce interference between the browser main thread and the GPU main thread. See the original change for the description of the efficiency improvements. The secondary goal for this change is to see whether stopping using work messages altogether would affect the rate of GPU hangs (crbug/596190). BUG=588798 ==========
Description was changed from ========== Version of MessagePumpForUI optimized for GPU This is re-landing of https://codereview.chromium.org/1714263002. The original patch was reverted due to responsiveness issues with the browser main window on Window 10 during high GPU load. It turned out not pumping Windows messages promptly enough in the GPU process resulted in blocking mouse messages in the browser process due to the shared nature of the underlying message queue (and the fact that DXVA window is a disabled child of the main browser window covering its entire client area). The original patch is patch-set 1 and patch-set 2 shows the revised version. There are two main differences: - Message Pumping is now done on every loop iteration just like in MessagePumpForUI. - At the same time I moved away from using a work message to signal the consumer thread and used an event instead. This should mostly eliminate the message traffic in the main GPU thread message queue and hopefully reduce interference between the browser main thread and the GPU main thread. See the original change for the description of the efficiency improvements. The secondary goal for this change is to see whether stopping using work messages altogether would affect the rate of GPU hangs (crbug/596190). BUG=588798 ========== to ========== Version of MessagePumpForUI optimized for GPU This is re-landing of https://codereview.chromium.org/1714263002. The original patch was reverted due to responsiveness issues with the browser main window on Window 10 during high GPU load. It turned out not pumping Windows messages promptly enough in the GPU process resulted in blocking mouse messages in the browser process due to the shared nature of the underlying message queue (and the fact that DXVA window is a disabled child of the main browser window covering its entire client area). The original patch is patch-set 1 and patch-set 2 shows the revised version. There are two main differences: - Message Pumping is now done on every loop iteration just like in MessagePumpForUI. - At the same time I moved away from using a work message to signal the consumer thread and used an event instead. This should mostly eliminate the message traffic in the main GPU thread message queue and hopefully reduce interference between the browser main thread and the GPU main thread. See the original change for the description of the efficiency improvements. The secondary goal for this change is to see whether stopping using work messages altogether would affect the rate of GPU hangs (crbug/596190). BUG=588798 ==========
stanisc@chromium.org changed reviewers: + jbauman@chromium.org, thakis@chromium.org
jbauman@chromium.org: Please review changes in content/gpu thakis@chromium.org: Please review changes in base
https://codereview.chromium.org/1943063002/diff/60001/base/message_loop/messa... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/1943063002/diff/60001/base/message_loop/messa... base/message_loop/message_pump_win.cc:539: while (PeekMessage(&msg, nullptr, 0, 0, PM_REMOVE)) { Actually, this could lead to starvation if windows messages keep getting posted. This function should probably return a "more_work_is_plausible" boolean, just like DoWork. It would try to process only one message, and return true if it succeeded.
https://codereview.chromium.org/1943063002/diff/60001/base/message_loop/messa... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/1943063002/diff/60001/base/message_loop/messa... base/message_loop/message_pump_win.cc:539: while (PeekMessage(&msg, nullptr, 0, 0, PM_REMOVE)) { On 2016/05/03 22:38:33, jbauman wrote: > Actually, this could lead to starvation if windows messages keep getting posted. > This function should probably return a "more_work_is_plausible" boolean, just > like DoWork. It would try to process only one message, and return true if it > succeeded. Thanks for the comment! I thought whether to process all messages at once or one at the time. With work messages removed PeekMessage would return true only if there are messages posted for the D3D window which is disabled. I played with Spy++ and it seems it gets very few messages - only messages related to painting and resizing. But I see your point. Perhaps with the window size getting continuously changed this might be somewhat an issue. I'll change this.
I've addressed John's concern regarding GPU work starvation and have re-done power testing on Windows 10 laptop. This version is still better than MessagePumpForUI in 1080p @ 60Hz mp4 video playback scenario but only by 0.1 Wt vs. 0.15 Wt I measured with the the original version. It also seems to be a bit more power efficient in WebGL Aquarium test. I don't know if such a small win is worth the switch. Also I am hoping that getting away from posting work messages would make it clear if that has anything to do with GPU hangs. Let me know what you think!
Ping.
lgtm On 2016/05/04 21:30:21, stanisc wrote: > I've addressed John's concern regarding GPU work starvation and have re-done > power testing on Windows 10 laptop. This version is still better than > MessagePumpForUI in 1080p @ 60Hz mp4 video playback scenario but only by 0.1 Wt > vs. 0.15 Wt I measured with the the original version. It also seems to be a bit > more power efficient in WebGL Aquarium test. > > I don't know if such a small win is worth the switch. Also I am hoping that > getting away from posting work messages would make it clear if that has anything > to do with GPU hangs. Let me know what you think! Hard to know, but I do like the extra simplicity here. Hopefully it'll help us understand the GPU hangs.
Description was changed from ========== Version of MessagePumpForUI optimized for GPU This is re-landing of https://codereview.chromium.org/1714263002. The original patch was reverted due to responsiveness issues with the browser main window on Window 10 during high GPU load. It turned out not pumping Windows messages promptly enough in the GPU process resulted in blocking mouse messages in the browser process due to the shared nature of the underlying message queue (and the fact that DXVA window is a disabled child of the main browser window covering its entire client area). The original patch is patch-set 1 and patch-set 2 shows the revised version. There are two main differences: - Message Pumping is now done on every loop iteration just like in MessagePumpForUI. - At the same time I moved away from using a work message to signal the consumer thread and used an event instead. This should mostly eliminate the message traffic in the main GPU thread message queue and hopefully reduce interference between the browser main thread and the GPU main thread. See the original change for the description of the efficiency improvements. The secondary goal for this change is to see whether stopping using work messages altogether would affect the rate of GPU hangs (crbug/596190). BUG=588798 ========== to ========== Version of MessagePumpForUI optimized for GPU This is re-landing of https://codereview.chromium.org/1714263002. The original patch was reverted due to responsiveness issues with the browser main window on Window 10 during high GPU load. It turned out not pumping Windows messages promptly enough in the GPU process resulted in blocking mouse messages in the browser process due to the shared nature of the underlying message queue (and the fact that DXVA window is a disabled child of the main browser window covering its entire client area). The original patch is patch-set 1 and patch-set 2 shows the revised version. There are two main differences: - Message Pumping is now done on every loop iteration just like in MessagePumpForUI. - At the same time I moved away from using a work message to signal the consumer thread and used an event instead. This should mostly eliminate the message traffic in the main GPU thread message queue and hopefully reduce interference between the browser main thread and the GPU main thread. See the original change for the description of the efficiency improvements. The secondary goal for this change is to see whether stopping using work messages altogether would affect the rate of GPU hangs (crbug/596190). Update: added thestig@ as reviewer. BUG=588798 ==========
stanisc@chromium.org changed reviewers: + thestig@chromium.org - thakis@chromium.org
Description was changed from ========== Version of MessagePumpForUI optimized for GPU This is re-landing of https://codereview.chromium.org/1714263002. The original patch was reverted due to responsiveness issues with the browser main window on Window 10 during high GPU load. It turned out not pumping Windows messages promptly enough in the GPU process resulted in blocking mouse messages in the browser process due to the shared nature of the underlying message queue (and the fact that DXVA window is a disabled child of the main browser window covering its entire client area). The original patch is patch-set 1 and patch-set 2 shows the revised version. There are two main differences: - Message Pumping is now done on every loop iteration just like in MessagePumpForUI. - At the same time I moved away from using a work message to signal the consumer thread and used an event instead. This should mostly eliminate the message traffic in the main GPU thread message queue and hopefully reduce interference between the browser main thread and the GPU main thread. See the original change for the description of the efficiency improvements. The secondary goal for this change is to see whether stopping using work messages altogether would affect the rate of GPU hangs (crbug/596190). Update: added thestig@ as reviewer. BUG=588798 ========== to ========== Version of MessagePumpForUI optimized for GPU This is re-landing of https://codereview.chromium.org/1714263002. The original patch was reverted due to responsiveness issues with the browser main window on Window 10 during high GPU load. It turned out not pumping Windows messages promptly enough in the GPU process resulted in blocking mouse messages in the browser process due to the shared nature of the underlying message queue (and the fact that DXVA window is a disabled child of the main browser window covering its entire client area). The original patch is patch-set 1 and patch-set 2 shows the revised version. There are two main differences: - Message Pumping is now done on every loop iteration just like in MessagePumpForUI. - At the same time I moved away from using a work message to signal the consumer thread and used an event instead. This should mostly eliminate the message traffic in the main GPU thread message queue and hopefully reduce interference between the browser main thread and the GPU main thread. See the original change for the description of the efficiency improvements. The secondary goal for this change is to see whether stopping using work messages altogether would affect the rate of GPU hangs (crbug/596190). Hangs in KillTimer account for about 2-2.5% of all GPU hangs. I hope that eliminating any timer related code altogether might help to reduce the rate. The previously used signalling mechanism based on PostMessage could fail as well when the message queue was overfilled. The new mechanism based on SetEvent should be more reliable. BUG=588798 ==========
thestig@, please take a look! This is essentially the same change that you've approved several weeks ago. There are just a few changes described above. You can also compare to Patch Set 1 to see what's different compared to the original patch.
On 2016/05/25 22:35:07, stanisc wrote: > thestig@, please take a look! > > This is essentially the same change that you've approved > several weeks ago. There are just a few changes described above. You can also > compare to Patch Set 1 to see what's different compared to the original patch. Just to be sure, Patch set 3 -> 4 doesn't involve any changes from this CL, right?
https://codereview.chromium.org/1943063002/diff/80001/base/message_loop/messa... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/1943063002/diff/80001/base/message_loop/messa... base/message_loop/message_pump_win.cc:541: if (PeekMessage(&msg, nullptr, 0, 0, PM_REMOVE)) { Early return? if (!PeekMessage()) return false;
Yes, patch set 3 -> 4 doesn't have my changes (there was one merge conflict resolved). https://codereview.chromium.org/1943063002/diff/80001/base/message_loop/messa... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/1943063002/diff/80001/base/message_loop/messa... base/message_loop/message_pump_win.cc:541: if (PeekMessage(&msg, nullptr, 0, 0, PM_REMOVE)) { On 2016/05/25 22:45:16, Lei Zhang wrote: > Early return? > > if (!PeekMessage()) return false; Done.
lgtm
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 Link to the patchset: https://codereview.chromium.org/1943063002/#ps120001 (title: "Addressed CR feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1943063002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1943063002/120001
Message was sent while issue was closed.
Description was changed from ========== Version of MessagePumpForUI optimized for GPU This is re-landing of https://codereview.chromium.org/1714263002. The original patch was reverted due to responsiveness issues with the browser main window on Window 10 during high GPU load. It turned out not pumping Windows messages promptly enough in the GPU process resulted in blocking mouse messages in the browser process due to the shared nature of the underlying message queue (and the fact that DXVA window is a disabled child of the main browser window covering its entire client area). The original patch is patch-set 1 and patch-set 2 shows the revised version. There are two main differences: - Message Pumping is now done on every loop iteration just like in MessagePumpForUI. - At the same time I moved away from using a work message to signal the consumer thread and used an event instead. This should mostly eliminate the message traffic in the main GPU thread message queue and hopefully reduce interference between the browser main thread and the GPU main thread. See the original change for the description of the efficiency improvements. The secondary goal for this change is to see whether stopping using work messages altogether would affect the rate of GPU hangs (crbug/596190). Hangs in KillTimer account for about 2-2.5% of all GPU hangs. I hope that eliminating any timer related code altogether might help to reduce the rate. The previously used signalling mechanism based on PostMessage could fail as well when the message queue was overfilled. The new mechanism based on SetEvent should be more reliable. BUG=588798 ========== to ========== Version of MessagePumpForUI optimized for GPU This is re-landing of https://codereview.chromium.org/1714263002. The original patch was reverted due to responsiveness issues with the browser main window on Window 10 during high GPU load. It turned out not pumping Windows messages promptly enough in the GPU process resulted in blocking mouse messages in the browser process due to the shared nature of the underlying message queue (and the fact that DXVA window is a disabled child of the main browser window covering its entire client area). The original patch is patch-set 1 and patch-set 2 shows the revised version. There are two main differences: - Message Pumping is now done on every loop iteration just like in MessagePumpForUI. - At the same time I moved away from using a work message to signal the consumer thread and used an event instead. This should mostly eliminate the message traffic in the main GPU thread message queue and hopefully reduce interference between the browser main thread and the GPU main thread. See the original change for the description of the efficiency improvements. The secondary goal for this change is to see whether stopping using work messages altogether would affect the rate of GPU hangs (crbug/596190). Hangs in KillTimer account for about 2-2.5% of all GPU hangs. I hope that eliminating any timer related code altogether might help to reduce the rate. The previously used signalling mechanism based on PostMessage could fail as well when the message queue was overfilled. The new mechanism based on SetEvent should be more reliable. BUG=588798 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Version of MessagePumpForUI optimized for GPU This is re-landing of https://codereview.chromium.org/1714263002. The original patch was reverted due to responsiveness issues with the browser main window on Window 10 during high GPU load. It turned out not pumping Windows messages promptly enough in the GPU process resulted in blocking mouse messages in the browser process due to the shared nature of the underlying message queue (and the fact that DXVA window is a disabled child of the main browser window covering its entire client area). The original patch is patch-set 1 and patch-set 2 shows the revised version. There are two main differences: - Message Pumping is now done on every loop iteration just like in MessagePumpForUI. - At the same time I moved away from using a work message to signal the consumer thread and used an event instead. This should mostly eliminate the message traffic in the main GPU thread message queue and hopefully reduce interference between the browser main thread and the GPU main thread. See the original change for the description of the efficiency improvements. The secondary goal for this change is to see whether stopping using work messages altogether would affect the rate of GPU hangs (crbug/596190). Hangs in KillTimer account for about 2-2.5% of all GPU hangs. I hope that eliminating any timer related code altogether might help to reduce the rate. The previously used signalling mechanism based on PostMessage could fail as well when the message queue was overfilled. The new mechanism based on SetEvent should be more reliable. BUG=588798 ========== to ========== Version of MessagePumpForUI optimized for GPU This is re-landing of https://codereview.chromium.org/1714263002. The original patch was reverted due to responsiveness issues with the browser main window on Window 10 during high GPU load. It turned out not pumping Windows messages promptly enough in the GPU process resulted in blocking mouse messages in the browser process due to the shared nature of the underlying message queue (and the fact that DXVA window is a disabled child of the main browser window covering its entire client area). The original patch is patch-set 1 and patch-set 2 shows the revised version. There are two main differences: - Message Pumping is now done on every loop iteration just like in MessagePumpForUI. - At the same time I moved away from using a work message to signal the consumer thread and used an event instead. This should mostly eliminate the message traffic in the main GPU thread message queue and hopefully reduce interference between the browser main thread and the GPU main thread. See the original change for the description of the efficiency improvements. The secondary goal for this change is to see whether stopping using work messages altogether would affect the rate of GPU hangs (crbug/596190). Hangs in KillTimer account for about 2-2.5% of all GPU hangs. I hope that eliminating any timer related code altogether might help to reduce the rate. The previously used signalling mechanism based on PostMessage could fail as well when the message queue was overfilled. The new mechanism based on SetEvent should be more reliable. BUG=588798 Committed: https://crrev.com/100d03d7637f9ec1f31ae012a7712a44d195c483 Cr-Commit-Position: refs/heads/master@{#396240} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/100d03d7637f9ec1f31ae012a7712a44d195c483 Cr-Commit-Position: refs/heads/master@{#396240} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
