Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2909873002:
Add cc::SharedBitmap::shared_memory_handle() (Closed)
|
Created:
3 years, 6 months ago by hajimehoshi Modified:
3 years, 6 months ago CC:
chromium-reviews, rjkroege, chromium-apps-reviews_chromium.org, cc-bugs_chromium.org, extensions-reviews_chromium.org, erikchen, ssid, Primiano Tucci (use gerrit) Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd cc::SharedBitmap::shared_memory_handle()
This CL adds a base::SharedMemoryHandle getter to cc::SharedBitmap. The
getter will be used by SharedBitmap users when OnMemoryDump is called to
create base::SharedMemory dumps.
This is part of work for exposing SharedMemory usages on tracing
memory-infra dumps.
Design doc: https://docs.google.com/document/d/16Mi5_puxKgQ-9IX7ANfbji0WiqBCK3yrnJ0Qi2wa3w0/edit#heading=h.lofdeze1a2hr
BUG=604726
TEST=components_unittests --gtest_filter=HostSharedBitmapManagerTest.SharedMemoryHandle
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2909873002
Cr-Commit-Position: refs/heads/master@{#477704}
Committed: https://chromium.googlesource.com/chromium/src/+/f4c8be6f9795e33e6f7478dd4ef9d7755d4e14d0
Patch Set 1 #
Total comments: 12
Patch Set 2 : Address on reviews #
Total comments: 2
Patch Set 3 : Address on danakj's review #Patch Set 4 : fix tests #
Messages
Total messages: 37 (21 generated)
Description was changed from ========== Add cc::SharedBitmap::shared_memory_handle() This CL adds a base::SharedMemoryHandle getter to cc::SharedBitmap. The getter will be used by SharedBitmap users when OnMemoryDump is called. The base::SharedMemoryHandle to dump base::SharedMemory usages on memory-infra correctly. Design doc: https://docs.google.com/document/d/16Mi5_puxKgQ-9IX7ANfbji0WiqBCK3yrnJ0Qi2wa3... BUG=604726 TEST=components_unittests --gtest_filter=HostSharedBitmapManagerTest.SharedMemoryHandle ========== to ========== Add cc::SharedBitmap::shared_memory_handle() This CL adds a base::SharedMemoryHandle getter to cc::SharedBitmap. The getter will be used by SharedBitmap users when OnMemoryDump is called. The base::SharedMemoryHandle to dump base::SharedMemory usages on memory-infra correctly. Design doc: https://docs.google.com/document/d/16Mi5_puxKgQ-9IX7ANfbji0WiqBCK3yrnJ0Qi2wa3... BUG=604726 TEST=components_unittests --gtest_filter=HostSharedBitmapManagerTest.SharedMemoryHandle CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Patchset #1 (id:1) has been deleted
The CQ bit was checked by hajimehoshi@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...
Description was changed from ========== Add cc::SharedBitmap::shared_memory_handle() This CL adds a base::SharedMemoryHandle getter to cc::SharedBitmap. The getter will be used by SharedBitmap users when OnMemoryDump is called. The base::SharedMemoryHandle to dump base::SharedMemory usages on memory-infra correctly. Design doc: https://docs.google.com/document/d/16Mi5_puxKgQ-9IX7ANfbji0WiqBCK3yrnJ0Qi2wa3... BUG=604726 TEST=components_unittests --gtest_filter=HostSharedBitmapManagerTest.SharedMemoryHandle CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Add cc::SharedBitmap::shared_memory_handle() This CL adds a base::SharedMemoryHandle getter to cc::SharedBitmap. The getter will be used by SharedBitmap users when OnMemoryDump is called to create base::SharedMemory dumps. This is part of work for exposing SharedMemory usages on memory-infra dumps. Design doc: https://docs.google.com/document/d/16Mi5_puxKgQ-9IX7ANfbji0WiqBCK3yrnJ0Qi2wa3... BUG=604726 TEST=components_unittests --gtest_filter=HostSharedBitmapManagerTest.SharedMemoryHandle CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
hajimehoshi@chromium.org changed reviewers: + danakj@chromium.org, sky@chromium.org
PTAL sky@: service/ui danakj@: cc and components/viz
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add cc::SharedBitmap::shared_memory_handle() This CL adds a base::SharedMemoryHandle getter to cc::SharedBitmap. The getter will be used by SharedBitmap users when OnMemoryDump is called to create base::SharedMemory dumps. This is part of work for exposing SharedMemory usages on memory-infra dumps. Design doc: https://docs.google.com/document/d/16Mi5_puxKgQ-9IX7ANfbji0WiqBCK3yrnJ0Qi2wa3... BUG=604726 TEST=components_unittests --gtest_filter=HostSharedBitmapManagerTest.SharedMemoryHandle CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Add cc::SharedBitmap::shared_memory_handle() This CL adds a base::SharedMemoryHandle getter to cc::SharedBitmap. The getter will be used by SharedBitmap users when OnMemoryDump is called to create base::SharedMemory dumps. This is part of work for exposing SharedMemory usages on tracing memory-infra dumps. Design doc: https://docs.google.com/document/d/16Mi5_puxKgQ-9IX7ANfbji0WiqBCK3yrnJ0Qi2wa3... BUG=604726 TEST=components_unittests --gtest_filter=HostSharedBitmapManagerTest.SharedMemoryHandle CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
LGTM
https://codereview.chromium.org/2909873002/diff/20001/cc/resources/shared_bit... File cc/resources/shared_bitmap.h (right): https://codereview.chromium.org/2909873002/diff/20001/cc/resources/shared_bit... cc/resources/shared_bitmap.h:18: nit: remove whitespace https://codereview.chromium.org/2909873002/diff/20001/cc/resources/shared_bit... cc/resources/shared_bitmap.h:20: nit: remove whitespace https://codereview.chromium.org/2909873002/diff/20001/cc/resources/shared_bit... cc/resources/shared_bitmap.h:21: } // namespace base nit: no need for this comment on such a short block https://codereview.chromium.org/2909873002/diff/20001/cc/resources/shared_bit... cc/resources/shared_bitmap.h:41: virtual base::SharedMemoryHandle shared_memory_handle() const; Who will call this? You only implement this for the host subclass, so will the caller not know that the type is HostSharedBitmap? https://codereview.chromium.org/2909873002/diff/20001/components/viz/display_... File components/viz/display_compositor/host_shared_bitmap_manager.cc (right): https://codereview.chromium.org/2909873002/diff/20001/components/viz/display_... components/viz/display_compositor/host_shared_bitmap_manager.cc:56: if (!bitmap_data_->memory) When can this be null?
Thank you! https://codereview.chromium.org/2909873002/diff/20001/cc/resources/shared_bit... File cc/resources/shared_bitmap.h (right): https://codereview.chromium.org/2909873002/diff/20001/cc/resources/shared_bit... cc/resources/shared_bitmap.h:18: On 2017/05/31 16:10:31, danakj wrote: > nit: remove whitespace Done. https://codereview.chromium.org/2909873002/diff/20001/cc/resources/shared_bit... cc/resources/shared_bitmap.h:20: On 2017/05/31 16:10:31, danakj wrote: > nit: remove whitespace Done. https://codereview.chromium.org/2909873002/diff/20001/cc/resources/shared_bit... cc/resources/shared_bitmap.h:21: } // namespace base On 2017/05/31 16:10:31, danakj wrote: > nit: no need for this comment on such a short block Done. https://codereview.chromium.org/2909873002/diff/20001/cc/resources/shared_bit... cc/resources/shared_bitmap.h:41: virtual base::SharedMemoryHandle shared_memory_handle() const; On 2017/05/31 16:10:31, danakj wrote: > Who will call this? You only implement this for the host subclass, so will the > caller not know that the type is HostSharedBitmap? cc::ResourceProvider::Resource will call this. In cc::ResourceProvider::OnMemoryDump, when a resource type is RESOURCE_TYPE_BITMAP, resource's shared bitmap can be obtained but there is no way to know if this is HostSharedBitmap or not. https://codereview.chromium.org/2909873002/diff/20001/components/viz/display_... File components/viz/display_compositor/host_shared_bitmap_manager.cc (right): https://codereview.chromium.org/2909873002/diff/20001/components/viz/display_... components/viz/display_compositor/host_shared_bitmap_manager.cc:56: if (!bitmap_data_->memory) On 2017/05/31 16:10:31, danakj wrote: > When can this be null? When this is created from HostSharedBitmapManager::AllocateSharedBitmap: the comment says that "Bitmaps allocated in host don't need to be shared to other processes, so allocate them with new instead.".
https://codereview.chromium.org/2909873002/diff/20001/cc/resources/shared_bit... File cc/resources/shared_bitmap.h (right): https://codereview.chromium.org/2909873002/diff/20001/cc/resources/shared_bit... cc/resources/shared_bitmap.h:41: virtual base::SharedMemoryHandle shared_memory_handle() const; On 2017/06/01 04:47:58, hajimehoshi wrote: > On 2017/05/31 16:10:31, danakj wrote: > > Who will call this? You only implement this for the host subclass, so will the > > caller not know that the type is HostSharedBitmap? > > cc::ResourceProvider::Resource will call this. In > cc::ResourceProvider::OnMemoryDump, when a resource type is > RESOURCE_TYPE_BITMAP, resource's shared bitmap can be obtained but there is no > way to know if this is HostSharedBitmap or not. Then why is there no override for ChildSharedBitmap as well? Do you only want to count memory in the browser process?
On 2017/06/01 13:25:49, danakj wrote: > https://codereview.chromium.org/2909873002/diff/20001/cc/resources/shared_bit... > File cc/resources/shared_bitmap.h (right): > > https://codereview.chromium.org/2909873002/diff/20001/cc/resources/shared_bit... > cc/resources/shared_bitmap.h:41: virtual base::SharedMemoryHandle > shared_memory_handle() const; > On 2017/06/01 04:47:58, hajimehoshi wrote: > > On 2017/05/31 16:10:31, danakj wrote: > > > Who will call this? You only implement this for the host subclass, so will > the > > > caller not know that the type is HostSharedBitmap? > > > > cc::ResourceProvider::Resource will call this. In > > cc::ResourceProvider::OnMemoryDump, when a resource type is > > RESOURCE_TYPE_BITMAP, resource's shared bitmap can be obtained but there is no > > way to know if this is HostSharedBitmap or not. > > Then why is there no override for ChildSharedBitmap as well? Do you only want to > count memory in the browser process? Ah, you're right. I missed to do that. I'll update this soon.
https://codereview.chromium.org/2909873002/diff/20001/cc/resources/shared_bit... File cc/resources/shared_bitmap.h (right): https://codereview.chromium.org/2909873002/diff/20001/cc/resources/shared_bit... cc/resources/shared_bitmap.h:41: virtual base::SharedMemoryHandle shared_memory_handle() const; On 2017/06/01 13:25:49, danakj wrote: > On 2017/06/01 04:47:58, hajimehoshi wrote: > > On 2017/05/31 16:10:31, danakj wrote: > > > Who will call this? You only implement this for the host subclass, so will > the > > > caller not know that the type is HostSharedBitmap? > > > > cc::ResourceProvider::Resource will call this. In > > cc::ResourceProvider::OnMemoryDump, when a resource type is > > RESOURCE_TYPE_BITMAP, resource's shared bitmap can be obtained but there is no > > way to know if this is HostSharedBitmap or not. > > Then why is there no override for ChildSharedBitmap as well? Do you only want to > count memory in the browser process? Can you make this pure virtual? That would force us to not miss the subclass.
On 2017/06/01 16:29:32, danakj wrote: > https://codereview.chromium.org/2909873002/diff/20001/cc/resources/shared_bit... > File cc/resources/shared_bitmap.h (right): > > https://codereview.chromium.org/2909873002/diff/20001/cc/resources/shared_bit... > cc/resources/shared_bitmap.h:41: virtual base::SharedMemoryHandle > shared_memory_handle() const; > On 2017/06/01 13:25:49, danakj wrote: > > On 2017/06/01 04:47:58, hajimehoshi wrote: > > > On 2017/05/31 16:10:31, danakj wrote: > > > > Who will call this? You only implement this for the host subclass, so will > > the > > > > caller not know that the type is HostSharedBitmap? > > > > > > cc::ResourceProvider::Resource will call this. In > > > cc::ResourceProvider::OnMemoryDump, when a resource type is > > > RESOURCE_TYPE_BITMAP, resource's shared bitmap can be obtained but there is > no > > > way to know if this is HostSharedBitmap or not. > > > > Then why is there no override for ChildSharedBitmap as well? Do you only want > to > > count memory in the browser process? > > Can you make this pure virtual? That would force us to not miss the subclass. It looks like SharedBitmap is used at test_shared_bitmap_manager directly.
On 2017/06/01 13:25:49, danakj wrote: > https://codereview.chromium.org/2909873002/diff/20001/cc/resources/shared_bit... > File cc/resources/shared_bitmap.h (right): > > https://codereview.chromium.org/2909873002/diff/20001/cc/resources/shared_bit... > cc/resources/shared_bitmap.h:41: virtual base::SharedMemoryHandle > shared_memory_handle() const; > On 2017/06/01 04:47:58, hajimehoshi wrote: > > On 2017/05/31 16:10:31, danakj wrote: > > > Who will call this? You only implement this for the host subclass, so will > the > > > caller not know that the type is HostSharedBitmap? > > > > cc::ResourceProvider::Resource will call this. In > > cc::ResourceProvider::OnMemoryDump, when a resource type is > > RESOURCE_TYPE_BITMAP, resource's shared bitmap can be obtained but there is no > > way to know if this is HostSharedBitmap or not. > > Then why is there no override for ChildSharedBitmap as well? Do you only want to > count memory in the browser process? Huh, ChildSharedBitmap::shared_memory_handle is overridden in this CL, right?
On 2017/06/02 06:00:43, hajimehoshi wrote: > On 2017/06/01 13:25:49, danakj wrote: > > > https://codereview.chromium.org/2909873002/diff/20001/cc/resources/shared_bit... > > File cc/resources/shared_bitmap.h (right): > > > > > https://codereview.chromium.org/2909873002/diff/20001/cc/resources/shared_bit... > > cc/resources/shared_bitmap.h:41: virtual base::SharedMemoryHandle > > shared_memory_handle() const; > > On 2017/06/01 04:47:58, hajimehoshi wrote: > > > On 2017/05/31 16:10:31, danakj wrote: > > > > Who will call this? You only implement this for the host subclass, so will > > the > > > > caller not know that the type is HostSharedBitmap? > > > > > > cc::ResourceProvider::Resource will call this. In > > > cc::ResourceProvider::OnMemoryDump, when a resource type is > > > RESOURCE_TYPE_BITMAP, resource's shared bitmap can be obtained but there is > no > > > way to know if this is HostSharedBitmap or not. > > > > Then why is there no override for ChildSharedBitmap as well? Do you only want > to > > count memory in the browser process? > > Huh, ChildSharedBitmap::shared_memory_handle is overridden in this CL, right? ping danakj@
> Huh, ChildSharedBitmap::shared_memory_handle is overridden in this CL, right? Oh, you're right it is, but it's not tested? https://codereview.chromium.org/2909873002/diff/40001/cc/resources/shared_bit... File cc/resources/shared_bitmap.h (right): https://codereview.chromium.org/2909873002/diff/40001/cc/resources/shared_bit... cc/resources/shared_bitmap.h:39: virtual base::SharedMemoryHandle shared_memory_handle() const; virtual functions should be CamelCase. I also didn't get why this can't be pure virtual? You would just have test subclasses implement it?
Thank you! https://codereview.chromium.org/2909873002/diff/40001/cc/resources/shared_bit... File cc/resources/shared_bitmap.h (right): https://codereview.chromium.org/2909873002/diff/40001/cc/resources/shared_bit... cc/resources/shared_bitmap.h:39: virtual base::SharedMemoryHandle shared_memory_handle() const; On 2017/06/06 16:03:47, danakj wrote: > virtual functions should be CamelCase. I also didn't get why this can't be pure > virtual? You would just have test subclasses implement it? Done.
On 2017/06/06 16:03:47, danakj wrote: > > Huh, ChildSharedBitmap::shared_memory_handle is overridden in this CL, right? > > Oh, you're right it is, but it's not tested? As it is very hard to make ChildSharedBitmapManager testable, can we leave this without tests in this CL? ChildSharedBitmapManager's constructor takes cc::mojom::SharedBitmapManagerAssociatedPtr, and I have no idea how to make a mock of this.
The CQ bit was checked by hajimehoshi@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: ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by hajimehoshi@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.
Ah, yeh I don't know either, you'd have to make a mojo service for the host I think. LGTM
The CQ bit was checked by hajimehoshi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/2909873002/#ps80001 (title: "fix tests")
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": 1496858622169430,
"parent_rev": "f6475be15cb108ef9eb62bd85809f366885b08c3", "commit_rev":
"f4c8be6f9795e33e6f7478dd4ef9d7755d4e14d0"}
Message was sent while issue was closed.
Description was changed from ========== Add cc::SharedBitmap::shared_memory_handle() This CL adds a base::SharedMemoryHandle getter to cc::SharedBitmap. The getter will be used by SharedBitmap users when OnMemoryDump is called to create base::SharedMemory dumps. This is part of work for exposing SharedMemory usages on tracing memory-infra dumps. Design doc: https://docs.google.com/document/d/16Mi5_puxKgQ-9IX7ANfbji0WiqBCK3yrnJ0Qi2wa3... BUG=604726 TEST=components_unittests --gtest_filter=HostSharedBitmapManagerTest.SharedMemoryHandle CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Add cc::SharedBitmap::shared_memory_handle() This CL adds a base::SharedMemoryHandle getter to cc::SharedBitmap. The getter will be used by SharedBitmap users when OnMemoryDump is called to create base::SharedMemory dumps. This is part of work for exposing SharedMemory usages on tracing memory-infra dumps. Design doc: https://docs.google.com/document/d/16Mi5_puxKgQ-9IX7ANfbji0WiqBCK3yrnJ0Qi2wa3... BUG=604726 TEST=components_unittests --gtest_filter=HostSharedBitmapManagerTest.SharedMemoryHandle CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2909873002 Cr-Commit-Position: refs/heads/master@{#477704} Committed: https://chromium.googlesource.com/chromium/src/+/f4c8be6f9795e33e6f7478dd4ef9... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/f4c8be6f9795e33e6f7478dd4ef9... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
