Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2286583002:
Make cc::SoftwareImageDecodeController, cc::ResourcePool, and cc::StagingBufferPoo… (Closed)
DescriptionMake cc::SoftwareImageDecodeController, cc::GpuImageDecodeController, cc::ResourcePool, and cc::StagingBufferPool MemoryCoordinatorClient.
- implement OnMemoryStateChange for background renderer's purge +
suspend. So the method reduces memory only when state == SUSPENDED.
- intent-to-implement-and-ship of background renderer's purge + suspend is
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4
- one of the documents attached in the above intent is
https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJvI/edit?usp=sharing
BUG=635419
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
Committed: https://crrev.com/e7ac1bbe93e087b173d503ca13b1e3b6f703b785
Cr-Commit-Position: refs/heads/master@{#418834}
Patch Set 1 #Patch Set 2 : Fixed DEPS. #
Total comments: 12
Patch Set 3 : WIP #
Total comments: 2
Patch Set 4 : Fixed BUILD.gn's DEPS #
Total comments: 3
Patch Set 5 : Fixed windows build failure #
Total comments: 2
Patch Set 6 : Updated (r418766) #
Total comments: 3
Patch Set 7 : Updated (r418783) #
Total comments: 1
Patch Set 8 : Patch for landing #
Messages
Total messages: 71 (38 generated)
Description was changed from ========== WIP: Make cc::SoftwareImageDecoder, cc::GpuImageDecoder, cc::ResourcePool, and cc::StagingBufferPool MemoryCoordinatorClients. - implement OnMemoryStateChange for background renderer's purge + suspend. So the method reduces memory only when state == SUSPENDED. - intent-to-implement-and-ship of background renderer's purge + suspend is https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4 - one of the documents attached in the above intent is https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ... BUG=635419 ========== to ========== WIP: Make cc::SoftwareImageDecoder, cc::GpuImageDecoder, cc::ResourcePool, and cc::StagingBufferPool MemoryCoordinatorClients. - implement OnMemoryStateChange for background renderer's purge + suspend. So the method reduces memory only when state == SUSPENDED. - intent-to-implement-and-ship of background renderer's purge + suspend is https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4 - one of the documents attached in the above intent is https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ... BUG=635419 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
The CQ bit was checked by tasak@google.com 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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tasak@google.com 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...
Description was changed from ========== WIP: Make cc::SoftwareImageDecoder, cc::GpuImageDecoder, cc::ResourcePool, and cc::StagingBufferPool MemoryCoordinatorClients. - implement OnMemoryStateChange for background renderer's purge + suspend. So the method reduces memory only when state == SUSPENDED. - intent-to-implement-and-ship of background renderer's purge + suspend is https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4 - one of the documents attached in the above intent is https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ... BUG=635419 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== WIP: Make cc::SoftwareImageDecoder, cc::GpuImageDecoder, cc::ResourcePool, and cc::StagingBufferPool MemoryCoordinatorClient. - implement OnMemoryStateChange for background renderer's purge + suspend. So the method reduces memory only when state == SUSPENDED. - intent-to-implement-and-ship of background renderer's purge + suspend is https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4 - one of the documents attached in the above intent is https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ... BUG=635419 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
tasak@google.com changed reviewers: + bashi@chromium.org, ericrk@chromium.org, haraken@chromium.org
Would you take a look at this patch? I would like to add OnMemoryStateChange to cc for purge+suspend. Thanks.
Description was changed from ========== WIP: Make cc::SoftwareImageDecoder, cc::GpuImageDecoder, cc::ResourcePool, and cc::StagingBufferPool MemoryCoordinatorClient. - implement OnMemoryStateChange for background renderer's purge + suspend. So the method reduces memory only when state == SUSPENDED. - intent-to-implement-and-ship of background renderer's purge + suspend is https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4 - one of the documents attached in the above intent is https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ... BUG=635419 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== WIP: Make cc::SoftwareImageDecodeController, cc::GpuImageDecoder, cc::ResourcePool, and cc::StagingBufferPool MemoryCoordinatorClient. - implement OnMemoryStateChange for background renderer's purge + suspend. So the method reduces memory only when state == SUSPENDED. - intent-to-implement-and-ship of background renderer's purge + suspend is https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4 - one of the documents attached in the above intent is https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ... BUG=635419 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
Description was changed from ========== WIP: Make cc::SoftwareImageDecodeController, cc::GpuImageDecoder, cc::ResourcePool, and cc::StagingBufferPool MemoryCoordinatorClient. - implement OnMemoryStateChange for background renderer's purge + suspend. So the method reduces memory only when state == SUSPENDED. - intent-to-implement-and-ship of background renderer's purge + suspend is https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4 - one of the documents attached in the above intent is https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ... BUG=635419 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== WIP: Make cc::SoftwareImageDecodeController, cc::GpuImageDecodeController, cc::ResourcePool, and cc::StagingBufferPool MemoryCoordinatorClient. - implement OnMemoryStateChange for background renderer's purge + suspend. So the method reduces memory only when state == SUSPENDED. - intent-to-implement-and-ship of background renderer's purge + suspend is https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4 - one of the documents attached in the above intent is https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ... BUG=635419 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
I have to add "registerClient" and "unregisterClient" to register/unregister these classes with MemoryCoordinator. https://codereview.chromium.org/2286583002/diff/20001/cc/raster/staging_buffe... File cc/raster/staging_buffer_pool.cc (right): https://codereview.chromium.org/2286583002/diff/20001/cc/raster/staging_buffe... cc/raster/staging_buffer_pool.cc:65: content_id(0) {} Need to register this with ChildMemoryCoordinator here. https://codereview.chromium.org/2286583002/diff/20001/cc/raster/staging_buffe... cc/raster/staging_buffer_pool.cc:70: DCHECK_EQ(query_id, 0u); Need to unregister this with ChildMemoryCoordinator here. https://codereview.chromium.org/2286583002/diff/20001/cc/resources/resource_p... File cc/resources/resource_pool.cc (right): https://codereview.chromium.org/2286583002/diff/20001/cc/resources/resource_p... cc/resources/resource_pool.cc:73: this, "cc::ResourcePool", task_runner_.get()); Need to register this with ChildMemoryCoordinator here. https://codereview.chromium.org/2286583002/diff/20001/cc/resources/resource_p... cc/resources/resource_pool.cc:90: DCHECK_EQ(0u, total_resource_count_); Need to unregister this with ChildMemoryCoordinator here. https://codereview.chromium.org/2286583002/diff/20001/cc/tiles/gpu_image_deco... File cc/tiles/gpu_image_decode_controller.cc (right): https://codereview.chromium.org/2286583002/diff/20001/cc/tiles/gpu_image_deco... cc/tiles/gpu_image_decode_controller.cc:220: GpuImageDecodeController::DecodedImageData::DecodedImageData() = default; Need to register this from ChildMemoryCoordinator here. https://codereview.chromium.org/2286583002/diff/20001/cc/tiles/gpu_image_deco... cc/tiles/gpu_image_decode_controller.cc:222: ResetData(); Need to unregister this from ChildMemoryCoordinator here. https://codereview.chromium.org/2286583002/diff/20001/cc/tiles/software_image... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/2286583002/diff/20001/cc/tiles/software_image... cc/tiles/software_image_decode_controller.cc:182: } Need to register this with ChildMemoryCoordinator here. https://codereview.chromium.org/2286583002/diff/20001/cc/tiles/software_image... cc/tiles/software_image_decode_controller.cc:191: this); Need to unregister this with ChildMemoryCoordinator here.
On 2016/08/26 10:08:05, tasak wrote: > I have to add "registerClient" and "unregisterClient" to register/unregister > these classes with MemoryCoordinator. Yeah, we need a static method to get the instance of ChildMemoryCoordinator. Are you going to implement it by yourself? Or should I do that? > > https://codereview.chromium.org/2286583002/diff/20001/cc/raster/staging_buffe... > File cc/raster/staging_buffer_pool.cc (right): > > https://codereview.chromium.org/2286583002/diff/20001/cc/raster/staging_buffe... > cc/raster/staging_buffer_pool.cc:65: content_id(0) {} > Need to register this with ChildMemoryCoordinator here. > > https://codereview.chromium.org/2286583002/diff/20001/cc/raster/staging_buffe... > cc/raster/staging_buffer_pool.cc:70: DCHECK_EQ(query_id, 0u); > Need to unregister this with ChildMemoryCoordinator here. > > https://codereview.chromium.org/2286583002/diff/20001/cc/resources/resource_p... > File cc/resources/resource_pool.cc (right): > > https://codereview.chromium.org/2286583002/diff/20001/cc/resources/resource_p... > cc/resources/resource_pool.cc:73: this, "cc::ResourcePool", task_runner_.get()); > Need to register this with ChildMemoryCoordinator here. > > https://codereview.chromium.org/2286583002/diff/20001/cc/resources/resource_p... > cc/resources/resource_pool.cc:90: DCHECK_EQ(0u, total_resource_count_); > Need to unregister this with ChildMemoryCoordinator here. > > https://codereview.chromium.org/2286583002/diff/20001/cc/tiles/gpu_image_deco... > File cc/tiles/gpu_image_decode_controller.cc (right): > > https://codereview.chromium.org/2286583002/diff/20001/cc/tiles/gpu_image_deco... > cc/tiles/gpu_image_decode_controller.cc:220: > GpuImageDecodeController::DecodedImageData::DecodedImageData() = default; > Need to register this from ChildMemoryCoordinator here. > > https://codereview.chromium.org/2286583002/diff/20001/cc/tiles/gpu_image_deco... > cc/tiles/gpu_image_decode_controller.cc:222: ResetData(); > Need to unregister this from ChildMemoryCoordinator here. > > https://codereview.chromium.org/2286583002/diff/20001/cc/tiles/software_image... > File cc/tiles/software_image_decode_controller.cc (right): > > https://codereview.chromium.org/2286583002/diff/20001/cc/tiles/software_image... > cc/tiles/software_image_decode_controller.cc:182: } > Need to register this with ChildMemoryCoordinator here. > > https://codereview.chromium.org/2286583002/diff/20001/cc/tiles/software_image... > cc/tiles/software_image_decode_controller.cc:191: this); > Need to unregister this with ChildMemoryCoordinator here.
Ericrk: Do you have any thoughts on this CL?
vmpstr@chromium.org changed reviewers: + vmpstr@chromium.org
Thanks for doing this! I only took a look at the software image decode controller. https://codereview.chromium.org/2286583002/diff/20001/cc/tiles/software_image... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/2286583002/diff/20001/cc/tiles/software_image... cc/tiles/software_image_decode_controller.cc:1083: if (state == memory_coordinator::mojom::MemoryState::SUSPENDED) { What are the different states that we can be in? https://codereview.chromium.org/2286583002/diff/20001/cc/tiles/software_image... cc/tiles/software_image_decode_controller.cc:1085: at_raster_decoded_images_.Clear(); I don't think this is safe to do, because both of these maps may contain locked images that are being used at raster on different threads. Can you rename ReduceCacheUsage to ReduceCacheUsageInternal and make it take an int for the max items to keep? Then this can call ReduceCacheUsageInternal(0) and ReduceCacheUsage() can then call ReduceCacheUsageInternal(kMaxItemsInCache)? I'm happy to put this in if you just want to leave the function empty for now as well (you can leave a TODO(vmpstr) here)
https://codereview.chromium.org/2286583002/diff/20001/cc/resources/resource_p... File cc/resources/resource_pool.cc (right): https://codereview.chromium.org/2286583002/diff/20001/cc/resources/resource_p... cc/resources/resource_pool.cc:460: DeleteResource(PopBack(&unused_resources_)); This doesn't free "busy" resources, which can safely be released. Prefer: EvictResourcesNotUsedSince(base::TimeTicks() + base::TimeDelta::Max()); https://codereview.chromium.org/2286583002/diff/20001/cc/tiles/gpu_image_deco... File cc/tiles/gpu_image_decode_controller.cc (right): https://codereview.chromium.org/2286583002/diff/20001/cc/tiles/gpu_image_deco... cc/tiles/gpu_image_decode_controller.cc:1140: SetShouldAggressivelyFreeResources(true); Setting this is stateful, so we need to be careful about setting this from more than one place (we need to make sure this is consistently re-set to "false" once we are no longer in a suspended memory state.) Can we ever be "SUSPENDED" while visible? GpuImageDecodeController should already set AggressivelyFreeResources(true) any time we are not visible, so this is likely not necessary. You could DCHECK that we are aggressively freeing in this case.
Also, it's great to get this basic support in! Should these functions be expanded in the future to handle THROTTLED as well? I'm happy to create bugs for/handle getting this added.
On 2016/08/31 17:53:00, ericrk wrote: > Also, it's great to get this basic support in! Should these functions be > expanded in the future to handle THROTTLED as well? I'm happy to create bugs > for/handle getting this added. Correct. Our plan is to ship SUSPENDED (to a Finch experiment) first and then support THROTTLE. SUSPENDED happens only on background renderers. Each memory component is expected to purge as much memory as possible. We're happy to just delegate this CL to you guys if that's easier :)
On 2016/08/31 17:59:34, haraken wrote: > On 2016/08/31 17:53:00, ericrk wrote: > > Also, it's great to get this basic support in! Should these functions be > > expanded in the future to handle THROTTLED as well? I'm happy to create bugs > > for/handle getting this added. > > Correct. Our plan is to ship SUSPENDED (to a Finch experiment) first and then > support THROTTLE. > > SUSPENDED happens only on background renderers. Each memory component is > expected to purge as much memory as possible. > > We're happy to just delegate this CL to you guys if that's easier :) Yeah that's fine. Can you folks just add the plumbing / registration / etc and just leave the functions as stubs with comments on what state we want to react to, and we can put in the actual code to react to that state :)
On 2016/08/31 18:08:03, vmpstr wrote: > On 2016/08/31 17:59:34, haraken wrote: > > On 2016/08/31 17:53:00, ericrk wrote: > > > Also, it's great to get this basic support in! Should these functions be > > > expanded in the future to handle THROTTLED as well? I'm happy to create bugs > > > for/handle getting this added. > > > > Correct. Our plan is to ship SUSPENDED (to a Finch experiment) first and then > > support THROTTLE. > > > > SUSPENDED happens only on background renderers. Each memory component is > > expected to purge as much memory as possible. > > > > We're happy to just delegate this CL to you guys if that's easier :) > > Yeah that's fine. Can you folks just add the plumbing / registration / etc and > just leave the functions as stubs with comments on what state we want to react > to, and we can put in the actual code to react to that state :) tasak@: Let's do this :)
https://codereview.chromium.org/2286583002/diff/40001/cc/BUILD.gn File cc/BUILD.gn (right): https://codereview.chromium.org/2286583002/diff/40001/cc/BUILD.gn#newcode570 cc/BUILD.gn:570: "//components/memory_coordinator/child", I made a mistake. We don't need this. https://codereview.chromium.org/2286583002/diff/40001/cc/raster/staging_buffe... File cc/raster/staging_buffer_pool.cc (right): https://codereview.chromium.org/2286583002/diff/40001/cc/raster/staging_buffe... cc/raster/staging_buffer_pool.cc:150: registry->RegisterClient(this); Is it better to implement RegisterClientIfAvailable or something?
The CQ bit was checked by tasak@google.com 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...
On 2016/08/31 18:15:11, haraken wrote: > On 2016/08/31 18:08:03, vmpstr wrote: > > On 2016/08/31 17:59:34, haraken wrote: > > > On 2016/08/31 17:53:00, ericrk wrote: > > > > Also, it's great to get this basic support in! Should these functions be > > > > expanded in the future to handle THROTTLED as well? I'm happy to create > bugs > > > > for/handle getting this added. > > > > > > Correct. Our plan is to ship SUSPENDED (to a Finch experiment) first and > then > > > support THROTTLE. > > > > > > SUSPENDED happens only on background renderers. Each memory component is > > > expected to purge as much memory as possible. > > > > > > We're happy to just delegate this CL to you guys if that's easier :) > > > > Yeah that's fine. Can you folks just add the plumbing / registration / etc and > > just leave the functions as stubs with comments on what state we want to react > > to, and we can put in the actual code to react to that state :) > > tasak@: Let's do this :) Sure. I uploaded a new patch.
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...)
lgtm - thanks for taking care of this! Looked at where ChildMemoryCoordinatorImpl is created (in RenderThreadImpl), and this seems fine at the moment, but one thing to be careful of is where this object is constructed/destructed, as the classes that are registering/unregistering with the registry are not necessarily doing so on the same thread. https://codereview.chromium.org/2286583002/diff/60001/cc/DEPS File cc/DEPS (right): https://codereview.chromium.org/2286583002/diff/60001/cc/DEPS#newcode4 cc/DEPS:4: "+components/memory_coordinator/public/interfaces/child_memory_coordinator.mojom.h", It doesn't look like the mojom.h is directly included anywhere - is this rule needed? https://codereview.chromium.org/2286583002/diff/60001/cc/raster/staging_buffe... File cc/raster/staging_buffer_pool.cc (right): https://codereview.chromium.org/2286583002/diff/60001/cc/raster/staging_buffe... cc/raster/staging_buffer_pool.cc:425: memory_coordinator::mojom::MemoryState state) { Thanks for the thorough stubs! Out of curiosity, what thread is OnMemoryStateChanged called on?
https://codereview.chromium.org/2286583002/diff/60001/cc/raster/staging_buffe... File cc/raster/staging_buffer_pool.cc (right): https://codereview.chromium.org/2286583002/diff/60001/cc/raster/staging_buffe... cc/raster/staging_buffer_pool.cc:425: memory_coordinator::mojom::MemoryState state) { On 2016/09/01 16:42:23, ericrk wrote: > Thanks for the thorough stubs! Out of curiosity, what thread is > OnMemoryStateChanged called on? RegisterClient/UnregisterClient/OnMemoryStateChange must be called on the main thread of the renderer process. If StagingBufferPool's constructor/destructor can be called on a non-main thread, we need to post a task to do the registration/unregistration on the main thread.
On 2016/09/01 17:42:05, haraken wrote: > https://codereview.chromium.org/2286583002/diff/60001/cc/raster/staging_buffe... > File cc/raster/staging_buffer_pool.cc (right): > > https://codereview.chromium.org/2286583002/diff/60001/cc/raster/staging_buffe... > cc/raster/staging_buffer_pool.cc:425: memory_coordinator::mojom::MemoryState > state) { > On 2016/09/01 16:42:23, ericrk wrote: > > Thanks for the thorough stubs! Out of curiosity, what thread is > > OnMemoryStateChanged called on? > > RegisterClient/UnregisterClient/OnMemoryStateChange must be called on the main > thread of the renderer process. > > If StagingBufferPool's constructor/destructor can be called on a non-main > thread, we need to post a task to do the registration/unregistration on the main > thread. Ah, good point - I was assuming the class was threadsafe and so the only race could whether it was initialized in time. If we need to call on the main thread, things get a bit interesting - as it currently stands, many of these classes are created on the compositor's Impl thread (not the main thread). Posting a task to register on the main thread is fine, but I'm a bit concerned about posting a task to unregister on the main thread. We need to unregister before the class destructs, so we would need to wait on the main thread task to complete in the destructor of the object. A better option might be to only register cc::LayerTreeHostImpl with the registry (rather than the various resource pools and image decode controllers). LayerTreeHostImpl is created/destroyed safely with regards to the main thread (it blocks it). It also owns the various pools and image decode controllers, and can forward these messages to them. WDYT?
On 2016/09/01 18:39:53, ericrk wrote: > On 2016/09/01 17:42:05, haraken wrote: > > > https://codereview.chromium.org/2286583002/diff/60001/cc/raster/staging_buffe... > > File cc/raster/staging_buffer_pool.cc (right): > > > > > https://codereview.chromium.org/2286583002/diff/60001/cc/raster/staging_buffe... > > cc/raster/staging_buffer_pool.cc:425: memory_coordinator::mojom::MemoryState > > state) { > > On 2016/09/01 16:42:23, ericrk wrote: > > > Thanks for the thorough stubs! Out of curiosity, what thread is > > > OnMemoryStateChanged called on? > > > > RegisterClient/UnregisterClient/OnMemoryStateChange must be called on the main > > thread of the renderer process. > > > > If StagingBufferPool's constructor/destructor can be called on a non-main > > thread, we need to post a task to do the registration/unregistration on the > main > > thread. > > Ah, good point - I was assuming the class was threadsafe and so the only race > could whether it was initialized in time. > > If we need to call on the main thread, things get a bit interesting - as it > currently stands, many of these classes are created on the compositor's Impl > thread (not the main thread). > > Posting a task to register on the main thread is fine, but I'm a bit concerned > about posting a task to unregister on the main thread. We need to unregister > before the class destructs, so we would need to wait on the main thread task to > complete in the destructor of the object. > > A better option might be to only register cc::LayerTreeHostImpl with the > registry (rather than the various resource pools and image decode controllers). > LayerTreeHostImpl is created/destroyed safely with regards to the main thread > (it blocks it). It also owns the various pools and image decode controllers, and > can forward these messages to them. > > WDYT? Sounds great to me!
On 2016/09/01 18:39:53, ericrk wrote: > On 2016/09/01 17:42:05, haraken wrote: > > > https://codereview.chromium.org/2286583002/diff/60001/cc/raster/staging_buffe... > > File cc/raster/staging_buffer_pool.cc (right): > > > > > https://codereview.chromium.org/2286583002/diff/60001/cc/raster/staging_buffe... > > cc/raster/staging_buffer_pool.cc:425: memory_coordinator::mojom::MemoryState > > state) { > > On 2016/09/01 16:42:23, ericrk wrote: > > > Thanks for the thorough stubs! Out of curiosity, what thread is > > > OnMemoryStateChanged called on? > > > > RegisterClient/UnregisterClient/OnMemoryStateChange must be called on the main > > thread of the renderer process. > > > > If StagingBufferPool's constructor/destructor can be called on a non-main > > thread, we need to post a task to do the registration/unregistration on the > main > > thread. > > Ah, good point - I was assuming the class was threadsafe and so the only race > could whether it was initialized in time. CMC is *thread-safe*. It uses ObserverListThreadSafe to manage clients and callbacks are called on the same thread registerClient() is called. > > If we need to call on the main thread, things get a bit interesting - as it > currently stands, many of these classes are created on the compositor's Impl > thread (not the main thread). > > Posting a task to register on the main thread is fine, but I'm a bit concerned > about posting a task to unregister on the main thread. We need to unregister > before the class destructs, so we would need to wait on the main thread task to > complete in the destructor of the object. > > A better option might be to only register cc::LayerTreeHostImpl with the > registry (rather than the various resource pools and image decode controllers). > LayerTreeHostImpl is created/destroyed safely with regards to the main thread > (it blocks it). It also owns the various pools and image decode controllers, and > can forward these messages to them. > > WDYT?
On 2016/09/01 22:58:22, bashi1 wrote: > On 2016/09/01 18:39:53, ericrk wrote: > > On 2016/09/01 17:42:05, haraken wrote: > > > > > > https://codereview.chromium.org/2286583002/diff/60001/cc/raster/staging_buffe... > > > File cc/raster/staging_buffer_pool.cc (right): > > > > > > > > > https://codereview.chromium.org/2286583002/diff/60001/cc/raster/staging_buffe... > > > cc/raster/staging_buffer_pool.cc:425: memory_coordinator::mojom::MemoryState > > > state) { > > > On 2016/09/01 16:42:23, ericrk wrote: > > > > Thanks for the thorough stubs! Out of curiosity, what thread is > > > > OnMemoryStateChanged called on? > > > > > > RegisterClient/UnregisterClient/OnMemoryStateChange must be called on the > main > > > thread of the renderer process. > > > > > > If StagingBufferPool's constructor/destructor can be called on a non-main > > > thread, we need to post a task to do the registration/unregistration on the > > main > > > thread. > > > > Ah, good point - I was assuming the class was threadsafe and so the only race > > could whether it was initialized in time. > > CMC is *thread-safe*. It uses ObserverListThreadSafe to manage clients and > callbacks > are called on the same thread registerClient() is called. Sorry, I was misunderstanding. ericrk@: Then do you think it makes sense to land this CL? Or would it be still better to centralize the OnMemoryStateChange call to cc::LayerTreeHostImpl? > > > > > If we need to call on the main thread, things get a bit interesting - as it > > currently stands, many of these classes are created on the compositor's Impl > > thread (not the main thread). > > > > Posting a task to register on the main thread is fine, but I'm a bit concerned > > about posting a task to unregister on the main thread. We need to unregister > > before the class destructs, so we would need to wait on the main thread task > to > > complete in the destructor of the object. > > > > A better option might be to only register cc::LayerTreeHostImpl with the > > registry (rather than the various resource pools and image decode > controllers). > > LayerTreeHostImpl is created/destroyed safely with regards to the main thread > > (it blocks it). It also owns the various pools and image decode controllers, > and > > can forward these messages to them. > > > > WDYT?
The CQ bit was checked by tasak@google.com 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.
I'm now waiting until https://codereview.chromium.org/2311723002/ is landed.
On 2016/09/01 23:17:14, haraken wrote: > On 2016/09/01 22:58:22, bashi1 wrote: > > On 2016/09/01 18:39:53, ericrk wrote: > > > On 2016/09/01 17:42:05, haraken wrote: > > > > > > > > > > https://codereview.chromium.org/2286583002/diff/60001/cc/raster/staging_buffe... > > > > File cc/raster/staging_buffer_pool.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2286583002/diff/60001/cc/raster/staging_buffe... > > > > cc/raster/staging_buffer_pool.cc:425: > memory_coordinator::mojom::MemoryState > > > > state) { > > > > On 2016/09/01 16:42:23, ericrk wrote: > > > > > Thanks for the thorough stubs! Out of curiosity, what thread is > > > > > OnMemoryStateChanged called on? > > > > > > > > RegisterClient/UnregisterClient/OnMemoryStateChange must be called on the > > main > > > > thread of the renderer process. > > > > > > > > If StagingBufferPool's constructor/destructor can be called on a non-main > > > > thread, we need to post a task to do the registration/unregistration on > the > > > main > > > > thread. > > > > > > Ah, good point - I was assuming the class was threadsafe and so the only > race > > > could whether it was initialized in time. > > > > CMC is *thread-safe*. It uses ObserverListThreadSafe to manage clients and > > callbacks > > are called on the same thread registerClient() is called. > > Sorry, I was misunderstanding. > > ericrk@: Then do you think it makes sense to land this CL? Or would it be still > better to centralize the OnMemoryStateChange call to cc::LayerTreeHostImpl? > > > > > > > > > If we need to call on the main thread, things get a bit interesting - as it > > > currently stands, many of these classes are created on the compositor's Impl > > > thread (not the main thread). > > > > > > Posting a task to register on the main thread is fine, but I'm a bit > concerned > > > about posting a task to unregister on the main thread. We need to unregister > > > before the class destructs, so we would need to wait on the main thread task > > to > > > complete in the destructor of the object. > > > > > > A better option might be to only register cc::LayerTreeHostImpl with the > > > registry (rather than the various resource pools and image decode > > controllers). > > > LayerTreeHostImpl is created/destroyed safely with regards to the main > thread > > > (it blocks it). It also owns the various pools and image decode controllers, > > and > > > can forward these messages to them. > > > > > > WDYT? Ah, ok. I think I like this better than centralizing, as it makes it easier to move components around / change ownership. Let's just stick with this CL. Still LGTM.
lgtm as well https://codereview.chromium.org/2286583002/diff/80001/cc/raster/staging_buffe... File cc/raster/staging_buffer_pool.cc (right): https://codereview.chromium.org/2286583002/diff/80001/cc/raster/staging_buffe... cc/raster/staging_buffer_pool.cc:438: default: nit: typically, if we handle all of the cases in the switch, we don't put the default case since then the compiler would complain if a new case is added.
The CQ bit was checked by tasak@google.com 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_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
ericrk, vmpstr, thank you for review. bashi-san, would you check if this CL correctly uses base::MemoryCoordinatorClient, base::MemoryCoordinatorClientRegistry, and so on? https://codereview.chromium.org/2286583002/diff/80001/cc/raster/staging_buffe... File cc/raster/staging_buffer_pool.cc (right): https://codereview.chromium.org/2286583002/diff/80001/cc/raster/staging_buffe... cc/raster/staging_buffer_pool.cc:438: default: On 2016/09/06 18:14:47, vmpstr wrote: > nit: typically, if we handle all of the cases in the switch, we don't put the > default case since then the compiler would complain if a new case is added. Done.
The CQ bit was checked by tasak@google.com 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...
https://codereview.chromium.org/2286583002/diff/100001/cc/raster/staging_buff... File cc/raster/staging_buffer_pool.cc (right): https://codereview.chromium.org/2286583002/diff/100001/cc/raster/staging_buff... cc/raster/staging_buffer_pool.cc:147: base::MemoryCoordinatorClientRegistry::GetInstance()->Register(this); Could you check base::FeatureList::IsEnabled(features::kMemoryCoordinator) ?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thank you for review, bashi-san. https://codereview.chromium.org/2286583002/diff/100001/cc/raster/staging_buff... File cc/raster/staging_buffer_pool.cc (right): https://codereview.chromium.org/2286583002/diff/100001/cc/raster/staging_buff... cc/raster/staging_buffer_pool.cc:147: base::MemoryCoordinatorClientRegistry::GetInstance()->Register(this); On 2016/09/15 07:41:01, bashi1 wrote: > Could you check base::FeatureList::IsEnabled(features::kMemoryCoordinator) ? Hmmm... I would like to ask one question. Is it possible to depend content/public/common/content_features.h?
https://codereview.chromium.org/2286583002/diff/100001/cc/raster/staging_buff... File cc/raster/staging_buffer_pool.cc (right): https://codereview.chromium.org/2286583002/diff/100001/cc/raster/staging_buff... cc/raster/staging_buffer_pool.cc:147: base::MemoryCoordinatorClientRegistry::GetInstance()->Register(this); On 2016/09/15 09:06:06, tasak wrote: > On 2016/09/15 07:41:01, bashi1 wrote: > > Could you check base::FeatureList::IsEnabled(features::kMemoryCoordinator) ? > > Hmmm... I would like to ask one question. > Is it possible to depend content/public/common/content_features.h? Ah, I wasn't aware of that. Then registering unconditionally makes sense to me. lgtm.
On 2016/09/15 10:00:34, bashi1 wrote: > https://codereview.chromium.org/2286583002/diff/100001/cc/raster/staging_buff... > File cc/raster/staging_buffer_pool.cc (right): > > https://codereview.chromium.org/2286583002/diff/100001/cc/raster/staging_buff... > cc/raster/staging_buffer_pool.cc:147: > base::MemoryCoordinatorClientRegistry::GetInstance()->Register(this); > On 2016/09/15 09:06:06, tasak wrote: > > On 2016/09/15 07:41:01, bashi1 wrote: > > > Could you check base::FeatureList::IsEnabled(features::kMemoryCoordinator) ? > > > > Hmmm... I would like to ask one question. > > Is it possible to depend content/public/common/content_features.h? > > Ah, I wasn't aware of that. Then registering unconditionally makes sense to me. > lgtm. Thank you, bashi-san.
The CQ bit was checked by tasak@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from ericrk@chromium.org, vmpstr@chromium.org Link to the patchset: https://codereview.chromium.org/2286583002/#ps120001 (title: "Updated (r418783)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== WIP: Make cc::SoftwareImageDecodeController, cc::GpuImageDecodeController, cc::ResourcePool, and cc::StagingBufferPool MemoryCoordinatorClient. - implement OnMemoryStateChange for background renderer's purge + suspend. So the method reduces memory only when state == SUSPENDED. - intent-to-implement-and-ship of background renderer's purge + suspend is https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4 - one of the documents attached in the above intent is https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ... BUG=635419 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Make cc::SoftwareImageDecodeController, cc::GpuImageDecodeController, cc::ResourcePool, and cc::StagingBufferPool MemoryCoordinatorClient. - implement OnMemoryStateChange for background renderer's purge + suspend. So the method reduces memory only when state == SUSPENDED. - intent-to-implement-and-ship of background renderer's purge + suspend is https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4 - one of the documents attached in the above intent is https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ... BUG=635419 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2286583002/diff/120001/content/renderer/rende... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/2286583002/diff/120001/content/renderer/rende... content/renderer/render_thread_impl.cc:1703: memory_coordinator_->PrepareToSuspend(); The code is just for explaining how to invoke PrepareToSuspend (for Purge+Suspend). I will add this code in another CL.
The CQ bit was checked by tasak@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from ericrk@chromium.org, vmpstr@chromium.org, bashi@chromium.org Link to the patchset: https://codereview.chromium.org/2286583002/#ps140001 (title: "Patch for landing")
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 ========== Make cc::SoftwareImageDecodeController, cc::GpuImageDecodeController, cc::ResourcePool, and cc::StagingBufferPool MemoryCoordinatorClient. - implement OnMemoryStateChange for background renderer's purge + suspend. So the method reduces memory only when state == SUSPENDED. - intent-to-implement-and-ship of background renderer's purge + suspend is https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4 - one of the documents attached in the above intent is https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ... BUG=635419 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Make cc::SoftwareImageDecodeController, cc::GpuImageDecodeController, cc::ResourcePool, and cc::StagingBufferPool MemoryCoordinatorClient. - implement OnMemoryStateChange for background renderer's purge + suspend. So the method reduces memory only when state == SUSPENDED. - intent-to-implement-and-ship of background renderer's purge + suspend is https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4 - one of the documents attached in the above intent is https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ... BUG=635419 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Make cc::SoftwareImageDecodeController, cc::GpuImageDecodeController, cc::ResourcePool, and cc::StagingBufferPool MemoryCoordinatorClient. - implement OnMemoryStateChange for background renderer's purge + suspend. So the method reduces memory only when state == SUSPENDED. - intent-to-implement-and-ship of background renderer's purge + suspend is https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4 - one of the documents attached in the above intent is https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ... BUG=635419 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Make cc::SoftwareImageDecodeController, cc::GpuImageDecodeController, cc::ResourcePool, and cc::StagingBufferPool MemoryCoordinatorClient. - implement OnMemoryStateChange for background renderer's purge + suspend. So the method reduces memory only when state == SUSPENDED. - intent-to-implement-and-ship of background renderer's purge + suspend is https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/DK189tnM8l4 - one of the documents attached in the above intent is https://docs.google.com/document/d/1EgLimgxWK5DGhptnNVbEGSvVn6Q609ZJaBkLjEPRJ... BUG=635419 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/e7ac1bbe93e087b173d503ca13b1e3b6f703b785 Cr-Commit-Position: refs/heads/master@{#418834} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/e7ac1bbe93e087b173d503ca13b1e3b6f703b785 Cr-Commit-Position: refs/heads/master@{#418834}
Message was sent while issue was closed.
Ericrk: Now that this CL has been landed, would you implement the contents of the OnMemoryStateChange method? Once it's done, we'll remeasure the memory impact of purge+suspend and move things forward to start a Finch experiment. Thanks! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
