Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2775423003:
Add ownership edges between HostSharedBitmap and shared memory
|
Created:
3 years, 8 months ago by hajimehoshi Modified:
3 years, 8 months ago CC:
chromium-reviews, vmpstr+watch_chromium.org, wfh+watch_chromium.org, jam, gavinp+memory_chromium.org, darin-cc_chromium.org, tracing+reviews_chromium.org, cc-bugs_chromium.org, erikchen Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd ownership edges between HostSharedBitmap and shared memory
This CL adds ownership edges between memory dumps of modules that use
shared memory and shared memory dump which was introduced at
crrev.com/2654073002. This replaces existing
CreateSharedGlobalAllocatorDump calls. This makes 'effective' size of
shared-memory-related parts on memory-infra accurate.
This CL is part of crrev.com/2535213002/ and there are still other
modules that require shared-memory ownership edges.
Design doc: https://docs.google.com/document/d/16Mi5_puxKgQ-9IX7ANfbji0WiqBCK3yrnJ0Qi2wa3w0/edit#
BUG=604726
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Patch Set 1 #
Total comments: 1
Patch Set 2 : Add test files #Patch Set 3 : Fix compile errors #
Total comments: 2
Patch Set 4 : Remove unneeded dumps #Patch Set 5 : Add null checks for bitmaps #
Total comments: 3
Patch Set 6 : Fix comments #
Messages
Total messages: 53 (29 generated)
Description was changed from ========== Add ownership edges for shared memory This CL adds ownership edges between memory dumps of modules that use shared memory and shared memory dump which was introduced at crrev.com/2654073002. This replaces existing CreateSharedGlobalAllocatorDump calls. This makes 'effective' size of shared-memory-related parts on memory-infra accurate. This CL is part of crrev.com/2535213002/ and there are still other modules that require ownership edges. Design doc: https://docs.google.com/document/d/16Mi5_puxKgQ-9IX7ANfbji0WiqBCK3yrnJ0Qi2wa3... BUG=604726 ========== to ========== Add ownership edges for shared memory This CL adds ownership edges between memory dumps of modules that use shared memory and shared memory dump which was introduced at crrev.com/2654073002. This replaces existing CreateSharedGlobalAllocatorDump calls. This makes 'effective' size of shared-memory-related parts on memory-infra accurate. This CL is part of crrev.com/2535213002/ and there are still other modules that require ownership edges. Design doc: https://docs.google.com/document/d/16Mi5_puxKgQ-9IX7ANfbji0WiqBCK3yrnJ0Qi2wa3... BUG=604726 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
hajimehoshi@chromium.org changed reviewers: + primiano@chromium.org, ssid@chromium.org
PTAL https://codereview.chromium.org/2775423003/diff/1/content/common/host_shared_... File content/common/host_shared_bitmap_manager.cc (left): https://codereview.chromium.org/2775423003/diff/1/content/common/host_shared_... content/common/host_shared_bitmap_manager.cc:179: pmd->CreateSharedGlobalAllocatorDump(guid); There are still CreateSharedGlobalAllocatorDump calls. I'm not sure it is allowed to mix CreateSharedGlobalAllocatorDump usages and my new SharedMemoryTracker::AddOwnershipEdges.
Description was changed from ========== Add ownership edges for shared memory This CL adds ownership edges between memory dumps of modules that use shared memory and shared memory dump which was introduced at crrev.com/2654073002. This replaces existing CreateSharedGlobalAllocatorDump calls. This makes 'effective' size of shared-memory-related parts on memory-infra accurate. This CL is part of crrev.com/2535213002/ and there are still other modules that require ownership edges. Design doc: https://docs.google.com/document/d/16Mi5_puxKgQ-9IX7ANfbji0WiqBCK3yrnJ0Qi2wa3... BUG=604726 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Add ownership edges for shared memory This CL adds ownership edges between memory dumps of modules that use shared memory and shared memory dump which was introduced at crrev.com/2654073002. This replaces existing CreateSharedGlobalAllocatorDump calls. This makes 'effective' size of shared-memory-related parts on memory-infra accurate. This CL is part of crrev.com/2535213002/ and there are still other modules that require shared-memory ownership edges. Design doc: https://docs.google.com/document/d/16Mi5_puxKgQ-9IX7ANfbji0WiqBCK3yrnJ0Qi2wa3... BUG=604726 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
ping
ssid: thoughts? hajime: does the final result (trace link?) still make sense? is memory still reported correctly to cc/? Did you and ssid sort out all the discussion about ownership edges? https://codereview.chromium.org/2775423003/diff/40001/base/memory/shared_memo... File base/memory/shared_memory_tracker.cc (right): https://codereview.chromium.org/2775423003/diff/40001/base/memory/shared_memo... base/memory/shared_memory_tracker.cc:65: StringPrintf("shared_memory_in_process/%s", id_str.c_str()); why not just "shared_memory"?
https://codereview.chromium.org/2775423003/diff/40001/base/memory/shared_memo... File base/memory/shared_memory_tracker.cc (right): https://codereview.chromium.org/2775423003/diff/40001/base/memory/shared_memo... base/memory/shared_memory_tracker.cc:65: StringPrintf("shared_memory_in_process/%s", id_str.c_str()); On 2017/03/29 17:19:50, Primiano Tucci wrote: > why not just "shared_memory"? I thought this dump is necessary but I was wrong. Removed 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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
On 2017/03/29 17:19:50, Primiano Tucci wrote: > ssid: thoughts? > hajime: does the final result (trace link?) still make sense? is memory still > reported correctly to cc/? Did you and ssid sort out all the discussion about > ownership edges? ssid@ and I have discussed on the doc and agreed the design about ownership edges. I'll attach a trace data, but before doing this, I need to investigate why try-bots are not happy. Thank you for waiting.
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 ownership edges for shared memory This CL adds ownership edges between memory dumps of modules that use shared memory and shared memory dump which was introduced at crrev.com/2654073002. This replaces existing CreateSharedGlobalAllocatorDump calls. This makes 'effective' size of shared-memory-related parts on memory-infra accurate. This CL is part of crrev.com/2535213002/ and there are still other modules that require shared-memory ownership edges. Design doc: https://docs.google.com/document/d/16Mi5_puxKgQ-9IX7ANfbji0WiqBCK3yrnJ0Qi2wa3... BUG=604726 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Add ownership edges between HostSharedBitmap and shared memory This CL adds ownership edges between memory dumps of modules that use shared memory and shared memory dump which was introduced at crrev.com/2654073002. This replaces existing CreateSharedGlobalAllocatorDump calls. This makes 'effective' size of shared-memory-related parts on memory-infra accurate. This CL is part of crrev.com/2535213002/ and there are still other modules that require shared-memory ownership edges. Design doc: https://docs.google.com/document/d/16Mi5_puxKgQ-9IX7ANfbji0WiqBCK3yrnJ0Qi2wa3... BUG=604726 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
On 2017/03/30 10:21:51, hajimehoshi wrote: > On 2017/03/29 17:19:50, Primiano Tucci wrote: > > ssid: thoughts? > > hajime: does the final result (trace link?) still make sense? is memory still > > reported correctly to cc/? Did you and ssid sort out all the discussion about > > ownership edges? > > ssid@ and I have discussed on the doc and agreed the design about ownership > edges. > > I'll attach a trace data, but before doing this, I need to investigate why > try-bots are not happy. Thank you for waiting. Trace dump example: https://drive.google.com/file/d/0BwW8PrCcts4WWXRKZzBqaFJTeVU/view?usp=sharing HostSharedBitmap should be dumped with a name sharedbitmap/..., but I couldn't find any of them. Am I missing something?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On Thu, Mar 30, 2017 at 7:53 AM, <hajimehoshi@chromium.org> wrote: > On 2017/03/30 10:21:51, hajimehoshi wrote: > > On 2017/03/29 17:19:50, Primiano Tucci wrote: > > > ssid: thoughts? > > > hajime: does the final result (trace link?) still make sense? is memory > still > > > reported correctly to cc/? Did you and ssid sort out all the discussion > about > > > ownership edges? > > > > ssid@ and I have discussed on the doc and agreed the design about > ownership > > edges. > > > > I'll attach a trace data, but before doing this, I need to investigate > why > > try-bots are not happy. Thank you for waiting. > > > Trace dump example: > > https://drive.google.com/file/d/0BwW8PrCcts4WWXRKZzBqaFJTeVU/ > view?usp=sharing > > HostSharedBitmap should be dumped with a name sharedbitmap/..., but I > couldn't > find any of them. Am I missing something? > Try --disable-gpu ? -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Can we start this with introducing a boolean in the SharedMemory constructor that says |current_process_keeps_memory_alive|. This would mean that the global dump edge for that segment will have higher importance in the process. +Erik who wanted to do similar change for tracking which process should take ownership of the shared_memory. https://codereview.chromium.org/2775423003/diff/80001/content/common/host_sha... File content/common/host_shared_bitmap_manager.cc (right): https://codereview.chromium.org/2775423003/diff/80001/content/common/host_sha... content/common/host_shared_bitmap_manager.cc:170: if (!dump) I think you can remove this condition. https://codereview.chromium.org/2775423003/diff/80001/content/common/host_sha... content/common/host_shared_bitmap_manager.cc:177: if (bitmap.second->memory) { When would the shared_memory field be null? What happens before this change id it was null? We still dumped the memory and added edges? I am thinking if you should move this condition to the beginning of the loop and not create the "sharedbitmap/xxx" at all since it will not have any edge and the actual memory backing does not exist. https://codereview.chromium.org/2775423003/diff/80001/content/common/host_sha... content/common/host_shared_bitmap_manager.cc:179: pmd, dump->guid(), bitmap.second->memory->handle()); You have updated only the browser side dumps to use the shared_memory edges. But, the "cc/resource_manager/X" dumps will still create global dump edges which will match the id given by cc::GetSharedBitmapGUIDForTracing(). I think the right order is first being able to detect if the memory should be attributes to the current process or not. Then move all the clients of shared_memory at once. Otherwise, we will end up with a situation where the memory is attributed equally to all processes sharing the segment. This would still break telemetry.
On 2017/03/31 00:25:21, ssid wrote: > Can we start this with introducing a boolean in the SharedMemory constructor > that says |current_process_keeps_memory_alive|. This would mean that the global > dump edge for that segment will have higher importance in the process. > > [...] > > I think the right order is first being able to detect if the memory should be > attributes to the current process or not. Then move all the clients of > shared_memory at once. Otherwise, we will end up with a situation where the > memory is attributed equally to all processes sharing the segment. This would > still break telemetry. I'm not sure why it is needed to detect if the memory is owned by the current process. Isn't it enough to simply replace the current ownership code that indicates importance at the caller side? I agree that we need to replace all the client ownership at once for consistent results on telemetry.
On 2017/03/30 18:51:26, danakj wrote: > On Thu, Mar 30, 2017 at 7:53 AM, <mailto:hajimehoshi@chromium.org> wrote: > > > On 2017/03/30 10:21:51, hajimehoshi wrote: > > > On 2017/03/29 17:19:50, Primiano Tucci wrote: > > > > ssid: thoughts? > > > > hajime: does the final result (trace link?) still make sense? is memory > > still > > > > reported correctly to cc/? Did you and ssid sort out all the discussion > > about > > > > ownership edges? > > > > > > ssid@ and I have discussed on the doc and agreed the design about > > ownership > > > edges. > > > > > > I'll attach a trace data, but before doing this, I need to investigate > > why > > > try-bots are not happy. Thank you for waiting. > > > > > > Trace dump example: > > > > https://drive.google.com/file/d/0BwW8PrCcts4WWXRKZzBqaFJTeVU/ > > view?usp=sharing > > > > HostSharedBitmap should be dumped with a name sharedbitmap/..., but I > > couldn't > > find any of them. Am I missing something? > > > > Try --disable-gpu ? This worked. Thank you! https://drive.google.com/file/d/0BwW8PrCcts4WcWNySEl3aUJxeE0/view?usp=sharing > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/03/31 06:43:21, hajimehoshi wrote: > On 2017/03/31 00:25:21, ssid wrote: > > Can we start this with introducing a boolean in the SharedMemory constructor > > that says |current_process_keeps_memory_alive|. This would mean that the > global > > dump edge for that segment will have higher importance in the process. > > > > [...] > > > > I think the right order is first being able to detect if the memory should be > > attributes to the current process or not. Then move all the clients of > > shared_memory at once. Otherwise, we will end up with a situation where the > > memory is attributed equally to all processes sharing the segment. This would > > still break telemetry. > > I'm not sure why it is needed to detect if the memory is owned by the current > process. Isn't it enough to simply replace the current ownership code that > indicates importance at the caller side? > > I agree that we need to replace all the client ownership at once for consistent > results on telemetry. ping
On 2017/03/31 06:43:21, hajimehoshi wrote: > On 2017/03/31 00:25:21, ssid wrote: > > Can we start this with introducing a boolean in the SharedMemory constructor > > that says |current_process_keeps_memory_alive|. This would mean that the > global > > dump edge for that segment will have higher importance in the process. > > > > [...] > > > > I think the right order is first being able to detect if the memory should be > > attributes to the current process or not. Then move all the clients of > > shared_memory at once. Otherwise, we will end up with a situation where the > > memory is attributed equally to all processes sharing the segment. This would > > still break telemetry. > > I'm not sure why it is needed to detect if the memory is owned by the current > process. Isn't it enough to simply replace the current ownership code that > indicates importance at the caller side? > I was really thinking about the future all the time where we should have all the clients of shared memory specify ownership of the memory correctly instead of splitting equally across the processes. But, you are right about the importance edges in this CL. the edges are correct. > I agree that we need to replace all the client ownership at once for consistent > results on telemetry. Yes, so what is the plan here? Should we do a large CL that changes all the clients of shared memory to use these edges instead? The latest patch still has only shared bitmap switched to the new type of edge.
On 2017/04/04 06:36:54, ssid wrote: > On 2017/03/31 06:43:21, hajimehoshi wrote: > > On 2017/03/31 00:25:21, ssid wrote: > > > Can we start this with introducing a boolean in the SharedMemory constructor > > > that says |current_process_keeps_memory_alive|. This would mean that the > > global > > > dump edge for that segment will have higher importance in the process. > > > > > > [...] > > > > > > I think the right order is first being able to detect if the memory should > be > > > attributes to the current process or not. Then move all the clients of > > > shared_memory at once. Otherwise, we will end up with a situation where the > > > memory is attributed equally to all processes sharing the segment. This > would > > > still break telemetry. > > > > I'm not sure why it is needed to detect if the memory is owned by the current > > process. Isn't it enough to simply replace the current ownership code that > > indicates importance at the caller side? > > > I was really thinking about the future all the time where we should have all the > clients of shared memory specify ownership of the memory correctly instead of > splitting equally across the processes. > But, you are right about the importance edges in this CL. the edges are correct. Thanks! Your plan sounds nice, and IIUC this is a kind of refactoring and will be important in the future. I'd like to keep this CL's way for the current plan. > > > I agree that we need to replace all the client ownership at once for > consistent > > results on telemetry. > > Yes, so what is the plan here? Should we do a large CL that changes all the > clients of shared memory to use these edges instead? > The latest patch still has only shared bitmap switched to the new type of edge. Well, my current plan is to refine https://codereview.chromium.org/2535213002 for only POSIX, the CL size would not be so different though. First I thought of just adding SharedMemoryTracker::AddOwnershipEdges without callers, but adding a new function without callers wouldn't be really a good idea.
On 2017/04/04 10:12:55, hajimehoshi wrote: > On 2017/04/04 06:36:54, ssid wrote: > > On 2017/03/31 06:43:21, hajimehoshi wrote: > > > On 2017/03/31 00:25:21, ssid wrote: > > > > Can we start this with introducing a boolean in the SharedMemory > constructor > > > > that says |current_process_keeps_memory_alive|. This would mean that the > > > global > > > > dump edge for that segment will have higher importance in the process. > > > > > > > > [...] > > > > > > > > I think the right order is first being able to detect if the memory should > > be > > > > attributes to the current process or not. Then move all the clients of > > > > shared_memory at once. Otherwise, we will end up with a situation where > the > > > > memory is attributed equally to all processes sharing the segment. This > > would > > > > still break telemetry. > > > > > > I'm not sure why it is needed to detect if the memory is owned by the > current > > > process. Isn't it enough to simply replace the current ownership code that > > > indicates importance at the caller side? > > > > > I was really thinking about the future all the time where we should have all > the > > clients of shared memory specify ownership of the memory correctly instead of > > splitting equally across the processes. > > But, you are right about the importance edges in this CL. the edges are > correct. > > Thanks! Your plan sounds nice, and IIUC this is a kind of refactoring and will > be important in the future. I'd like to keep this CL's way for the current plan. > > > > > > I agree that we need to replace all the client ownership at once for > > consistent > > > results on telemetry. > > > > Yes, so what is the plan here? Should we do a large CL that changes all the > > clients of shared memory to use these edges instead? > > The latest patch still has only shared bitmap switched to the new type of > edge. > > Well, my current plan is to refine https://codereview.chromium.org/2535213002 > for only POSIX, the CL size would not be so different though. First I thought of > just adding SharedMemoryTracker::AddOwnershipEdges without callers, but adding a > new function without callers wouldn't be really a good idea. On second thought, I started to wonder if adding ownership edges only for HostSharedBitmapManager is really not feasible. As the shared memory for shared bitmap is used only at HostSharedBitmapManager, will this really break the other existing ownership edges ? The original big CL https://codereview.chromium.org/2535213002 is not finished yet and the CL size will be much bigger.
I don't see a no way to get around a big CL. Since it is just a mechanical change which does move all the global dump edges to SharedMemoryTracker::AddOwnershipEdge() it should be just fine. We cannot do the switch for each dump provider separately since we will break telemetry metrics. Maybe we could do a pass for all gpu providers then do a pass for discardable provider. I'd suggest talk to ericrk@ if you want to split the CL. He knows which gpu providers are connected by ownership edges better.
hajimehoshi@chromium.org changed reviewers: + ericrk@chromium.org
On 2017/04/04 18:04:28, ssid wrote: > I don't see a no way to get around a big CL. Since it is just a mechanical > change which does move all the global dump edges to > SharedMemoryTracker::AddOwnershipEdge() it should be just fine. > We cannot do the switch for each dump provider separately since we will break > telemetry metrics. Maybe we could do a pass for all gpu providers then do a pass > for discardable provider. > I'd suggest talk to ericrk@ if you want to split the CL. He knows which gpu > providers are connected by ownership edges better. Switching to the tracker at all clients is mostly done: https://codereview.chromium.org/2535213002/ only on POSIX. IMHO this is not so mechanical change because I needed more fix like exposing shared memory handle to the clients to get shared memory ID for tracing. I still want to split the CL. BTW, DiscardableSharedMemoryManager::OnMemoryDump doesn't use 'size' attribute and I have no idea how to fix this. https://cs.chromium.org/chromium/src/components/discardable_memory/service/di.... What is 'resident_size'? Is this different from 'size'? Our plan is to use 'size' attribute to calculate effective memory size, so how do I need to fix this? As for HostSharedBitmapManager, the guid is generated GetSharedBitmapGUIDForTracing and this is shared only with cc/resource/ResourceProvider. I think all clients can be separated into four groups: * GetSharedBitmapGUIForTracing: HostSharedBitmapManager, ResourceProvider (type=bitmap) * GetGpuMemoryBufferGUIDForTracing: StaginBufferPool, GpuMemoryBufferVideoFramePool, BrowserGpuMemoryBufferManager, ResourceProvider (type=gpu) * GetGenericSharedMemoryGUIDForTracing GLImageSharedMemory, GLImageIOSurface (Mac only) * GetBufferGUIDForTracing TransferBufferManager, CommandBufferHelper, MappedMemoryManager, GLES2Implementation +ericrk, we are trying to replace current ownership edges for tracing following https://docs.google.com/document/d/16Mi5_puxKgQ-9IX7ANfbji0WiqBCK3yrnJ0Qi2wa3... so that we can know shared memory usage correctly. We are trying to replace the GUID generators including GetGpuMemoryBufferGUIDForTracing with the unified ID generator for shared memory (SharedMemory::GetUniqueID). Is it possible to replace only GPU part (part that uses GetGpuMemoryBufferGUIDForTracing in the above list) and leave the other parts as they are?
On 2017/04/05 09:23:58, hajimehoshi wrote: > On 2017/04/04 18:04:28, ssid wrote: > > I don't see a no way to get around a big CL. Since it is just a mechanical > > change which does move all the global dump edges to > > SharedMemoryTracker::AddOwnershipEdge() it should be just fine. > > We cannot do the switch for each dump provider separately since we will break > > telemetry metrics. Maybe we could do a pass for all gpu providers then do a > pass > > for discardable provider. > > I'd suggest talk to ericrk@ if you want to split the CL. He knows which gpu > > providers are connected by ownership edges better. > > Switching to the tracker at all clients is mostly done: > https://codereview.chromium.org/2535213002/ only on POSIX. IMHO this is not so > mechanical change because I needed more fix like exposing shared memory handle > to the clients to get shared memory ID for tracing. I still want to split the > CL. > > BTW, DiscardableSharedMemoryManager::OnMemoryDump doesn't use 'size' attribute > and I have no idea how to fix this. > https://cs.chromium.org/chromium/src/components/discardable_memory/service/di.... > What is 'resident_size'? Is this different from 'size'? Our plan is to use > 'size' attribute to calculate effective memory size, so how do I need to fix > this? > > As for HostSharedBitmapManager, the guid is generated > GetSharedBitmapGUIDForTracing and this is shared only with > cc/resource/ResourceProvider. I think all clients can be separated into four > groups: > > * GetSharedBitmapGUIForTracing: > HostSharedBitmapManager, ResourceProvider (type=bitmap) > * GetGpuMemoryBufferGUIDForTracing: > StaginBufferPool, GpuMemoryBufferVideoFramePool, > BrowserGpuMemoryBufferManager, ResourceProvider (type=gpu) > * GetGenericSharedMemoryGUIDForTracing > GLImageSharedMemory, GLImageIOSurface (Mac only) > * GetBufferGUIDForTracing > TransferBufferManager, CommandBufferHelper, MappedMemoryManager, > GLES2Implementation > > +ericrk, we are trying to replace current ownership edges for tracing following > https://docs.google.com/document/d/16Mi5_puxKgQ-9IX7ANfbji0WiqBCK3yrnJ0Qi2wa3... > so that we can know shared memory usage correctly. We are trying to replace the > GUID generators including GetGpuMemoryBufferGUIDForTracing with the unified ID > generator for shared memory (SharedMemory::GetUniqueID). Is it possible to > replace only GPU part (part that uses GetGpuMemoryBufferGUIDForTracing in the > above list) and leave the other parts as they are? This plan may need a bit of tweaking to handle the GPUMemoryBuffer and GLImage cases. The first issue is that the IDs used for both these types must match - currently, GetGpuMemoryBufferGUIDForTracing uses GetGenericSharedMemoryGUIDForTracing. This is necessary, as we use these IDs to create ownership edges for GPUMemoryBuffers and GLImages which represent the same memory. Additionally, certain types of GPU Memory buffers (IOSurfaces), don't use Chrome "shared memory", but represent alternate system-specific shared memory concepts. Currently, they use the same set of IDs as shared memory GPUMemoyBuffers and GLImages. This isn't a hard requirement, and would probably have to change with the design you propose. I think that in order for your system to work, you probably need to split the GPUMemoryBuffer and GLImage IDs into two categories: Shared Memory GUIDs (as described in your doc) - used for GLImageSharedMemory and GPUMemoryBufferSharedMemory (will need to tweak BrowserGpuMemoryBufferManager to use a virtual method or switch GUID generation based on GPU Memory buffer type). GenericSharedGPUMemoryID - some new type used for IOSurfaces and other platform specific GPUMemoryBuffers - currently needs to be used for GLImageIOSurface and GPUMemoryBufferIOSurface. Let me know if this makes sense, and I'm happy to help with this. Thanks!
On 2017/04/11 17:08:29, ericrk wrote: > On 2017/04/05 09:23:58, hajimehoshi wrote: > > On 2017/04/04 18:04:28, ssid wrote: > > > I don't see a no way to get around a big CL. Since it is just a mechanical > > > change which does move all the global dump edges to > > > SharedMemoryTracker::AddOwnershipEdge() it should be just fine. > > > We cannot do the switch for each dump provider separately since we will > break > > > telemetry metrics. Maybe we could do a pass for all gpu providers then do a > > pass > > > for discardable provider. > > > I'd suggest talk to ericrk@ if you want to split the CL. He knows which gpu > > > providers are connected by ownership edges better. > > > > Switching to the tracker at all clients is mostly done: > > https://codereview.chromium.org/2535213002/ only on POSIX. IMHO this is not so > > mechanical change because I needed more fix like exposing shared memory handle > > to the clients to get shared memory ID for tracing. I still want to split the > > CL. > > > > BTW, DiscardableSharedMemoryManager::OnMemoryDump doesn't use 'size' attribute > > and I have no idea how to fix this. > > > https://cs.chromium.org/chromium/src/components/discardable_memory/service/di.... > > What is 'resident_size'? Is this different from 'size'? Our plan is to use > > 'size' attribute to calculate effective memory size, so how do I need to fix > > this? > > > > As for HostSharedBitmapManager, the guid is generated > > GetSharedBitmapGUIDForTracing and this is shared only with > > cc/resource/ResourceProvider. I think all clients can be separated into four > > groups: > > > > * GetSharedBitmapGUIForTracing: > > HostSharedBitmapManager, ResourceProvider (type=bitmap) > > * GetGpuMemoryBufferGUIDForTracing: > > StaginBufferPool, GpuMemoryBufferVideoFramePool, > > BrowserGpuMemoryBufferManager, ResourceProvider (type=gpu) > > * GetGenericSharedMemoryGUIDForTracing > > GLImageSharedMemory, GLImageIOSurface (Mac only) > > * GetBufferGUIDForTracing > > TransferBufferManager, CommandBufferHelper, MappedMemoryManager, > > GLES2Implementation > > > > +ericrk, we are trying to replace current ownership edges for tracing > following > > > https://docs.google.com/document/d/16Mi5_puxKgQ-9IX7ANfbji0WiqBCK3yrnJ0Qi2wa3... > > so that we can know shared memory usage correctly. We are trying to replace > the > > GUID generators including GetGpuMemoryBufferGUIDForTracing with the unified ID > > generator for shared memory (SharedMemory::GetUniqueID). Is it possible to > > replace only GPU part (part that uses GetGpuMemoryBufferGUIDForTracing in the > > above list) and leave the other parts as they are? > > This plan may need a bit of tweaking to handle the GPUMemoryBuffer and GLImage > cases. > > The first issue is that the IDs used for both these types must match - > currently, GetGpuMemoryBufferGUIDForTracing uses > GetGenericSharedMemoryGUIDForTracing. This is necessary, as we use these IDs to > create ownership edges for GPUMemoryBuffers and GLImages which represent the > same memory. > > Additionally, certain types of GPU Memory buffers (IOSurfaces), don't use Chrome > "shared memory", but represent alternate system-specific shared memory concepts. > Currently, they use the same set of IDs as shared memory GPUMemoyBuffers and > GLImages. This isn't a hard requirement, and would probably have to change with > the design you propose. > > I think that in order for your system to work, you probably need to split the > GPUMemoryBuffer and GLImage IDs into two categories: > > Shared Memory GUIDs (as described in your doc) - used for GLImageSharedMemory > and GPUMemoryBufferSharedMemory (will need to tweak > BrowserGpuMemoryBufferManager to use a virtual method or switch GUID generation > based on GPU Memory buffer type). > > GenericSharedGPUMemoryID - some new type used for IOSurfaces and other platform > specific GPUMemoryBuffers - currently needs to be used for GLImageIOSurface and > GPUMemoryBufferIOSurface. > > Let me know if this makes sense, and I'm happy to help with this. Thanks! Thank you, Eric! My understanding is: * Current GUIDs doesn't tell whether GpuMemoryBuffer, GLImage or other related objects use shared memory. * GetGenericSharedMemoryGUIDForTracing is used by GLImageSharedMemory and GLImageIOSurface * GetGpuMemoryBufferGUIDForTracing is used by GpuMemoryBuffer mainly and StagingBuffer, GpuMemoryBufferVideoFramePool::PlaneResource and ResourceProvider::Resource indirectly * All of them might or might not use shared memory. * Your suggest is to get both GetGenericSharedMemoryGUIDForTracing and GetGpuMemoryBufferGUIDForTracing together and create GetSharedMemoryGUIDForTracing and GetGenericSharedGPUMemoryGUIDFOrTracing Is this correct?
On 2017/04/13 08:39:03, hajimehoshi wrote: > On 2017/04/11 17:08:29, ericrk wrote: > > On 2017/04/05 09:23:58, hajimehoshi wrote: > > > On 2017/04/04 18:04:28, ssid wrote: > > > > I don't see a no way to get around a big CL. Since it is just a mechanical > > > > change which does move all the global dump edges to > > > > SharedMemoryTracker::AddOwnershipEdge() it should be just fine. > > > > We cannot do the switch for each dump provider separately since we will > > break > > > > telemetry metrics. Maybe we could do a pass for all gpu providers then do > a > > > pass > > > > for discardable provider. > > > > I'd suggest talk to ericrk@ if you want to split the CL. He knows which > gpu > > > > providers are connected by ownership edges better. > > > > > > Switching to the tracker at all clients is mostly done: > > > https://codereview.chromium.org/2535213002/ only on POSIX. IMHO this is not > so > > > mechanical change because I needed more fix like exposing shared memory > handle > > > to the clients to get shared memory ID for tracing. I still want to split > the > > > CL. > > > > > > BTW, DiscardableSharedMemoryManager::OnMemoryDump doesn't use 'size' > attribute > > > and I have no idea how to fix this. > > > > > > https://cs.chromium.org/chromium/src/components/discardable_memory/service/di.... > > > What is 'resident_size'? Is this different from 'size'? Our plan is to use > > > 'size' attribute to calculate effective memory size, so how do I need to fix > > > this? > > > > > > As for HostSharedBitmapManager, the guid is generated > > > GetSharedBitmapGUIDForTracing and this is shared only with > > > cc/resource/ResourceProvider. I think all clients can be separated into four > > > groups: > > > > > > * GetSharedBitmapGUIForTracing: > > > HostSharedBitmapManager, ResourceProvider (type=bitmap) > > > * GetGpuMemoryBufferGUIDForTracing: > > > StaginBufferPool, GpuMemoryBufferVideoFramePool, > > > BrowserGpuMemoryBufferManager, ResourceProvider (type=gpu) > > > * GetGenericSharedMemoryGUIDForTracing > > > GLImageSharedMemory, GLImageIOSurface (Mac only) > > > * GetBufferGUIDForTracing > > > TransferBufferManager, CommandBufferHelper, MappedMemoryManager, > > > GLES2Implementation > > > > > > +ericrk, we are trying to replace current ownership edges for tracing > > following > > > > > > https://docs.google.com/document/d/16Mi5_puxKgQ-9IX7ANfbji0WiqBCK3yrnJ0Qi2wa3... > > > so that we can know shared memory usage correctly. We are trying to replace > > the > > > GUID generators including GetGpuMemoryBufferGUIDForTracing with the unified > ID > > > generator for shared memory (SharedMemory::GetUniqueID). Is it possible to > > > replace only GPU part (part that uses GetGpuMemoryBufferGUIDForTracing in > the > > > above list) and leave the other parts as they are? > > > > This plan may need a bit of tweaking to handle the GPUMemoryBuffer and GLImage > > cases. > > > > The first issue is that the IDs used for both these types must match - > > currently, GetGpuMemoryBufferGUIDForTracing uses > > GetGenericSharedMemoryGUIDForTracing. This is necessary, as we use these IDs > to > > create ownership edges for GPUMemoryBuffers and GLImages which represent the > > same memory. > > > > Additionally, certain types of GPU Memory buffers (IOSurfaces), don't use > Chrome > > "shared memory", but represent alternate system-specific shared memory > concepts. > > Currently, they use the same set of IDs as shared memory GPUMemoyBuffers and > > GLImages. This isn't a hard requirement, and would probably have to change > with > > the design you propose. > > > > I think that in order for your system to work, you probably need to split the > > GPUMemoryBuffer and GLImage IDs into two categories: > > > > Shared Memory GUIDs (as described in your doc) - used for GLImageSharedMemory > > and GPUMemoryBufferSharedMemory (will need to tweak > > BrowserGpuMemoryBufferManager to use a virtual method or switch GUID > generation > > based on GPU Memory buffer type). > > > > GenericSharedGPUMemoryID - some new type used for IOSurfaces and other > platform > > specific GPUMemoryBuffers - currently needs to be used for GLImageIOSurface > and > > GPUMemoryBufferIOSurface. > > > > Let me know if this makes sense, and I'm happy to help with this. Thanks! > > Thank you, Eric! My understanding is: > > * Current GUIDs doesn't tell whether GpuMemoryBuffer, GLImage or other related > objects use shared memory. > * GetGenericSharedMemoryGUIDForTracing is used by GLImageSharedMemory and > GLImageIOSurface > * GetGpuMemoryBufferGUIDForTracing is used by GpuMemoryBuffer mainly and > StagingBuffer, GpuMemoryBufferVideoFramePool::PlaneResource and > ResourceProvider::Resource indirectly > * All of them might or might not use shared memory. > * Your suggest is to get both GetGenericSharedMemoryGUIDForTracing and > GetGpuMemoryBufferGUIDForTracing together and create > GetSharedMemoryGUIDForTracing and GetGenericSharedGPUMemoryGUIDFOrTracing > > Is this correct? Another question: GetGenericSharedMemoryGUIDForTracing takes the tracing process id, but why is this necessary? It looks like independent global dumps are created for each client, and there seems no shared dump between multiple processes. For example, in BrowserGpuMemoryBufferManager, CreateSharedGlobalAllocatorDump generates a global dump for a given id, but the ids are different among child processes. https://cs.chromium.org/chromium/src/content/browser/gpu/browser_gpu_memory_b... I thought dumps for shared objects or general shared things should have one ID and ownership edges must be created toward it from its client, but am I misunderstanding?
On 2017/04/13 12:08:24, hajimehoshi wrote: > On 2017/04/13 08:39:03, hajimehoshi wrote: > > On 2017/04/11 17:08:29, ericrk wrote: > > > On 2017/04/05 09:23:58, hajimehoshi wrote: > > > > On 2017/04/04 18:04:28, ssid wrote: > > > > > I don't see a no way to get around a big CL. Since it is just a > mechanical > > > > > change which does move all the global dump edges to > > > > > SharedMemoryTracker::AddOwnershipEdge() it should be just fine. > > > > > We cannot do the switch for each dump provider separately since we will > > > break > > > > > telemetry metrics. Maybe we could do a pass for all gpu providers then > do > > a > > > > pass > > > > > for discardable provider. > > > > > I'd suggest talk to ericrk@ if you want to split the CL. He knows which > > gpu > > > > > providers are connected by ownership edges better. > > > > > > > > Switching to the tracker at all clients is mostly done: > > > > https://codereview.chromium.org/2535213002/ only on POSIX. IMHO this is > not > > so > > > > mechanical change because I needed more fix like exposing shared memory > > handle > > > > to the clients to get shared memory ID for tracing. I still want to split > > the > > > > CL. > > > > > > > > BTW, DiscardableSharedMemoryManager::OnMemoryDump doesn't use 'size' > > attribute > > > > and I have no idea how to fix this. > > > > > > > > > > https://cs.chromium.org/chromium/src/components/discardable_memory/service/di.... > > > > What is 'resident_size'? Is this different from 'size'? Our plan is to use > > > > 'size' attribute to calculate effective memory size, so how do I need to > fix > > > > this? > > > > > > > > As for HostSharedBitmapManager, the guid is generated > > > > GetSharedBitmapGUIDForTracing and this is shared only with > > > > cc/resource/ResourceProvider. I think all clients can be separated into > four > > > > groups: > > > > > > > > * GetSharedBitmapGUIForTracing: > > > > HostSharedBitmapManager, ResourceProvider (type=bitmap) > > > > * GetGpuMemoryBufferGUIDForTracing: > > > > StaginBufferPool, GpuMemoryBufferVideoFramePool, > > > > BrowserGpuMemoryBufferManager, ResourceProvider (type=gpu) > > > > * GetGenericSharedMemoryGUIDForTracing > > > > GLImageSharedMemory, GLImageIOSurface (Mac only) > > > > * GetBufferGUIDForTracing > > > > TransferBufferManager, CommandBufferHelper, MappedMemoryManager, > > > > GLES2Implementation > > > > > > > > +ericrk, we are trying to replace current ownership edges for tracing > > > following > > > > > > > > > > https://docs.google.com/document/d/16Mi5_puxKgQ-9IX7ANfbji0WiqBCK3yrnJ0Qi2wa3... > > > > so that we can know shared memory usage correctly. We are trying to > replace > > > the > > > > GUID generators including GetGpuMemoryBufferGUIDForTracing with the > unified > > ID > > > > generator for shared memory (SharedMemory::GetUniqueID). Is it possible to > > > > replace only GPU part (part that uses GetGpuMemoryBufferGUIDForTracing in > > the > > > > above list) and leave the other parts as they are? > > > > > > This plan may need a bit of tweaking to handle the GPUMemoryBuffer and > GLImage > > > cases. > > > > > > The first issue is that the IDs used for both these types must match - > > > currently, GetGpuMemoryBufferGUIDForTracing uses > > > GetGenericSharedMemoryGUIDForTracing. This is necessary, as we use these IDs > > to > > > create ownership edges for GPUMemoryBuffers and GLImages which represent the > > > same memory. > > > > > > Additionally, certain types of GPU Memory buffers (IOSurfaces), don't use > > Chrome > > > "shared memory", but represent alternate system-specific shared memory > > concepts. > > > Currently, they use the same set of IDs as shared memory GPUMemoyBuffers and > > > GLImages. This isn't a hard requirement, and would probably have to change > > with > > > the design you propose. > > > > > > I think that in order for your system to work, you probably need to split > the > > > GPUMemoryBuffer and GLImage IDs into two categories: > > > > > > Shared Memory GUIDs (as described in your doc) - used for > GLImageSharedMemory > > > and GPUMemoryBufferSharedMemory (will need to tweak > > > BrowserGpuMemoryBufferManager to use a virtual method or switch GUID > > generation > > > based on GPU Memory buffer type). > > > > > > GenericSharedGPUMemoryID - some new type used for IOSurfaces and other > > platform > > > specific GPUMemoryBuffers - currently needs to be used for GLImageIOSurface > > and > > > GPUMemoryBufferIOSurface. > > > > > > Let me know if this makes sense, and I'm happy to help with this. Thanks! > > > > Thank you, Eric! My understanding is: > > > > * Current GUIDs doesn't tell whether GpuMemoryBuffer, GLImage or other related > > objects use shared memory. > > * GetGenericSharedMemoryGUIDForTracing is used by GLImageSharedMemory and > > GLImageIOSurface > > * GetGpuMemoryBufferGUIDForTracing is used by GpuMemoryBuffer mainly and > > StagingBuffer, GpuMemoryBufferVideoFramePool::PlaneResource and > > ResourceProvider::Resource indirectly > > * All of them might or might not use shared memory. > > * Your suggest is to get both GetGenericSharedMemoryGUIDForTracing and > > GetGpuMemoryBufferGUIDForTracing together and create > > GetSharedMemoryGUIDForTracing and GetGenericSharedGPUMemoryGUIDFOrTracing > > > > Is this correct? Yup, that's my understanding - we need one set of IDs for shared memory (which is what you describe in your doc), and one set for other things which GPU Memory Buffers / GL Images use interchangeably with shared memory. > > Another question: GetGenericSharedMemoryGUIDForTracing takes the tracing process > id, but why is this necessary? It looks like independent global dumps are > created for each client, and there seems no shared dump between multiple > processes. > > For example, in BrowserGpuMemoryBufferManager, CreateSharedGlobalAllocatorDump > generates a global dump for a given id, but the ids are different among child > processes. > https://cs.chromium.org/chromium/src/content/browser/gpu/browser_gpu_memory_b... > I thought dumps for shared objects or general shared things should have one ID > and ownership edges must be created toward it from its client, but am I > misunderstanding? Taking the shared memory case (ignoring IOsurfaces): The BrowserGpuMemoryBufferManager, ResourceProvider, and GLImageSharedMemory should all end up generating the same GUID for the global dump - they all use the tracing process ID of the client (renderer), and they should all share the same buffer_id. We then add an ownership edges from the dump to the shared GUID. (there is currently a be a bug in the GLImageSharedMemory part of this. I'm taking a look now, but I've confirmed that everything works as expected if the bug is fixed).
On 2017/04/13 18:31:53, ericrk wrote: > On 2017/04/13 12:08:24, hajimehoshi wrote: > > On 2017/04/13 08:39:03, hajimehoshi wrote: > > > On 2017/04/11 17:08:29, ericrk wrote: > > > > On 2017/04/05 09:23:58, hajimehoshi wrote: > > > > > On 2017/04/04 18:04:28, ssid wrote: > > > > > > I don't see a no way to get around a big CL. Since it is just a > > mechanical > > > > > > change which does move all the global dump edges to > > > > > > SharedMemoryTracker::AddOwnershipEdge() it should be just fine. > > > > > > We cannot do the switch for each dump provider separately since we > will > > > > break > > > > > > telemetry metrics. Maybe we could do a pass for all gpu providers then > > do > > > a > > > > > pass > > > > > > for discardable provider. > > > > > > I'd suggest talk to ericrk@ if you want to split the CL. He knows > which > > > gpu > > > > > > providers are connected by ownership edges better. > > > > > > > > > > Switching to the tracker at all clients is mostly done: > > > > > https://codereview.chromium.org/2535213002/ only on POSIX. IMHO this is > > not > > > so > > > > > mechanical change because I needed more fix like exposing shared memory > > > handle > > > > > to the clients to get shared memory ID for tracing. I still want to > split > > > the > > > > > CL. > > > > > > > > > > BTW, DiscardableSharedMemoryManager::OnMemoryDump doesn't use 'size' > > > attribute > > > > > and I have no idea how to fix this. > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/components/discardable_memory/service/di.... > > > > > What is 'resident_size'? Is this different from 'size'? Our plan is to > use > > > > > 'size' attribute to calculate effective memory size, so how do I need to > > fix > > > > > this? > > > > > > > > > > As for HostSharedBitmapManager, the guid is generated > > > > > GetSharedBitmapGUIDForTracing and this is shared only with > > > > > cc/resource/ResourceProvider. I think all clients can be separated into > > four > > > > > groups: > > > > > > > > > > * GetSharedBitmapGUIForTracing: > > > > > HostSharedBitmapManager, ResourceProvider (type=bitmap) > > > > > * GetGpuMemoryBufferGUIDForTracing: > > > > > StaginBufferPool, GpuMemoryBufferVideoFramePool, > > > > > BrowserGpuMemoryBufferManager, ResourceProvider (type=gpu) > > > > > * GetGenericSharedMemoryGUIDForTracing > > > > > GLImageSharedMemory, GLImageIOSurface (Mac only) > > > > > * GetBufferGUIDForTracing > > > > > TransferBufferManager, CommandBufferHelper, MappedMemoryManager, > > > > > GLES2Implementation > > > > > > > > > > +ericrk, we are trying to replace current ownership edges for tracing > > > > following > > > > > > > > > > > > > > > https://docs.google.com/document/d/16Mi5_puxKgQ-9IX7ANfbji0WiqBCK3yrnJ0Qi2wa3... > > > > > so that we can know shared memory usage correctly. We are trying to > > replace > > > > the > > > > > GUID generators including GetGpuMemoryBufferGUIDForTracing with the > > unified > > > ID > > > > > generator for shared memory (SharedMemory::GetUniqueID). Is it possible > to > > > > > replace only GPU part (part that uses GetGpuMemoryBufferGUIDForTracing > in > > > the > > > > > above list) and leave the other parts as they are? > > > > > > > > This plan may need a bit of tweaking to handle the GPUMemoryBuffer and > > GLImage > > > > cases. > > > > > > > > The first issue is that the IDs used for both these types must match - > > > > currently, GetGpuMemoryBufferGUIDForTracing uses > > > > GetGenericSharedMemoryGUIDForTracing. This is necessary, as we use these > IDs > > > to > > > > create ownership edges for GPUMemoryBuffers and GLImages which represent > the > > > > same memory. > > > > > > > > Additionally, certain types of GPU Memory buffers (IOSurfaces), don't use > > > Chrome > > > > "shared memory", but represent alternate system-specific shared memory > > > concepts. > > > > Currently, they use the same set of IDs as shared memory GPUMemoyBuffers > and > > > > GLImages. This isn't a hard requirement, and would probably have to change > > > with > > > > the design you propose. > > > > > > > > I think that in order for your system to work, you probably need to split > > the > > > > GPUMemoryBuffer and GLImage IDs into two categories: > > > > > > > > Shared Memory GUIDs (as described in your doc) - used for > > GLImageSharedMemory > > > > and GPUMemoryBufferSharedMemory (will need to tweak > > > > BrowserGpuMemoryBufferManager to use a virtual method or switch GUID > > > generation > > > > based on GPU Memory buffer type). > > > > > > > > GenericSharedGPUMemoryID - some new type used for IOSurfaces and other > > > platform > > > > specific GPUMemoryBuffers - currently needs to be used for > GLImageIOSurface > > > and > > > > GPUMemoryBufferIOSurface. > > > > > > > > Let me know if this makes sense, and I'm happy to help with this. Thanks! > > > > > > Thank you, Eric! My understanding is: > > > > > > * Current GUIDs doesn't tell whether GpuMemoryBuffer, GLImage or other > related > > > objects use shared memory. > > > * GetGenericSharedMemoryGUIDForTracing is used by GLImageSharedMemory and > > > GLImageIOSurface > > > * GetGpuMemoryBufferGUIDForTracing is used by GpuMemoryBuffer mainly and > > > StagingBuffer, GpuMemoryBufferVideoFramePool::PlaneResource and > > > ResourceProvider::Resource indirectly > > > * All of them might or might not use shared memory. > > > * Your suggest is to get both GetGenericSharedMemoryGUIDForTracing and > > > GetGpuMemoryBufferGUIDForTracing together and create > > > GetSharedMemoryGUIDForTracing and GetGenericSharedGPUMemoryGUIDFOrTracing > > > > > > Is this correct? > Yup, that's my understanding - we need one set of IDs for shared memory (which > is what you describe in your doc), and one set for other things which GPU Memory > Buffers / GL Images use interchangeably with shared memory. > > > > > Another question: GetGenericSharedMemoryGUIDForTracing takes the tracing > process > > id, but why is this necessary? It looks like independent global dumps are > > created for each client, and there seems no shared dump between multiple > > processes. > > > > For example, in BrowserGpuMemoryBufferManager, CreateSharedGlobalAllocatorDump > > generates a global dump for a given id, but the ids are different among child > > processes. > > > https://cs.chromium.org/chromium/src/content/browser/gpu/browser_gpu_memory_b... > > I thought dumps for shared objects or general shared things should have one ID > > and ownership edges must be created toward it from its client, but am I > > misunderstanding? > > Taking the shared memory case (ignoring IOsurfaces): > > The BrowserGpuMemoryBufferManager, ResourceProvider, and GLImageSharedMemory > should all end up generating the same GUID for the global dump - they all use > the tracing process ID of the client (renderer), and they should all share > the same buffer_id. We then add an ownership edges from the dump to the > shared GUID. (there is currently a be a bug in the GLImageSharedMemory part > of this. I'm taking a look now, but I've confirmed that > everything works as expected if the bug is fixed). Yep, if shared-memory clients are in the same process, the global guid is same. My question is, "the global guid would be different for each client process, and is this correct?". Isn't there a dump that is shared among multiple processes in this case?
On 2017/04/14 08:11:10, hajimehoshi wrote: > On 2017/04/13 18:31:53, ericrk wrote: > > On 2017/04/13 12:08:24, hajimehoshi wrote: > > > On 2017/04/13 08:39:03, hajimehoshi wrote: > > > > On 2017/04/11 17:08:29, ericrk wrote: > > > > > On 2017/04/05 09:23:58, hajimehoshi wrote: > > > > > > On 2017/04/04 18:04:28, ssid wrote: > > > > > > > I don't see a no way to get around a big CL. Since it is just a > > > mechanical > > > > > > > change which does move all the global dump edges to > > > > > > > SharedMemoryTracker::AddOwnershipEdge() it should be just fine. > > > > > > > We cannot do the switch for each dump provider separately since we > > will > > > > > break > > > > > > > telemetry metrics. Maybe we could do a pass for all gpu providers > then > > > do > > > > a > > > > > > pass > > > > > > > for discardable provider. > > > > > > > I'd suggest talk to ericrk@ if you want to split the CL. He knows > > which > > > > gpu > > > > > > > providers are connected by ownership edges better. > > > > > > > > > > > > Switching to the tracker at all clients is mostly done: > > > > > > https://codereview.chromium.org/2535213002/ only on POSIX. IMHO this > is > > > not > > > > so > > > > > > mechanical change because I needed more fix like exposing shared > memory > > > > handle > > > > > > to the clients to get shared memory ID for tracing. I still want to > > split > > > > the > > > > > > CL. > > > > > > > > > > > > BTW, DiscardableSharedMemoryManager::OnMemoryDump doesn't use 'size' > > > > attribute > > > > > > and I have no idea how to fix this. > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/components/discardable_memory/service/di.... > > > > > > What is 'resident_size'? Is this different from 'size'? Our plan is to > > use > > > > > > 'size' attribute to calculate effective memory size, so how do I need > to > > > fix > > > > > > this? > > > > > > > > > > > > As for HostSharedBitmapManager, the guid is generated > > > > > > GetSharedBitmapGUIDForTracing and this is shared only with > > > > > > cc/resource/ResourceProvider. I think all clients can be separated > into > > > four > > > > > > groups: > > > > > > > > > > > > * GetSharedBitmapGUIForTracing: > > > > > > HostSharedBitmapManager, ResourceProvider (type=bitmap) > > > > > > * GetGpuMemoryBufferGUIDForTracing: > > > > > > StaginBufferPool, GpuMemoryBufferVideoFramePool, > > > > > > BrowserGpuMemoryBufferManager, ResourceProvider (type=gpu) > > > > > > * GetGenericSharedMemoryGUIDForTracing > > > > > > GLImageSharedMemory, GLImageIOSurface (Mac only) > > > > > > * GetBufferGUIDForTracing > > > > > > TransferBufferManager, CommandBufferHelper, MappedMemoryManager, > > > > > > GLES2Implementation > > > > > > > > > > > > +ericrk, we are trying to replace current ownership edges for tracing > > > > > following > > > > > > > > > > > > > > > > > > > > > https://docs.google.com/document/d/16Mi5_puxKgQ-9IX7ANfbji0WiqBCK3yrnJ0Qi2wa3... > > > > > > so that we can know shared memory usage correctly. We are trying to > > > replace > > > > > the > > > > > > GUID generators including GetGpuMemoryBufferGUIDForTracing with the > > > unified > > > > ID > > > > > > generator for shared memory (SharedMemory::GetUniqueID). Is it > possible > > to > > > > > > replace only GPU part (part that uses GetGpuMemoryBufferGUIDForTracing > > in > > > > the > > > > > > above list) and leave the other parts as they are? > > > > > > > > > > This plan may need a bit of tweaking to handle the GPUMemoryBuffer and > > > GLImage > > > > > cases. > > > > > > > > > > The first issue is that the IDs used for both these types must match - > > > > > currently, GetGpuMemoryBufferGUIDForTracing uses > > > > > GetGenericSharedMemoryGUIDForTracing. This is necessary, as we use these > > IDs > > > > to > > > > > create ownership edges for GPUMemoryBuffers and GLImages which represent > > the > > > > > same memory. > > > > > > > > > > Additionally, certain types of GPU Memory buffers (IOSurfaces), don't > use > > > > Chrome > > > > > "shared memory", but represent alternate system-specific shared memory > > > > concepts. > > > > > Currently, they use the same set of IDs as shared memory GPUMemoyBuffers > > and > > > > > GLImages. This isn't a hard requirement, and would probably have to > change > > > > with > > > > > the design you propose. > > > > > > > > > > I think that in order for your system to work, you probably need to > split > > > the > > > > > GPUMemoryBuffer and GLImage IDs into two categories: > > > > > > > > > > Shared Memory GUIDs (as described in your doc) - used for > > > GLImageSharedMemory > > > > > and GPUMemoryBufferSharedMemory (will need to tweak > > > > > BrowserGpuMemoryBufferManager to use a virtual method or switch GUID > > > > generation > > > > > based on GPU Memory buffer type). > > > > > > > > > > GenericSharedGPUMemoryID - some new type used for IOSurfaces and other > > > > platform > > > > > specific GPUMemoryBuffers - currently needs to be used for > > GLImageIOSurface > > > > and > > > > > GPUMemoryBufferIOSurface. > > > > > > > > > > Let me know if this makes sense, and I'm happy to help with this. > Thanks! > > > > > > > > Thank you, Eric! My understanding is: > > > > > > > > * Current GUIDs doesn't tell whether GpuMemoryBuffer, GLImage or other > > related > > > > objects use shared memory. > > > > * GetGenericSharedMemoryGUIDForTracing is used by GLImageSharedMemory and > > > > GLImageIOSurface > > > > * GetGpuMemoryBufferGUIDForTracing is used by GpuMemoryBuffer mainly and > > > > StagingBuffer, GpuMemoryBufferVideoFramePool::PlaneResource and > > > > ResourceProvider::Resource indirectly > > > > * All of them might or might not use shared memory. > > > > * Your suggest is to get both GetGenericSharedMemoryGUIDForTracing and > > > > GetGpuMemoryBufferGUIDForTracing together and create > > > > GetSharedMemoryGUIDForTracing and GetGenericSharedGPUMemoryGUIDFOrTracing > > > > > > > > Is this correct? > > Yup, that's my understanding - we need one set of IDs for shared memory (which > > is what you describe in your doc), and one set for other things which GPU > Memory > > Buffers / GL Images use interchangeably with shared memory. > > > > > > > > Another question: GetGenericSharedMemoryGUIDForTracing takes the tracing > > process > > > id, but why is this necessary? It looks like independent global dumps are > > > created for each client, and there seems no shared dump between multiple > > > processes. > > > > > > For example, in BrowserGpuMemoryBufferManager, > CreateSharedGlobalAllocatorDump > > > generates a global dump for a given id, but the ids are different among > child > > > processes. > > > > > > https://cs.chromium.org/chromium/src/content/browser/gpu/browser_gpu_memory_b... > > > I thought dumps for shared objects or general shared things should have one > ID > > > and ownership edges must be created toward it from its client, but am I > > > misunderstanding? > > > > Taking the shared memory case (ignoring IOsurfaces): > > > > The BrowserGpuMemoryBufferManager, ResourceProvider, and GLImageSharedMemory > > should all end up generating the same GUID for the global dump - they all use > > the tracing process ID of the client (renderer), and they should all share > > the same buffer_id. We then add an ownership edges from the dump to the > > shared GUID. (there is currently a be a bug in the GLImageSharedMemory part > > of this. I'm taking a look now, but I've confirmed that > > everything works as expected if the bug is fixed). > > Yep, if shared-memory clients are in the same process, the global guid is same. > My question is, "the global guid would be different for each client process, and > is this correct?". Isn't there a dump that is shared among multiple processes in > this case? The tracing process ID we're using isn't the current process ID, but the ID of the allocating client. So in this case, we do end up with the same GUID in multiple processes. We can discuss more on chat if that would help? However, looking at things, I think that the buffer IDs may be GUIDs already, so this extra ID may be unnecessary (although I think it is currently working as expected). If you agree that the buffer IDs are already globally unique, feel free to drop the client tracing ID.
On 2017/04/17 17:55:34, ericrk wrote: > On 2017/04/14 08:11:10, hajimehoshi wrote: > > On 2017/04/13 18:31:53, ericrk wrote: > > > On 2017/04/13 12:08:24, hajimehoshi wrote: > > > > On 2017/04/13 08:39:03, hajimehoshi wrote: > > > > > On 2017/04/11 17:08:29, ericrk wrote: > > > > > > On 2017/04/05 09:23:58, hajimehoshi wrote: > > > > > > > On 2017/04/04 18:04:28, ssid wrote: > > > > > > > > I don't see a no way to get around a big CL. Since it is just a > > > > mechanical > > > > > > > > change which does move all the global dump edges to > > > > > > > > SharedMemoryTracker::AddOwnershipEdge() it should be just fine. > > > > > > > > We cannot do the switch for each dump provider separately since we > > > will > > > > > > break > > > > > > > > telemetry metrics. Maybe we could do a pass for all gpu providers > > then > > > > do > > > > > a > > > > > > > pass > > > > > > > > for discardable provider. > > > > > > > > I'd suggest talk to ericrk@ if you want to split the CL. He knows > > > which > > > > > gpu > > > > > > > > providers are connected by ownership edges better. > > > > > > > > > > > > > > Switching to the tracker at all clients is mostly done: > > > > > > > https://codereview.chromium.org/2535213002/ only on POSIX. IMHO this > > is > > > > not > > > > > so > > > > > > > mechanical change because I needed more fix like exposing shared > > memory > > > > > handle > > > > > > > to the clients to get shared memory ID for tracing. I still want to > > > split > > > > > the > > > > > > > CL. > > > > > > > > > > > > > > BTW, DiscardableSharedMemoryManager::OnMemoryDump doesn't use 'size' > > > > > attribute > > > > > > > and I have no idea how to fix this. > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/components/discardable_memory/service/di.... > > > > > > > What is 'resident_size'? Is this different from 'size'? Our plan is > to > > > use > > > > > > > 'size' attribute to calculate effective memory size, so how do I > need > > to > > > > fix > > > > > > > this? > > > > > > > > > > > > > > As for HostSharedBitmapManager, the guid is generated > > > > > > > GetSharedBitmapGUIDForTracing and this is shared only with > > > > > > > cc/resource/ResourceProvider. I think all clients can be separated > > into > > > > four > > > > > > > groups: > > > > > > > > > > > > > > * GetSharedBitmapGUIForTracing: > > > > > > > HostSharedBitmapManager, ResourceProvider (type=bitmap) > > > > > > > * GetGpuMemoryBufferGUIDForTracing: > > > > > > > StaginBufferPool, GpuMemoryBufferVideoFramePool, > > > > > > > BrowserGpuMemoryBufferManager, ResourceProvider (type=gpu) > > > > > > > * GetGenericSharedMemoryGUIDForTracing > > > > > > > GLImageSharedMemory, GLImageIOSurface (Mac only) > > > > > > > * GetBufferGUIDForTracing > > > > > > > TransferBufferManager, CommandBufferHelper, MappedMemoryManager, > > > > > > > GLES2Implementation > > > > > > > > > > > > > > +ericrk, we are trying to replace current ownership edges for > tracing > > > > > > following > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://docs.google.com/document/d/16Mi5_puxKgQ-9IX7ANfbji0WiqBCK3yrnJ0Qi2wa3... > > > > > > > so that we can know shared memory usage correctly. We are trying to > > > > replace > > > > > > the > > > > > > > GUID generators including GetGpuMemoryBufferGUIDForTracing with the > > > > unified > > > > > ID > > > > > > > generator for shared memory (SharedMemory::GetUniqueID). Is it > > possible > > > to > > > > > > > replace only GPU part (part that uses > GetGpuMemoryBufferGUIDForTracing > > > in > > > > > the > > > > > > > above list) and leave the other parts as they are? > > > > > > > > > > > > This plan may need a bit of tweaking to handle the GPUMemoryBuffer and > > > > GLImage > > > > > > cases. > > > > > > > > > > > > The first issue is that the IDs used for both these types must match - > > > > > > currently, GetGpuMemoryBufferGUIDForTracing uses > > > > > > GetGenericSharedMemoryGUIDForTracing. This is necessary, as we use > these > > > IDs > > > > > to > > > > > > create ownership edges for GPUMemoryBuffers and GLImages which > represent > > > the > > > > > > same memory. > > > > > > > > > > > > Additionally, certain types of GPU Memory buffers (IOSurfaces), don't > > use > > > > > Chrome > > > > > > "shared memory", but represent alternate system-specific shared memory > > > > > concepts. > > > > > > Currently, they use the same set of IDs as shared memory > GPUMemoyBuffers > > > and > > > > > > GLImages. This isn't a hard requirement, and would probably have to > > change > > > > > with > > > > > > the design you propose. > > > > > > > > > > > > I think that in order for your system to work, you probably need to > > split > > > > the > > > > > > GPUMemoryBuffer and GLImage IDs into two categories: > > > > > > > > > > > > Shared Memory GUIDs (as described in your doc) - used for > > > > GLImageSharedMemory > > > > > > and GPUMemoryBufferSharedMemory (will need to tweak > > > > > > BrowserGpuMemoryBufferManager to use a virtual method or switch GUID > > > > > generation > > > > > > based on GPU Memory buffer type). > > > > > > > > > > > > GenericSharedGPUMemoryID - some new type used for IOSurfaces and other > > > > > platform > > > > > > specific GPUMemoryBuffers - currently needs to be used for > > > GLImageIOSurface > > > > > and > > > > > > GPUMemoryBufferIOSurface. > > > > > > > > > > > > Let me know if this makes sense, and I'm happy to help with this. > > Thanks! > > > > > > > > > > Thank you, Eric! My understanding is: > > > > > > > > > > * Current GUIDs doesn't tell whether GpuMemoryBuffer, GLImage or other > > > related > > > > > objects use shared memory. > > > > > * GetGenericSharedMemoryGUIDForTracing is used by GLImageSharedMemory > and > > > > > GLImageIOSurface > > > > > * GetGpuMemoryBufferGUIDForTracing is used by GpuMemoryBuffer mainly and > > > > > StagingBuffer, GpuMemoryBufferVideoFramePool::PlaneResource and > > > > > ResourceProvider::Resource indirectly > > > > > * All of them might or might not use shared memory. > > > > > * Your suggest is to get both GetGenericSharedMemoryGUIDForTracing and > > > > > GetGpuMemoryBufferGUIDForTracing together and create > > > > > GetSharedMemoryGUIDForTracing and > GetGenericSharedGPUMemoryGUIDFOrTracing > > > > > > > > > > Is this correct? > > > Yup, that's my understanding - we need one set of IDs for shared memory > (which > > > is what you describe in your doc), and one set for other things which GPU > > Memory > > > Buffers / GL Images use interchangeably with shared memory. > > > > > > > > > > > Another question: GetGenericSharedMemoryGUIDForTracing takes the tracing > > > process > > > > id, but why is this necessary? It looks like independent global dumps are > > > > created for each client, and there seems no shared dump between multiple > > > > processes. > > > > > > > > For example, in BrowserGpuMemoryBufferManager, > > CreateSharedGlobalAllocatorDump > > > > generates a global dump for a given id, but the ids are different among > > child > > > > processes. > > > > > > > > > > https://cs.chromium.org/chromium/src/content/browser/gpu/browser_gpu_memory_b... > > > > I thought dumps for shared objects or general shared things should have > one > > ID > > > > and ownership edges must be created toward it from its client, but am I > > > > misunderstanding? > > > > > > Taking the shared memory case (ignoring IOsurfaces): > > > > > > The BrowserGpuMemoryBufferManager, ResourceProvider, and GLImageSharedMemory > > > should all end up generating the same GUID for the global dump - they all > use > > > the tracing process ID of the client (renderer), and they should all share > > > the same buffer_id. We then add an ownership edges from the dump to the > > > shared GUID. (there is currently a be a bug in the GLImageSharedMemory part > > > of this. I'm taking a look now, but I've confirmed that > > > everything works as expected if the bug is fixed). > > > > Yep, if shared-memory clients are in the same process, the global guid is > same. > > My question is, "the global guid would be different for each client process, > and > > is this correct?". Isn't there a dump that is shared among multiple processes > in > > this case? > > The tracing process ID we're using isn't the current process ID, but the ID of > the > allocating client. So in this case, we do end up with the same GUID in multiple > processes. We can discuss more on chat if that would help? > > However, looking at things, I think that the buffer IDs may be GUIDs already, so > this extra ID may be unnecessary (although I think it is currently working as > expected). If you agree that the buffer IDs are already globally unique, feel > free > to drop the client tracing ID. Thank you for explanation! My goal is to replace them with shared memory tracker's GUID generators, and my worry was I didn't know how to treat tracing IDs. Now I understand I can ignore them when using shared memory tracker.
My current idea is, as ssid@ said, not to split the big CL (https://codereview.chromium.org/2535213002/), and to replace all owner ship edges once. I'll close this CL. Replacing once would be safe and be a mechanical change after https://codereview.chromium.org/2819803002/. I'm now rebasing the big CL, which requires some code change. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
