Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2912753002:
Add gfx::GpuMemoryBuffer::shared_memory_handle() (Closed)
|
Created:
3 years, 6 months ago by hajimehoshi Modified:
3 years, 6 months ago CC:
chromium-reviews, erikchen, piman+watch_chromium.org, Primiano Tucci (use gerrit), ssid Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd gfx::GpuMemoryBuffer::shared_memory_handle()
This CL adds a base::SharedMemoryHandle getter to gfx::GpuMemoryBuffer.
The getter will be used by GpuMemoryBuffer 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=gpu_unittests --gtest_filter=GpuMemoryBufferImplSharedMemoryTest.*
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
Patch Set 1 #
Total comments: 3
Messages
Total messages: 19 (10 generated)
Description was changed from ========== Add gfx::GpuMemoryBuffer::shared_memory_handle() This CL adds a base::SharedMemoryHandle getter to gfx::GpuMemoryBuffer. 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. BUG=604726 TEST=gpu_unittests --gtest_filter=GpuMemoryBufferImplSharedMemoryTest.* ========== to ========== Add gfx::GpuMemoryBuffer::shared_memory_handle() This CL adds a base::SharedMemoryHandle getter to gfx::GpuMemoryBuffer. 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. BUG=604726 TEST=gpu_unittests --gtest_filter=GpuMemoryBufferImplSharedMemoryTest.* 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 ==========
Description was changed from ========== Add gfx::GpuMemoryBuffer::shared_memory_handle() This CL adds a base::SharedMemoryHandle getter to gfx::GpuMemoryBuffer. 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. BUG=604726 TEST=gpu_unittests --gtest_filter=GpuMemoryBufferImplSharedMemoryTest.* 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 ========== Add gfx::GpuMemoryBuffer::shared_memory_handle() This CL adds a base::SharedMemoryHandle getter to gfx::GpuMemoryBuffer. 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. BUG=604726 TEST=gpu_unittests --gtest_filter=GpuMemoryBufferImplSharedMemoryTest.* 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 ==========
hajimehoshi@chromium.org changed reviewers: + reveman@chromium.org
PTAL
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 gfx::GpuMemoryBuffer::shared_memory_handle() This CL adds a base::SharedMemoryHandle getter to gfx::GpuMemoryBuffer. 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. BUG=604726 TEST=gpu_unittests --gtest_filter=GpuMemoryBufferImplSharedMemoryTest.* 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 ========== Add gfx::GpuMemoryBuffer::shared_memory_handle() This CL adds a base::SharedMemoryHandle getter to gfx::GpuMemoryBuffer. 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=gpu_unittests --gtest_filter=GpuMemoryBufferImplSharedMemoryTest.* 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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Add gfx::GpuMemoryBuffer::shared_memory_handle() This CL adds a base::SharedMemoryHandle getter to gfx::GpuMemoryBuffer. 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=gpu_unittests --gtest_filter=GpuMemoryBufferImplSharedMemoryTest.* 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 ========== Add gfx::GpuMemoryBuffer::shared_memory_handle() This CL adds a base::SharedMemoryHandle getter to gfx::GpuMemoryBuffer. The getter will be used by GpuMemoryBuffer 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=gpu_unittests --gtest_filter=GpuMemoryBufferImplSharedMemoryTest.* 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 ==========
https://codereview.chromium.org/2912753002/diff/1/ui/gfx/gpu_memory_buffer.h File ui/gfx/gpu_memory_buffer.h (right): https://codereview.chromium.org/2912753002/diff/1/ui/gfx/gpu_memory_buffer.h#... ui/gfx/gpu_memory_buffer.h:67: virtual base::SharedMemoryHandle shared_memory_handle() const; I don't like to expose implementation specific functions in this API. Why is this needed?
ssid@chromium.org changed reviewers: + ssid@chromium.org
https://codereview.chromium.org/2912753002/diff/1/ui/gfx/gpu_memory_buffer.h File ui/gfx/gpu_memory_buffer.h (right): https://codereview.chromium.org/2912753002/diff/1/ui/gfx/gpu_memory_buffer.h#... ui/gfx/gpu_memory_buffer.h:67: virtual base::SharedMemoryHandle shared_memory_handle() const; On 2017/05/30 21:37:01, reveman wrote: > I don't like to expose implementation specific functions in this API. Why is > this needed? We now have a shared memory tracker which records memory usage in tracing. This method is required because GpuMemoryBufferClient::OnMemoryDump needs to know which of the shared memory segments in tracing is the buffer allocated from. The GpuMemoryBufferClient::OnMemoryDump will call SharedMemory::GetSegmentNameForTracing (to say that the backing for current segment has this id in trace). If not this method we would need something like GetSharedMemoryIdForTracing or similar, which would still be implementation specific, in sense that it will be specific to shared memory backing. Another idea that comes to my mind here is to have a function that says: // Called during tracing for creating tracing suballocation edges // to appropriate memory backing of the buffer. CreateSuballocationEdgesForTracing(MemoryAllocatorDumpGuid guid, int edge_importance); guid of the dump created by the buffer impl in OnMemoryDump, importance to specify if the current process owns the memory (refer ProcessMemoryDump::AddOwnershipEdge. This function creates edges instead of exposing the handle here.
ping reveman@
https://codereview.chromium.org/2912753002/diff/1/ui/gfx/gpu_memory_buffer.h File ui/gfx/gpu_memory_buffer.h (right): https://codereview.chromium.org/2912753002/diff/1/ui/gfx/gpu_memory_buffer.h#... ui/gfx/gpu_memory_buffer.h:67: virtual base::SharedMemoryHandle shared_memory_handle() const; On 2017/05/31 at 03:35:45, ssid wrote: > On 2017/05/30 21:37:01, reveman wrote: > > I don't like to expose implementation specific functions in this API. Why is > > this needed? > > We now have a shared memory tracker which records memory usage in tracing. This method is required because GpuMemoryBufferClient::OnMemoryDump needs to know which of the shared memory segments in tracing is the buffer allocated from. > The GpuMemoryBufferClient::OnMemoryDump will call SharedMemory::GetSegmentNameForTracing (to say that the backing for current segment has this id in trace). > If not this method we would need something like GetSharedMemoryIdForTracing or similar, which would still be implementation specific, in sense that it will be specific to shared memory backing. > > Another idea that comes to my mind here is to have a function that says: > // Called during tracing for creating tracing suballocation edges > // to appropriate memory backing of the buffer. > CreateSuballocationEdgesForTracing(MemoryAllocatorDumpGuid guid, int edge_importance); > guid of the dump created by the buffer impl in OnMemoryDump, importance to specify if the current process owns the memory (refer ProcessMemoryDump::AddOwnershipEdge. > > This function creates edges instead of exposing the handle here. I prefer this latter approach as it's something that can potentially be used by other implementation types. Most importantly, I like it as it avoids adding functions that allow the client to determine what type an instance is of as that's easily abused and results in code that is hard to maintain and change in the future. If it could also be combined with the GetGUIDForTracing function below someone then even better.
On 2017/06/01 14:13:45, reveman wrote: > https://codereview.chromium.org/2912753002/diff/1/ui/gfx/gpu_memory_buffer.h > File ui/gfx/gpu_memory_buffer.h (right): > > https://codereview.chromium.org/2912753002/diff/1/ui/gfx/gpu_memory_buffer.h#... > ui/gfx/gpu_memory_buffer.h:67: virtual base::SharedMemoryHandle > shared_memory_handle() const; > On 2017/05/31 at 03:35:45, ssid wrote: > > On 2017/05/30 21:37:01, reveman wrote: > > > I don't like to expose implementation specific functions in this API. Why is > > > this needed? > > > > We now have a shared memory tracker which records memory usage in tracing. > This method is required because GpuMemoryBufferClient::OnMemoryDump needs to > know which of the shared memory segments in tracing is the buffer allocated > from. > > The GpuMemoryBufferClient::OnMemoryDump will call > SharedMemory::GetSegmentNameForTracing (to say that the backing for current > segment has this id in trace). > > If not this method we would need something like GetSharedMemoryIdForTracing or > similar, which would still be implementation specific, in sense that it will be > specific to shared memory backing. > > > > Another idea that comes to my mind here is to have a function that says: > > // Called during tracing for creating tracing suballocation edges > > // to appropriate memory backing of the buffer. > > CreateSuballocationEdgesForTracing(MemoryAllocatorDumpGuid guid, int > edge_importance); > > guid of the dump created by the buffer impl in OnMemoryDump, importance to > specify if the current process owns the memory (refer > ProcessMemoryDump::AddOwnershipEdge. > > > > This function creates edges instead of exposing the handle here. > > I prefer this latter approach as it's something that can potentially be used by > other implementation types. Most importantly, I like it as it avoids adding > functions that allow the client to determine what type an instance is of as > that's easily abused and results in code that is hard to maintain and change in > the future. > > If it could also be combined with the GetGUIDForTracing function below someone > then even better. Thank you for suggestion. Wouldn't it be possible to leave comment to note that the new shared_memory_handle is only for memory-infra dump only? I'd like to continue this way since we have already done the same way in other components like TransferBufferInterface. If not, I'll follow your suggestion.
On 2017/06/01 at 16:34:42, hajimehoshi wrote: > On 2017/06/01 14:13:45, reveman wrote: > > https://codereview.chromium.org/2912753002/diff/1/ui/gfx/gpu_memory_buffer.h > > File ui/gfx/gpu_memory_buffer.h (right): > > > > https://codereview.chromium.org/2912753002/diff/1/ui/gfx/gpu_memory_buffer.h#... > > ui/gfx/gpu_memory_buffer.h:67: virtual base::SharedMemoryHandle > > shared_memory_handle() const; > > On 2017/05/31 at 03:35:45, ssid wrote: > > > On 2017/05/30 21:37:01, reveman wrote: > > > > I don't like to expose implementation specific functions in this API. Why is > > > > this needed? > > > > > > We now have a shared memory tracker which records memory usage in tracing. > > This method is required because GpuMemoryBufferClient::OnMemoryDump needs to > > know which of the shared memory segments in tracing is the buffer allocated > > from. > > > The GpuMemoryBufferClient::OnMemoryDump will call > > SharedMemory::GetSegmentNameForTracing (to say that the backing for current > > segment has this id in trace). > > > If not this method we would need something like GetSharedMemoryIdForTracing or > > similar, which would still be implementation specific, in sense that it will be > > specific to shared memory backing. > > > > > > Another idea that comes to my mind here is to have a function that says: > > > // Called during tracing for creating tracing suballocation edges > > > // to appropriate memory backing of the buffer. > > > CreateSuballocationEdgesForTracing(MemoryAllocatorDumpGuid guid, int > > edge_importance); > > > guid of the dump created by the buffer impl in OnMemoryDump, importance to > > specify if the current process owns the memory (refer > > ProcessMemoryDump::AddOwnershipEdge. > > > > > > This function creates edges instead of exposing the handle here. > > > > I prefer this latter approach as it's something that can potentially be used by > > other implementation types. Most importantly, I like it as it avoids adding > > functions that allow the client to determine what type an instance is of as > > that's easily abused and results in code that is hard to maintain and change in > > the future. > > > > If it could also be combined with the GetGUIDForTracing function below someone > > then even better. > > Thank you for suggestion. Wouldn't it be possible to leave comment to note that the new shared_memory_handle is only for memory-infra dump only? I'd like to continue this way since we have already done the same way in other components like TransferBufferInterface. If not, I'll follow your suggestion. I think the difference compared to other cases is that they are shared memory only and this interface is not. Shared memory is just what one implementation happen to use and by exposing it this way we're leaking details about a specific implementation that users of the API should not be aware of. Please adjust this to avoid that.
On 2017/06/01 19:38:27, reveman wrote: > On 2017/06/01 at 16:34:42, hajimehoshi wrote: > > On 2017/06/01 14:13:45, reveman wrote: > > > https://codereview.chromium.org/2912753002/diff/1/ui/gfx/gpu_memory_buffer.h > > > File ui/gfx/gpu_memory_buffer.h (right): > > > > > > > https://codereview.chromium.org/2912753002/diff/1/ui/gfx/gpu_memory_buffer.h#... > > > ui/gfx/gpu_memory_buffer.h:67: virtual base::SharedMemoryHandle > > > shared_memory_handle() const; > > > On 2017/05/31 at 03:35:45, ssid wrote: > > > > On 2017/05/30 21:37:01, reveman wrote: > > > > > I don't like to expose implementation specific functions in this API. > Why is > > > > > this needed? > > > > > > > > We now have a shared memory tracker which records memory usage in tracing. > > > This method is required because GpuMemoryBufferClient::OnMemoryDump needs to > > > know which of the shared memory segments in tracing is the buffer allocated > > > from. > > > > The GpuMemoryBufferClient::OnMemoryDump will call > > > SharedMemory::GetSegmentNameForTracing (to say that the backing for current > > > segment has this id in trace). > > > > If not this method we would need something like > GetSharedMemoryIdForTracing or > > > similar, which would still be implementation specific, in sense that it will > be > > > specific to shared memory backing. > > > > > > > > Another idea that comes to my mind here is to have a function that says: > > > > // Called during tracing for creating tracing suballocation edges > > > > // to appropriate memory backing of the buffer. > > > > CreateSuballocationEdgesForTracing(MemoryAllocatorDumpGuid guid, int > > > edge_importance); > > > > guid of the dump created by the buffer impl in OnMemoryDump, importance to > > > specify if the current process owns the memory (refer > > > ProcessMemoryDump::AddOwnershipEdge. > > > > > > > > This function creates edges instead of exposing the handle here. > > > > > > I prefer this latter approach as it's something that can potentially be used > by > > > other implementation types. Most importantly, I like it as it avoids adding > > > functions that allow the client to determine what type an instance is of as > > > that's easily abused and results in code that is hard to maintain and change > in > > > the future. > > > > > > If it could also be combined with the GetGUIDForTracing function below > someone > > > then even better. > > > > Thank you for suggestion. Wouldn't it be possible to leave comment to note > that the new shared_memory_handle is only for memory-infra dump only? I'd like > to continue this way since we have already done the same way in other components > like TransferBufferInterface. If not, I'll follow your suggestion. > > I think the difference compared to other cases is that they are shared memory > only and this interface is not. Shared memory is just what one implementation > happen to use and by exposing it this way we're leaking details about a specific > implementation that users of the API should not be aware of. Please adjust this > to avoid that. Actually they might not be base::SharedMemory and this depends on context. For example, TransferBuffer's back-end is gpu::Buffer, and gpu::Buffer's implementation might be base::SharedMemory or native gpu memory.
On 2017/06/02 04:42:22, hajimehoshi wrote: > On 2017/06/01 19:38:27, reveman wrote: > > On 2017/06/01 at 16:34:42, hajimehoshi wrote: > > > On 2017/06/01 14:13:45, reveman wrote: > > > > > https://codereview.chromium.org/2912753002/diff/1/ui/gfx/gpu_memory_buffer.h > > > > File ui/gfx/gpu_memory_buffer.h (right): > > > > > > > > > > > https://codereview.chromium.org/2912753002/diff/1/ui/gfx/gpu_memory_buffer.h#... > > > > ui/gfx/gpu_memory_buffer.h:67: virtual base::SharedMemoryHandle > > > > shared_memory_handle() const; > > > > On 2017/05/31 at 03:35:45, ssid wrote: > > > > > On 2017/05/30 21:37:01, reveman wrote: > > > > > > I don't like to expose implementation specific functions in this API. > > Why is > > > > > > this needed? > > > > > > > > > > We now have a shared memory tracker which records memory usage in > tracing. > > > > This method is required because GpuMemoryBufferClient::OnMemoryDump needs > to > > > > know which of the shared memory segments in tracing is the buffer > allocated > > > > from. > > > > > The GpuMemoryBufferClient::OnMemoryDump will call > > > > SharedMemory::GetSegmentNameForTracing (to say that the backing for > current > > > > segment has this id in trace). > > > > > If not this method we would need something like > > GetSharedMemoryIdForTracing or > > > > similar, which would still be implementation specific, in sense that it > will > > be > > > > specific to shared memory backing. > > > > > > > > > > Another idea that comes to my mind here is to have a function that says: > > > > > // Called during tracing for creating tracing suballocation edges > > > > > // to appropriate memory backing of the buffer. > > > > > CreateSuballocationEdgesForTracing(MemoryAllocatorDumpGuid guid, int > > > > edge_importance); > > > > > guid of the dump created by the buffer impl in OnMemoryDump, importance > to > > > > specify if the current process owns the memory (refer > > > > ProcessMemoryDump::AddOwnershipEdge. > > > > > > > > > > This function creates edges instead of exposing the handle here. > > > > > > > > I prefer this latter approach as it's something that can potentially be > used > > by > > > > other implementation types. Most importantly, I like it as it avoids > adding > > > > functions that allow the client to determine what type an instance is of > as > > > > that's easily abused and results in code that is hard to maintain and > change > > in > > > > the future. > > > > > > > > If it could also be combined with the GetGUIDForTracing function below > > someone > > > > then even better. > > > > > > Thank you for suggestion. Wouldn't it be possible to leave comment to note > > that the new shared_memory_handle is only for memory-infra dump only? I'd like > > to continue this way since we have already done the same way in other > components > > like TransferBufferInterface. If not, I'll follow your suggestion. > > > > I think the difference compared to other cases is that they are shared memory > > only and this interface is not. Shared memory is just what one implementation > > happen to use and by exposing it this way we're leaking details about a > specific > > implementation that users of the API should not be aware of. Please adjust > this > > to avoid that. > > Actually they might not be base::SharedMemory and this depends on context. For > example, TransferBuffer's back-end is gpu::Buffer, and gpu::Buffer's > implementation might be base::SharedMemory or native gpu memory. On second thought, I found that base::SharedMemoryHandle is already available via GetHandle() that returns GpuMemoryBufferHandle, so this CL is not needed anyway so far. Sorry for disturbing and thank you for discussing. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
