Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2208153002:
Create GPU child window on new thread. (Closed)
DescriptionCreate GPU child window on new thread.
On Windows 10 when processing touch events, the GPU process main thread
can block waiting for the browser UI thread to process the events. This
can cause a deadlock if the browser process has sent a synchronous IPC
to the GPU process.
We can work around this by creating all GPU process windows on separate
threads. Then the GPU process main thread won't attach input queues with
the browser process main thread, which reduce the risk of deadlock.
There's still a small risk of deadlock when the window is resized, which
causes the GPU main thread to need to wait for the window owner thread.
BUG=596190
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Committed: https://crrev.com/6b820b8ad092a59c8315c5ed942d21f723890ca1
Committed: https://crrev.com/8a13be547ac764cde127bf3262e4e45d71d63f87
Cr-Original-Commit-Position: refs/heads/master@{#410902}
Cr-Commit-Position: refs/heads/master@{#418030}
Patch Set 1 #
Total comments: 13
Patch Set 2 : review changes #Patch Set 3 : make hidden window on main thread #
Messages
Total messages: 35 (23 generated)
Description was changed from ========== Create GPU child window on new thread. BUG= ========== to ========== Create GPU child window on new thread. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was checked by jbauman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Create GPU child window on new thread. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Create GPU child window on new thread. On Windows 10 when processing touch events, the GPU process main thread can block waiting for the browser UI thread to process the events. This can cause a deadlock if the browser process has sent a synchronous IPC to the GPU process. We can work around this by creating all GPU process windows on separate threads. Then the GPU process main thread won't attach input queues with the browser process main thread, which reduce the risk of deadlock. There's still a small risk of deadlock when the window is resized, which causes the GPU main thread to need to wait for the window owner thread. BUG=596190 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
jbauman@chromium.org changed reviewers: + piman@chromium.org, stanisc@chromium.org
Looks clever! I thought this would be more complex! https://codereview.chromium.org/2208153002/diff/1/gpu/ipc/service/child_windo... File gpu/ipc/service/child_window_surface_win.cc (right): https://codereview.chromium.org/2208153002/diff/1/gpu/ipc/service/child_windo... gpu/ipc/service/child_window_surface_win.cc:57: shared_data->rect_to_clear.Union(gfx::Rect(paint.rcPaint)); Should the lock scope end after this line? https://codereview.chromium.org/2208153002/diff/1/gpu/ipc/service/child_windo... gpu/ipc/service/child_window_surface_win.cc:84: void CreateChildWindow(HWND parent, It would be good to indicate in comments which thread each method runs on. https://codereview.chromium.org/2208153002/diff/1/gpu/ipc/service/child_windo... gpu/ipc/service/child_window_surface_win.cc:86: SharedData* shared_data, Should this be const pointer? https://codereview.chromium.org/2208153002/diff/1/gpu/ipc/service/child_windo... gpu/ipc/service/child_window_surface_win.cc:111: } Add // namespace https://codereview.chromium.org/2208153002/diff/1/gpu/ipc/service/child_windo... gpu/ipc/service/child_window_surface_win.cc:250: base::win::ScopedGetDC dc(window_); Is this running on the main thread? Is it safe to get DC on a thread different than the window thread? https://codereview.chromium.org/2208153002/diff/1/gpu/ipc/service/child_windo... gpu/ipc/service/child_window_surface_win.cc:267: base::Bind(&DestroySharedData, base::Passed(std::move(shared_data_)))); Clever solution! https://codereview.chromium.org/2208153002/diff/1/gpu/ipc/service/child_windo... File gpu/ipc/service/child_window_surface_win.h (right): https://codereview.chromium.org/2208153002/diff/1/gpu/ipc/service/child_windo... gpu/ipc/service/child_window_surface_win.h:36: std::unique_ptr<SharedData> shared_data_; Since this class is multi-threaded now it would be good to indicate which thread each field is owned by and accessed from.
The CQ bit was checked by jbauman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
https://codereview.chromium.org/2208153002/diff/1/gpu/ipc/service/child_windo... File gpu/ipc/service/child_window_surface_win.cc (right): https://codereview.chromium.org/2208153002/diff/1/gpu/ipc/service/child_windo... gpu/ipc/service/child_window_surface_win.cc:57: shared_data->rect_to_clear.Union(gfx::Rect(paint.rcPaint)); On 2016/08/04 02:03:35, stanisc wrote: > Should the lock scope end after this line? Done. https://codereview.chromium.org/2208153002/diff/1/gpu/ipc/service/child_windo... gpu/ipc/service/child_window_surface_win.cc:84: void CreateChildWindow(HWND parent, On 2016/08/04 02:03:35, stanisc wrote: > It would be good to indicate in comments which thread each method runs on. Done. https://codereview.chromium.org/2208153002/diff/1/gpu/ipc/service/child_windo... gpu/ipc/service/child_window_surface_win.cc:86: SharedData* shared_data, On 2016/08/04 02:03:35, stanisc wrote: > Should this be const pointer? No, SetWindowUserData takes a non-const pointer. https://codereview.chromium.org/2208153002/diff/1/gpu/ipc/service/child_windo... gpu/ipc/service/child_window_surface_win.cc:111: } On 2016/08/04 02:03:35, stanisc wrote: > Add // namespace Done. https://codereview.chromium.org/2208153002/diff/1/gpu/ipc/service/child_windo... gpu/ipc/service/child_window_surface_win.cc:250: base::win::ScopedGetDC dc(window_); On 2016/08/04 02:03:35, stanisc wrote: > Is this running on the main thread? > Is it safe to get DC on a thread different than the window thread? Yeah, it's fine to do GetDC on any thread (or even a different process). https://codereview.chromium.org/2208153002/diff/1/gpu/ipc/service/child_windo... File gpu/ipc/service/child_window_surface_win.h (right): https://codereview.chromium.org/2208153002/diff/1/gpu/ipc/service/child_windo... gpu/ipc/service/child_window_surface_win.h:36: std::unique_ptr<SharedData> shared_data_; On 2016/08/04 02:03:35, stanisc wrote: > Since this class is multi-threaded now it would be good to indicate which thread > each field is owned by and accessed from. Ok, mentioned that shared_data_ is the only thing accessed from multiple threads.
lgtm
The CQ bit was checked by jbauman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Create GPU child window on new thread. On Windows 10 when processing touch events, the GPU process main thread can block waiting for the browser UI thread to process the events. This can cause a deadlock if the browser process has sent a synchronous IPC to the GPU process. We can work around this by creating all GPU process windows on separate threads. Then the GPU process main thread won't attach input queues with the browser process main thread, which reduce the risk of deadlock. There's still a small risk of deadlock when the window is resized, which causes the GPU main thread to need to wait for the window owner thread. BUG=596190 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Create GPU child window on new thread. On Windows 10 when processing touch events, the GPU process main thread can block waiting for the browser UI thread to process the events. This can cause a deadlock if the browser process has sent a synchronous IPC to the GPU process. We can work around this by creating all GPU process windows on separate threads. Then the GPU process main thread won't attach input queues with the browser process main thread, which reduce the risk of deadlock. There's still a small risk of deadlock when the window is resized, which causes the GPU main thread to need to wait for the window owner thread. BUG=596190 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Create GPU child window on new thread. On Windows 10 when processing touch events, the GPU process main thread can block waiting for the browser UI thread to process the events. This can cause a deadlock if the browser process has sent a synchronous IPC to the GPU process. We can work around this by creating all GPU process windows on separate threads. Then the GPU process main thread won't attach input queues with the browser process main thread, which reduce the risk of deadlock. There's still a small risk of deadlock when the window is resized, which causes the GPU main thread to need to wait for the window owner thread. BUG=596190 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Create GPU child window on new thread. On Windows 10 when processing touch events, the GPU process main thread can block waiting for the browser UI thread to process the events. This can cause a deadlock if the browser process has sent a synchronous IPC to the GPU process. We can work around this by creating all GPU process windows on separate threads. Then the GPU process main thread won't attach input queues with the browser process main thread, which reduce the risk of deadlock. There's still a small risk of deadlock when the window is resized, which causes the GPU main thread to need to wait for the window owner thread. BUG=596190 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/6b820b8ad092a59c8315c5ed942d21f723890ca1 Cr-Commit-Position: refs/heads/master@{#410902} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/6b820b8ad092a59c8315c5ed942d21f723890ca1 Cr-Commit-Position: refs/heads/master@{#410902}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2230683004/ by jbauman@chromium.org. The reason for reverting is: This is most likely causing the window received from the GPU process to be invalid. BUG=612300.
Message was sent while issue was closed.
Description was changed from ========== Create GPU child window on new thread. On Windows 10 when processing touch events, the GPU process main thread can block waiting for the browser UI thread to process the events. This can cause a deadlock if the browser process has sent a synchronous IPC to the GPU process. We can work around this by creating all GPU process windows on separate threads. Then the GPU process main thread won't attach input queues with the browser process main thread, which reduce the risk of deadlock. There's still a small risk of deadlock when the window is resized, which causes the GPU main thread to need to wait for the window owner thread. BUG=596190 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/6b820b8ad092a59c8315c5ed942d21f723890ca1 Cr-Commit-Position: refs/heads/master@{#410902} ========== to ========== Create GPU child window on new thread. On Windows 10 when processing touch events, the GPU process main thread can block waiting for the browser UI thread to process the events. This can cause a deadlock if the browser process has sent a synchronous IPC to the GPU process. We can work around this by creating all GPU process windows on separate threads. Then the GPU process main thread won't attach input queues with the browser process main thread, which reduce the risk of deadlock. There's still a small risk of deadlock when the window is resized, which causes the GPU main thread to need to wait for the window owner thread. BUG=596190 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/6b820b8ad092a59c8315c5ed942d21f723890ca1 Cr-Commit-Position: refs/heads/master@{#410902} ==========
The CQ bit was checked by jbauman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jbauman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stanisc@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/2208153002/#ps40001 (title: "make hidden window on main thread")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Create GPU child window on new thread. On Windows 10 when processing touch events, the GPU process main thread can block waiting for the browser UI thread to process the events. This can cause a deadlock if the browser process has sent a synchronous IPC to the GPU process. We can work around this by creating all GPU process windows on separate threads. Then the GPU process main thread won't attach input queues with the browser process main thread, which reduce the risk of deadlock. There's still a small risk of deadlock when the window is resized, which causes the GPU main thread to need to wait for the window owner thread. BUG=596190 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/6b820b8ad092a59c8315c5ed942d21f723890ca1 Cr-Commit-Position: refs/heads/master@{#410902} ========== to ========== Create GPU child window on new thread. On Windows 10 when processing touch events, the GPU process main thread can block waiting for the browser UI thread to process the events. This can cause a deadlock if the browser process has sent a synchronous IPC to the GPU process. We can work around this by creating all GPU process windows on separate threads. Then the GPU process main thread won't attach input queues with the browser process main thread, which reduce the risk of deadlock. There's still a small risk of deadlock when the window is resized, which causes the GPU main thread to need to wait for the window owner thread. BUG=596190 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/6b820b8ad092a59c8315c5ed942d21f723890ca1 Cr-Commit-Position: refs/heads/master@{#410902} ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Create GPU child window on new thread. On Windows 10 when processing touch events, the GPU process main thread can block waiting for the browser UI thread to process the events. This can cause a deadlock if the browser process has sent a synchronous IPC to the GPU process. We can work around this by creating all GPU process windows on separate threads. Then the GPU process main thread won't attach input queues with the browser process main thread, which reduce the risk of deadlock. There's still a small risk of deadlock when the window is resized, which causes the GPU main thread to need to wait for the window owner thread. BUG=596190 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/6b820b8ad092a59c8315c5ed942d21f723890ca1 Cr-Commit-Position: refs/heads/master@{#410902} ========== to ========== Create GPU child window on new thread. On Windows 10 when processing touch events, the GPU process main thread can block waiting for the browser UI thread to process the events. This can cause a deadlock if the browser process has sent a synchronous IPC to the GPU process. We can work around this by creating all GPU process windows on separate threads. Then the GPU process main thread won't attach input queues with the browser process main thread, which reduce the risk of deadlock. There's still a small risk of deadlock when the window is resized, which causes the GPU main thread to need to wait for the window owner thread. BUG=596190 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/6b820b8ad092a59c8315c5ed942d21f723890ca1 Committed: https://crrev.com/8a13be547ac764cde127bf3262e4e45d71d63f87 Cr-Original-Commit-Position: refs/heads/master@{#410902} Cr-Commit-Position: refs/heads/master@{#418030} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8a13be547ac764cde127bf3262e4e45d71d63f87 Cr-Commit-Position: refs/heads/master@{#418030} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
