Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2701233002:
gpu: Use mojom.GpuService API for memory allocation. (Closed)
Descriptiongpu: Use mojom.GpuService API for memory allocation.
Continue to replace chrome IPC messages with corresponding mojo API used
for mus. This CL removes the messages for gpu memory [de]allocations.
BUG=643746, 630895
Review-Url: https://codereview.chromium.org/2701233002
Cr-Commit-Position: refs/heads/master@{#460253}
Committed: https://chromium.googlesource.com/chromium/src/+/16d36323ee408ce8d5f7b77ff6975718b5d6eed0
Patch Set 1 #
Total comments: 2
Patch Set 2 : tot merge #Patch Set 3 : tot merge fix #Patch Set 4 : merge io thread change. #Patch Set 5 : . #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 32 (22 generated)
The CQ bit was checked by sadrul@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 ========== gpu: Use mojom.GpuService API for memory allocation. ... BUG=643746, 630895 ========== to ========== gpu: Use mojom.GpuService API for memory allocation. Continue to replace chrome IPC messages with corresponding mojo API used for mus. This CL removes the messages for gpu memory [de]allocations. BUG=643746, 630895 ==========
sadrul@chromium.org changed reviewers: + jbauman@chromium.org, tsepez@chromium.org
+tsepez@ for gpu_host_messages.h jbauman@ for all of it.
lgtm
https://codereview.chromium.org/2701233002/diff/1/content/gpu/gpu_child_threa... File content/gpu/gpu_child_thread.cc (left): https://codereview.chromium.org/2701233002/diff/1/content/gpu/gpu_child_threa... content/gpu/gpu_child_thread.cc:79: // on the IO thread. This allows the UI thread in the browser process to remain I think we want to continue to be able to create the gpu memory buffers on the IO thread. Would it work to make a new channel just for these messages and process them there?
https://codereview.chromium.org/2701233002/diff/1/content/gpu/gpu_child_threa... File content/gpu/gpu_child_thread.cc (left): https://codereview.chromium.org/2701233002/diff/1/content/gpu/gpu_child_threa... content/gpu/gpu_child_thread.cc:79: // on the IO thread. This allows the UI thread in the browser process to remain On 2017/02/22 02:49:45, jbauman wrote: > I think we want to continue to be able to create the gpu memory buffers on the > IO thread. Would it work to make a new channel just for these messages and > process them there? Having the allocation messages come to a different channel/message-pipe would essentially expose this detail to the client (i.e. the gpu host will need to explicitly know about this, and do things differently), which doesn't feel great. Alternatively, it would be possible to set things up so that all incoming mojo messages are received on the IO thread first. I could do that, and handle the memory-allocation messages on IO thread, but post-task the rest of the messages to the 'main' thread. WDYT? ... although, looking back in history, the CL that introduced this filter [1] I think did so because otherwise, the filter was being set-up too late, and some of the initial allocation-requests were going unhandled. That should not happen with the mojo API, since mojo takes care of queueing the messages appropriately. So perhaps handling the messages on the IO thread isn't quite essential? 1: https://codereview.chromium.org/1210703002
On 2017/02/22 03:08:51, sadrul (OOO) wrote: > https://codereview.chromium.org/2701233002/diff/1/content/gpu/gpu_child_threa... > File content/gpu/gpu_child_thread.cc (left): > > https://codereview.chromium.org/2701233002/diff/1/content/gpu/gpu_child_threa... > content/gpu/gpu_child_thread.cc:79: // on the IO thread. This allows the UI > thread in the browser process to remain > On 2017/02/22 02:49:45, jbauman wrote: > > I think we want to continue to be able to create the gpu memory buffers on the > > IO thread. Would it work to make a new channel just for these messages and > > process them there? > > Having the allocation messages come to a different channel/message-pipe would > essentially expose this detail to the client (i.e. the gpu host will need to > explicitly know about this, and do things differently), which doesn't feel > great. > > Alternatively, it would be possible to set things up so that all incoming mojo > messages are received on the IO thread first. I could do that, and handle the > memory-allocation messages on IO thread, but post-task the rest of the messages > to the 'main' thread. WDYT? I think that could work, but it seems a bit painful to reinvent the MessageFilter for this. > > ... although, looking back in history, the CL that introduced this filter [1] I > think did so because otherwise, the filter was being set-up too late, and some > of the initial allocation-requests were going unhandled. That should not happen > with the mojo API, since mojo takes care of queueing the messages appropriately. > So perhaps handling the messages on the IO thread isn't quite essential? That just moved the code from content/common/gpu/gpu_channel_manager.cc. > > 1: https://codereview.chromium.org/1210703002
On 2017/02/22 03:14:03, jbauman wrote: > On 2017/02/22 03:08:51, sadrul (OOO) wrote: > > > https://codereview.chromium.org/2701233002/diff/1/content/gpu/gpu_child_threa... > > File content/gpu/gpu_child_thread.cc (left): > > > > > https://codereview.chromium.org/2701233002/diff/1/content/gpu/gpu_child_threa... > > content/gpu/gpu_child_thread.cc:79: // on the IO thread. This allows the UI > > thread in the browser process to remain > > On 2017/02/22 02:49:45, jbauman wrote: > > > I think we want to continue to be able to create the gpu memory buffers on > the > > > IO thread. Would it work to make a new channel just for these messages and > > > process them there? > > > > Having the allocation messages come to a different channel/message-pipe would > > essentially expose this detail to the client (i.e. the gpu host will need to > > explicitly know about this, and do things differently), which doesn't feel > > great. > > > > Alternatively, it would be possible to set things up so that all incoming mojo > > messages are received on the IO thread first. I could do that, and handle the > > memory-allocation messages on IO thread, but post-task the rest of the > messages > > to the 'main' thread. WDYT? > > I think that could work, but it seems a bit painful to reinvent the > MessageFilter for this. I am working on fleshing out more of the browser<-->gpu communication, to get a better feel of how much complexity we would need to introduce for the thread-hopping. If it turns out we need to do the thread-hopping too often for too many messages, then it might make sense to look for a more generic approach (like MessageFilters, maybe). So for now, I am going to pause work on this particular CL, and instead keep converting other chrome-ipc messages (e.g. https://codereview.chromium.org/2717233003/)
The CQ bit was checked by sadrul@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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by sadrul@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 checked by sadrul@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 checked by sadrul@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...
I have updated this CL. PTAL (the change to bind the GpuServifeRequest on the IO thread is done in https://codereview.chromium.org/2779523004/, and this CL depends on that)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by sadrul@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org Link to the patchset: https://codereview.chromium.org/2701233002/#ps80001 (title: ".")
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": 1490747245843490,
"parent_rev": "fc921013f0a8420393406ba8208321fb4c60229f", "commit_rev":
"16d36323ee408ce8d5f7b77ff6975718b5d6eed0"}
Message was sent while issue was closed.
Description was changed from ========== gpu: Use mojom.GpuService API for memory allocation. Continue to replace chrome IPC messages with corresponding mojo API used for mus. This CL removes the messages for gpu memory [de]allocations. BUG=643746, 630895 ========== to ========== gpu: Use mojom.GpuService API for memory allocation. Continue to replace chrome IPC messages with corresponding mojo API used for mus. This CL removes the messages for gpu memory [de]allocations. BUG=643746, 630895 Review-Url: https://codereview.chromium.org/2701233002 Cr-Commit-Position: refs/heads/master@{#460253} Committed: https://chromium.googlesource.com/chromium/src/+/16d36323ee408ce8d5f7b77ff697... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/16d36323ee408ce8d5f7b77ff697... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
