Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1268383003:
Sync compositor: Ignore command buffer memory callback (Closed)
|
Created:
5 years, 4 months ago by boliu Modified:
5 years, 4 months ago Reviewers:
no sievers CC:
chromium-reviews, jam, sievers+watch_chromium.org, jbauman+watch_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSync compositor: Ignore command buffer memory callback
Synchronous compositor client directly controls memory allocation. After
switching to ipc-based command buffer, this conflicts the memory
allocation callbacks from the command buffer.
Note in-process command buffer never supposed memory callbacks.
BUG=509702
Committed: https://crrev.com/ec1476c26c6602c80cc8131cd507c250f6150652
Cr-Commit-Position: refs/heads/master@{#342267}
Patch Set 1 #
Total comments: 2
Patch Set 2 : remove override #
Total comments: 3
Patch Set 3 : review #
Total comments: 1
Patch Set 4 : don't touch remove #
Messages
Total messages: 39 (12 generated)
boliu@chromium.org changed reviewers: + sievers@chromium.org
ptal Lease hacky solution I thought of..
Can we override the policy either further down, where it originates, or further up (in the OutputSurface or so), rather than just cutting a callback in the middle? Esp. I think you can already override SetMemoryPolcyChangedCallback (which you are doing), so I assume you are only not registering the WGC3D callback as an optimization? (In which case if you are doing this micro-optimization let's fully disable the code.)
On 2015/08/05 20:23:06, sievers wrote: > Can we override the policy either further down, where it originates, or further > up (in the OutputSurface or so), rather than just cutting a callback in the > middle? Esp. I think you can already override SetMemoryPolcyChangedCallback > (which you are doing), so I assume you are only not registering the WGC3D > callback as an optimization? (In which case if you are doing this > micro-optimization let's fully disable the code.) Actually this is sort of the "future down" already. Not sending GpuCommandBufferMsg_SetClientHasMemoryAllocationChangedCallback disables all the work. I guess the only other thing is to not create the GpuMemoryManager at all, and DCHECK it's there if it's needed. Worth it?
On 2015/08/05 20:31:56, boliu wrote: > On 2015/08/05 20:23:06, sievers wrote: > > Can we override the policy either further down, where it originates, or > further > > up (in the OutputSurface or so), rather than just cutting a callback in the > > middle? Esp. I think you can already override SetMemoryPolcyChangedCallback > > (which you are doing), so I assume you are only not registering the WGC3D > > callback as an optimization? (In which case if you are doing this > > micro-optimization let's fully disable the code.) > > Actually this is sort of the "future down" already. Not sending > GpuCommandBufferMsg_SetClientHasMemoryAllocationChangedCallback disables all the > work. > > I guess the only other thing is to not create the GpuMemoryManager at all, and > DCHECK it's there if it's needed. Worth it? Actually that won't work. Other parts of command buffer tracks memory using the GpuMemoryManager it seems. It's not only for these callbacks.
https://codereview.chromium.org/1268383003/diff/1/content/browser/android/in_... File content/browser/android/in_process/synchronous_compositor_context_provider.cc (right): https://codereview.chromium.org/1268383003/diff/1/content/browser/android/in_... content/browser/android/in_process/synchronous_compositor_context_provider.cc:18: // Intentional no-op. But this can be NOTREACHED() then?
https://codereview.chromium.org/1268383003/diff/1/content/browser/android/in_... File content/browser/android/in_process/synchronous_compositor_context_provider.cc (right): https://codereview.chromium.org/1268383003/diff/1/content/browser/android/in_... content/browser/android/in_process/synchronous_compositor_context_provider.cc:18: // Intentional no-op. On 2015/08/05 22:31:27, sievers wrote: > But this can be NOTREACHED() then? Nope.. it's still called. But this override is technically not really needed. Setting the callback in provider doesn't hurt anything, as long as it's not hooked up to the command buffer. Ehh, I'll get rid of it.
https://codereview.chromium.org/1268383003/diff/20001/content/common/gpu/clie... File content/common/gpu/client/context_provider_command_buffer.cc (right): https://codereview.chromium.org/1268383003/diff/20001/content/common/gpu/clie... content/common/gpu/client/context_provider_command_buffer.cc:114: context3d_->GetCommandBufferProxy()->SetMemoryAllocationChangedCallback( Wait, can you just move this code to SetMemoryPolicyChangedCallback() and then override that in your subclass to do nothing? :)
https://codereview.chromium.org/1268383003/diff/20001/content/common/gpu/clie... File content/common/gpu/client/context_provider_command_buffer.cc (right): https://codereview.chromium.org/1268383003/diff/20001/content/common/gpu/clie... content/common/gpu/client/context_provider_command_buffer.cc:114: context3d_->GetCommandBufferProxy()->SetMemoryAllocationChangedCallback( On 2015/08/05 22:50:32, sievers wrote: > Wait, can you just move this code to SetMemoryPolicyChangedCallback() and then > override that in your subclass to do nothing? :) Totally thought about that! But... enabling callbacks is not a synchronous operation. So doing it later could affect when the first memory allocation callback happens, which could intern affect when first raster starts, which can affect timings of a lot of things. So decided to not do that :)
https://codereview.chromium.org/1268383003/diff/20001/content/common/gpu/clie... File content/common/gpu/client/context_provider_command_buffer.cc (right): https://codereview.chromium.org/1268383003/diff/20001/content/common/gpu/clie... content/common/gpu/client/context_provider_command_buffer.cc:114: context3d_->GetCommandBufferProxy()->SetMemoryAllocationChangedCallback( On 2015/08/06 01:30:33, boliu wrote: > On 2015/08/05 22:50:32, sievers wrote: > > Wait, can you just move this code to SetMemoryPolicyChangedCallback() and then > > override that in your subclass to do nothing? :) > > Totally thought about that! > > But... enabling callbacks is not a synchronous operation. So doing it later > could affect when the first memory allocation callback happens, which could > intern affect when first raster starts, which can affect timings of a lot of > things. > > So decided to not do that :) And then I realize these two things actually happen at the exact same time. So overriding this it is!
lgtm
The CQ bit was checked by boliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1268383003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1268383003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by boliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1268383003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1268383003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by boliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1268383003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1268383003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by boliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1268383003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1268383003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2015/08/06 20:45:40, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) Ok that failure failed twice now. And error logs do indicate something wrong with command buffer..
https://codereview.chromium.org/1268383003/diff/40001/content/common/gpu/clie... File content/common/gpu/client/context_provider_command_buffer.cc (right): https://codereview.chromium.org/1268383003/diff/40001/content/common/gpu/clie... content/common/gpu/client/context_provider_command_buffer.cc:246: context3d_->GetCommandBufferProxy()->SetMemoryAllocationChangedCallback( Oh I bet this needs a null check. context3d_->GetCommandBufferProxy may be null if context creation fails (on purpose in the failing tests) I wish linux bots provide better debug info..
On 2015/08/06 21:13:50, boliu wrote: > https://codereview.chromium.org/1268383003/diff/40001/content/common/gpu/clie... > File content/common/gpu/client/context_provider_command_buffer.cc (right): > > https://codereview.chromium.org/1268383003/diff/40001/content/common/gpu/clie... > content/common/gpu/client/context_provider_command_buffer.cc:246: > context3d_->GetCommandBufferProxy()->SetMemoryAllocationChangedCallback( > Oh I bet this needs a null check. context3d_->GetCommandBufferProxy may be null > if context creation fails (on purpose in the failing tests) > > I wish linux bots provide better debug info.. ptal No longer changing the callback unset behavior. Just do it in the destructor instead. ContextProviderCommandBuffer::OnMemoryAllocationChanged already has a null check so it's ok.
lgtm
The CQ bit was checked by boliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1268383003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1268383003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by boliu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1268383003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1268383003/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ec1476c26c6602c80cc8131cd507c250f6150652 Cr-Commit-Position: refs/heads/master@{#342267} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
