Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2772513005:
gpu: Move out of order / control message handling to message filter. (Closed)
Descriptiongpu: Move out of order message handling from message queue.
This code is common to the scheduler and old (message queue) code paths
so move it to the gpu channel which is used in both cases.
R=piman
BUG=514813
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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
Review-Url: https://codereview.chromium.org/2772513005
Cr-Commit-Position: refs/heads/master@{#460239}
Committed: https://chromium.googlesource.com/chromium/src/+/546b3e91945b120a9540fc1bf9b53719ad9e64b5
Patch Set 1 #
Total comments: 9
Patch Set 2 : piman's review #Patch Set 3 : handle control messages like out of order messages #Patch Set 4 : move post task to message filter #Patch Set 5 : better dchecks #Messages
Total messages: 34 (25 generated)
Description was changed from ========== gpu: Move out of order / control message handling to message filter. This code is common to the scheduler and old (message queue) code paths so move it to the message filter which is used in both cases. R=piman BUG=514813 ========== to ========== gpu: Move out of order / control message handling to message filter. This code is common to the scheduler and old (message queue) code paths so move it to the message filter which is used in both cases. R=piman BUG=514813 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 sunnyps@chromium.org to run a CQ dry run
PTAL
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.
https://codereview.chromium.org/2772513005/diff/1/gpu/ipc/service/gpu_channel.cc File gpu/ipc/service/gpu_channel.cc (right): https://codereview.chromium.org/2772513005/diff/1/gpu/ipc/service/gpu_channel... gpu/ipc/service/gpu_channel.cc:559: message_queue_->Destroy(); This has a race, because the message queue gets destroyed while the IO thread may still receive IPCs from the client. Nothing in GpuChannelMessageQueue prevents this from happening any more. A possibility is to move the filter_->Destroy() above this line. https://codereview.chromium.org/2772513005/diff/1/gpu/ipc/service/gpu_channel... gpu/ipc/service/gpu_channel.cc:887: filter_->AddChannelFilter(filter); I don't think it's generally safe to call filters on the main thread. For example, filter->OnFilterAdded(), which is called from GpuChannelMessageFilter::AddChannelFilter is supposed to be called on the IO thread (like all other MessageFilter API). I would keep the PostTask, and only grab the lock in places that access gpu_channel_ in GpuChannelMessageFilter (Destroy() and OnMessageReceived()) https://codereview.chromium.org/2772513005/diff/1/gpu/ipc/service/gpu_channel... gpu/ipc/service/gpu_channel.cc:891: filter_->RemoveChannelFilter(filter); ditto https://codereview.chromium.org/2772513005/diff/1/gpu/ipc/service/gpu_channel.h File gpu/ipc/service/gpu_channel.h (left): https://codereview.chromium.org/2772513005/diff/1/gpu/ipc/service/gpu_channel... gpu/ipc/service/gpu_channel.h:261: // variable's destructors are executed, rendering them invalid. I think we should keep this property. Using SupportsWeakPtr is equivalent to moving weak_factory_ to be the first field, losing this property.
Description was changed from ========== gpu: Move out of order / control message handling to message filter. This code is common to the scheduler and old (message queue) code paths so move it to the message filter which is used in both cases. R=piman BUG=514813 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 ========== gpu: Move out of order message handling from message queue. This code is common to the scheduler and old (message queue) code paths so move it to the gpu channel which is used in both cases. R=piman BUG=514813 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 sunnyps@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...
ptal https://codereview.chromium.org/2772513005/diff/1/gpu/ipc/service/gpu_channel.cc File gpu/ipc/service/gpu_channel.cc (right): https://codereview.chromium.org/2772513005/diff/1/gpu/ipc/service/gpu_channel... gpu/ipc/service/gpu_channel.cc:559: message_queue_->Destroy(); On 2017/03/24 04:31:40, piman wrote: > This has a race, because the message queue gets destroyed while the IO thread > may still receive IPCs from the client. Nothing in GpuChannelMessageQueue > prevents this from happening any more. > > A possibility is to move the filter_->Destroy() above this line. That's a mistake, I meant to do filter_->Destroy first. https://codereview.chromium.org/2772513005/diff/1/gpu/ipc/service/gpu_channel... gpu/ipc/service/gpu_channel.cc:684: if (msg.routing_id() == MSG_ROUTING_CONTROL || Actually control messages should not be PostTask'd because they are sync messages and need to be replied to even if the channel is destroyed. So we always need a dedicated message queue / task queue that must be drained at the end. I added DCHECK(!msg.is_sync()) in PostHandleOutOfOrderMessage. https://codereview.chromium.org/2772513005/diff/1/gpu/ipc/service/gpu_channel... gpu/ipc/service/gpu_channel.cc:887: filter_->AddChannelFilter(filter); On 2017/03/24 04:31:40, piman wrote: > I don't think it's generally safe to call filters on the main thread. For > example, filter->OnFilterAdded(), which is called from > GpuChannelMessageFilter::AddChannelFilter is supposed to be called on the IO > thread (like all other MessageFilter API). > > I would keep the PostTask, and only grab the lock in places that access > gpu_channel_ in GpuChannelMessageFilter (Destroy() and OnMessageReceived()) Done. https://codereview.chromium.org/2772513005/diff/1/gpu/ipc/service/gpu_channel... gpu/ipc/service/gpu_channel.cc:891: filter_->RemoveChannelFilter(filter); On 2017/03/24 04:31:40, piman wrote: > ditto Done. https://codereview.chromium.org/2772513005/diff/1/gpu/ipc/service/gpu_channel.h File gpu/ipc/service/gpu_channel.h (left): https://codereview.chromium.org/2772513005/diff/1/gpu/ipc/service/gpu_channel... gpu/ipc/service/gpu_channel.h:261: // variable's destructors are executed, rendering them invalid. On 2017/03/24 04:31:40, piman wrote: > I think we should keep this property. Using SupportsWeakPtr is equivalent to > moving weak_factory_ to be the first field, losing this property. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
On 2017/03/24 21:27:22, sunnyps wrote: > ptal > > https://codereview.chromium.org/2772513005/diff/1/gpu/ipc/service/gpu_channel.cc > File gpu/ipc/service/gpu_channel.cc (right): > > https://codereview.chromium.org/2772513005/diff/1/gpu/ipc/service/gpu_channel... > gpu/ipc/service/gpu_channel.cc:559: message_queue_->Destroy(); > On 2017/03/24 04:31:40, piman wrote: > > This has a race, because the message queue gets destroyed while the IO thread > > may still receive IPCs from the client. Nothing in GpuChannelMessageQueue > > prevents this from happening any more. > > > > A possibility is to move the filter_->Destroy() above this line. > > That's a mistake, I meant to do filter_->Destroy first. > > https://codereview.chromium.org/2772513005/diff/1/gpu/ipc/service/gpu_channel... > gpu/ipc/service/gpu_channel.cc:684: if (msg.routing_id() == MSG_ROUTING_CONTROL > || > Actually control messages should not be PostTask'd because they are sync > messages and need to be replied to even if the channel is destroyed. So we > always need a dedicated message queue / task queue that must be drained at the > end. I added DCHECK(!msg.is_sync()) in PostHandleOutOfOrderMessage. WaitForToken/OffsetInRange are also sync messages. My point is moot because if a channel dies the client Send will fail. So we don't anything special on the service side in that case. > > https://codereview.chromium.org/2772513005/diff/1/gpu/ipc/service/gpu_channel... > gpu/ipc/service/gpu_channel.cc:887: filter_->AddChannelFilter(filter); > On 2017/03/24 04:31:40, piman wrote: > > I don't think it's generally safe to call filters on the main thread. For > > example, filter->OnFilterAdded(), which is called from > > GpuChannelMessageFilter::AddChannelFilter is supposed to be called on the IO > > thread (like all other MessageFilter API). > > > > I would keep the PostTask, and only grab the lock in places that access > > gpu_channel_ in GpuChannelMessageFilter (Destroy() and OnMessageReceived()) > > Done. > > https://codereview.chromium.org/2772513005/diff/1/gpu/ipc/service/gpu_channel... > gpu/ipc/service/gpu_channel.cc:891: filter_->RemoveChannelFilter(filter); > On 2017/03/24 04:31:40, piman wrote: > > ditto > > Done. > > https://codereview.chromium.org/2772513005/diff/1/gpu/ipc/service/gpu_channel.h > File gpu/ipc/service/gpu_channel.h (left): > > https://codereview.chromium.org/2772513005/diff/1/gpu/ipc/service/gpu_channel... > gpu/ipc/service/gpu_channel.h:261: // variable's destructors are executed, > rendering them invalid. > On 2017/03/24 04:31:40, piman wrote: > > I think we should keep this property. Using SupportsWeakPtr is equivalent to > > moving weak_factory_ to be the first field, losing this property. > > Done.
The CQ bit was checked by sunnyps@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: Try jobs failed on following builders: mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
LGTM, thanks!
The CQ bit was checked by sunnyps@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...
PTAL I made some small changes that made sense based on upcoming patches. In particular I think it's best that the main/IO thread shared state (like route -> sequence lookup) stays on the message filter which already has a lock for that, instead of adding a lock to GpuChannel. That means the GpuChannel::HandleMessageOnIOThread method doesn't really do much so I moved all of that to the filter, which will also post task to the scheduler.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by sunnyps@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: CQ has no permission to schedule in bucket master.tryserver.chromium.linux
lgtm
The CQ bit was checked by sunnyps@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490742635468020,
"parent_rev": "21ea0c9b1eb01f5cd463d812c6b9a759d5227239", "commit_rev":
"546b3e91945b120a9540fc1bf9b53719ad9e64b5"}
Message was sent while issue was closed.
Description was changed from ========== gpu: Move out of order message handling from message queue. This code is common to the scheduler and old (message queue) code paths so move it to the gpu channel which is used in both cases. R=piman BUG=514813 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 ========== gpu: Move out of order message handling from message queue. This code is common to the scheduler and old (message queue) code paths so move it to the gpu channel which is used in both cases. R=piman BUG=514813 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 Review-Url: https://codereview.chromium.org/2772513005 Cr-Commit-Position: refs/heads/master@{#460239} Committed: https://chromium.googlesource.com/chromium/src/+/546b3e91945b120a9540fc1bf9b5... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/546b3e91945b120a9540fc1bf9b5... |
