Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1259333003:
[tracing] Add SkResourceCache as suballocation to avoid double counting. (Closed)
Description[tracing] Add SkResourceCache as suballocation to avoid double counting.
The SkResourceCache can be allocated from either malloc or discardable
memory. This causes the memory dumps to be double counted. This CL
adds the resource cache dumps as sub-allocation from appropriate
allocator.
BUG=503168
Patch Set 1 #Patch Set 2 : Fixes. #Patch Set 3 : Clean up. #
Total comments: 11
Patch Set 4 : Adding host and fixing style. #Patch Set 5 : More fixes. #Patch Set 6 : Nits. #
Total comments: 15
Patch Set 7 : addresssing comments. #
Total comments: 8
Patch Set 8 : Nits. #Patch Set 9 : Fixing comments. #
Total comments: 19
Patch Set 10 : Adressing comments. #Patch Set 11 : Nits. #
Total comments: 10
Patch Set 12 : Removing segment dump from base. #
Messages
Total messages: 23 (3 generated)
ssid@chromium.org changed reviewers: + petrcermak@chromium.org
PTAL.
Looks good overall. Couple of comments. Thanks, Petr https://codereview.chromium.org/1259333003/diff/40001/content/child/child_dis... File content/child/child_discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/1259333003/diff/40001/content/child/child_dis... content/child/child_discardable_shared_memory_manager.cc:201: pmd->CreateAllocatorDump("discardable/allocated_objects"); Any chance this name could be a constant like https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/m... ? https://codereview.chromium.org/1259333003/diff/40001/content/child/child_dis... content/child/child_discardable_shared_memory_manager.cc:203: pmd->CreateAllocatorDump("discardable/segments"); Could this be a constant? https://codereview.chromium.org/1259333003/diff/40001/content/child/child_dis... content/child/child_discardable_shared_memory_manager.cc:207: // "discardable/allocated_objects" will be owned by it. The relationship is reversed (in the comment). https://codereview.chromium.org/1259333003/diff/40001/skia/ext/skia_memory_du... File skia/ext/skia_memory_dump_provider.cc (right): https://codereview.chromium.org/1259333003/diff/40001/skia/ext/skia_memory_du... skia/ext/skia_memory_dump_provider.cc:37: if (skia_resource_cache_size > 0) { Shouldn't we report zero explicitly as well? https://codereview.chromium.org/1259333003/diff/40001/skia/ext/skia_memory_du... skia/ext/skia_memory_dump_provider.cc:44: process_memory_dump->AddSuballocation(resource_mad->guid(), You could use a ternary operator or factor out the name to reduce code duplication. https://codereview.chromium.org/1259333003/diff/40001/skia/ext/skia_memory_du... skia/ext/skia_memory_dump_provider.cc:48: "malloc/allocated_objects"); Please use the base::trace_event::MallocDumpProvider::kAllocatedObjects constant instead.
PTAL https://codereview.chromium.org/1259333003/diff/40001/content/child/child_dis... File content/child/child_discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/1259333003/diff/40001/content/child/child_dis... content/child/child_discardable_shared_memory_manager.cc:201: pmd->CreateAllocatorDump("discardable/allocated_objects"); On 2015/07/29 11:53:34, petrcermak wrote: > Any chance this name could be a constant like > https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/m... > ? Done. https://codereview.chromium.org/1259333003/diff/40001/content/child/child_dis... content/child/child_discardable_shared_memory_manager.cc:203: pmd->CreateAllocatorDump("discardable/segments"); On 2015/07/29 11:53:34, petrcermak wrote: > Could this be a constant? Done. https://codereview.chromium.org/1259333003/diff/40001/content/child/child_dis... content/child/child_discardable_shared_memory_manager.cc:207: // "discardable/allocated_objects" will be owned by it. On 2015/07/29 11:53:34, petrcermak wrote: > The relationship is reversed (in the comment). Done. https://codereview.chromium.org/1259333003/diff/40001/skia/ext/skia_memory_du... File skia/ext/skia_memory_dump_provider.cc (right): https://codereview.chromium.org/1259333003/diff/40001/skia/ext/skia_memory_du... skia/ext/skia_memory_dump_provider.cc:37: if (skia_resource_cache_size > 0) { On 2015/07/29 11:53:34, petrcermak wrote: > Shouldn't we report zero explicitly as well? Yea i think it makes more sense to dump 0 https://codereview.chromium.org/1259333003/diff/40001/skia/ext/skia_memory_du... skia/ext/skia_memory_dump_provider.cc:48: "malloc/allocated_objects"); On 2015/07/29 11:53:34, petrcermak wrote: > Please use the base::trace_event::MallocDumpProvider::kAllocatedObjects constant > instead. Done.
A few more comments. Thanks, Petr https://codereview.chromium.org/1259333003/diff/100001/base/memory/discardabl... File base/memory/discardable_shared_memory.h (right): https://codereview.chromium.org/1259333003/diff/100001/base/memory/discardabl... base/memory/discardable_shared_memory.h:35: // Eg. : ProcessMemoryDump::AddSuballocation(skia_dump_guid, nit: There should be a dot after "E" and no space before the colon. https://codereview.chromium.org/1259333003/diff/100001/base/memory/discardabl... base/memory/discardable_shared_memory.h:36: // kDiscardableSegmentsDumpName); How is this related to the constant??? Shouldn't it be s/kDiscardableSegmentsDumpName/kAllocatedObjectsDumpName/? https://codereview.chromium.org/1259333003/diff/100001/content/common/discard... File content/common/discardable_shared_memory_heap.cc (right): https://codereview.chromium.org/1259333003/diff/100001/content/common/discard... content/common/discardable_shared_memory_heap.cc:396: void DiscardableSharedMemoryHeap::CreateAllocatedObjectsDump( nit: I think you should say "// static" above the function definition. https://codereview.chromium.org/1259333003/diff/100001/content/common/discard... content/common/discardable_shared_memory_heap.cc:404: // The discardable memory segments will contain kDiscardableSegmentsDumpName This is reversed. The dump called kDiscardableSegmentsDumpName will contain discardable memory segments. https://codereview.chromium.org/1259333003/diff/100001/content/common/discard... content/common/discardable_shared_memory_heap.cc:405: // as the root and will own any sub-allocations with kAllocatedObjectsDumpName Probably "parent" or "ancestor" instead of "root" ("discardable/" is the root) (twice). https://codereview.chromium.org/1259333003/diff/100001/content/common/discard... content/common/discardable_shared_memory_heap.cc:405: // as the root and will own any sub-allocations with kAllocatedObjectsDumpName s/own/be owned by/ https://codereview.chromium.org/1259333003/diff/100001/skia/ext/skia_memory_d... File skia/ext/skia_memory_dump_provider.cc (right): https://codereview.chromium.org/1259333003/diff/100001/skia/ext/skia_memory_d... skia/ext/skia_memory_dump_provider.cc:46: ? base::DiscardableSharedMemory::kAllocatedObjectsDumpName Maybe rename the constant to "kAllocatedObjects" to be consistent with MallocDumpProvider? https://codereview.chromium.org/1259333003/diff/100001/skia/ext/skia_memory_d... skia/ext/skia_memory_dump_provider.cc:49: return true; nit: I'd keep a blank line in front of the return statement.
Thanks, made changes. https://codereview.chromium.org/1259333003/diff/100001/base/memory/discardabl... File base/memory/discardable_shared_memory.h (right): https://codereview.chromium.org/1259333003/diff/100001/base/memory/discardabl... base/memory/discardable_shared_memory.h:35: // Eg. : ProcessMemoryDump::AddSuballocation(skia_dump_guid, On 2015/07/29 16:38:59, petrcermak wrote: > nit: There should be a dot after "E" and no space before the colon. Done. https://codereview.chromium.org/1259333003/diff/100001/base/memory/discardabl... base/memory/discardable_shared_memory.h:36: // kDiscardableSegmentsDumpName); On 2015/07/29 16:38:59, petrcermak wrote: > How is this related to the constant??? Shouldn't it be > s/kDiscardableSegmentsDumpName/kAllocatedObjectsDumpName/? Done. https://codereview.chromium.org/1259333003/diff/100001/content/common/discard... File content/common/discardable_shared_memory_heap.cc (right): https://codereview.chromium.org/1259333003/diff/100001/content/common/discard... content/common/discardable_shared_memory_heap.cc:396: void DiscardableSharedMemoryHeap::CreateAllocatedObjectsDump( On 2015/07/29 16:38:59, petrcermak wrote: > nit: I think you should say "// static" above the function definition. Done. https://codereview.chromium.org/1259333003/diff/100001/content/common/discard... content/common/discardable_shared_memory_heap.cc:404: // The discardable memory segments will contain kDiscardableSegmentsDumpName On 2015/07/29 16:38:59, petrcermak wrote: > This is reversed. The dump called kDiscardableSegmentsDumpName will contain > discardable memory segments. Done. https://codereview.chromium.org/1259333003/diff/100001/content/common/discard... content/common/discardable_shared_memory_heap.cc:405: // as the root and will own any sub-allocations with kAllocatedObjectsDumpName On 2015/07/29 16:38:59, petrcermak wrote: > s/own/be owned by/ Done. https://codereview.chromium.org/1259333003/diff/100001/skia/ext/skia_memory_d... File skia/ext/skia_memory_dump_provider.cc (right): https://codereview.chromium.org/1259333003/diff/100001/skia/ext/skia_memory_d... skia/ext/skia_memory_dump_provider.cc:46: ? base::DiscardableSharedMemory::kAllocatedObjectsDumpName On 2015/07/29 16:39:00, petrcermak wrote: > Maybe rename the constant to "kAllocatedObjects" to be consistent with > MallocDumpProvider? Added dumpName since This is not really a dumpProvider. Just a string name AllocatedObjects won't really mean anything to this class. https://codereview.chromium.org/1259333003/diff/100001/skia/ext/skia_memory_d... skia/ext/skia_memory_dump_provider.cc:49: return true; On 2015/07/29 16:38:59, petrcermak wrote: > nit: I'd keep a blank line in front of the return statement. Done.
LGTM with 5 more comments (sorry). Thanks, Petr https://codereview.chromium.org/1259333003/diff/120001/content/common/discard... File content/common/discardable_shared_memory_heap.cc (right): https://codereview.chromium.org/1259333003/diff/120001/content/common/discard... content/common/discardable_shared_memory_heap.cc:406: // kDiscardableSegmentsDumpName as the patent and is owned by the s/patent/parent https://codereview.chromium.org/1259333003/diff/120001/content/common/discard... content/common/discardable_shared_memory_heap.cc:406: // kDiscardableSegmentsDumpName as the patent and is owned by the s/is/will be/ https://codereview.chromium.org/1259333003/diff/120001/content/common/discard... content/common/discardable_shared_memory_heap.cc:407: // sub-allocations dumped with kAllocatedObjectsDumpName as the parent. nit: s/the/their/ https://codereview.chromium.org/1259333003/diff/120001/content/common/discard... File content/common/discardable_shared_memory_heap.h (right): https://codereview.chromium.org/1259333003/diff/120001/content/common/discard... content/common/discardable_shared_memory_heap.h:91: // Creates a common dump for all the sub-allocated objects from discardable nit: I don't think there should be "the" here https://codereview.chromium.org/1259333003/diff/120001/skia/ext/skia_memory_d... File skia/ext/skia_memory_dump_provider.cc (right): https://codereview.chromium.org/1259333003/diff/120001/skia/ext/skia_memory_d... skia/ext/skia_memory_dump_provider.cc:38: const size_t skia_resource_cache_size = SkResourceCache::GetTotalBytesUsed(); nit: I don't think there's a need to factor it out (given that you don't do it above either).
Thanks! No problem https://codereview.chromium.org/1259333003/diff/120001/content/common/discard... File content/common/discardable_shared_memory_heap.cc (right): https://codereview.chromium.org/1259333003/diff/120001/content/common/discard... content/common/discardable_shared_memory_heap.cc:406: // kDiscardableSegmentsDumpName as the patent and is owned by the On 2015/07/29 17:04:07, petrcermak wrote: > s/is/will be/ Done. https://codereview.chromium.org/1259333003/diff/120001/content/common/discard... content/common/discardable_shared_memory_heap.cc:407: // sub-allocations dumped with kAllocatedObjectsDumpName as the parent. On 2015/07/29 17:04:07, petrcermak wrote: > nit: s/the/their/ Done. https://codereview.chromium.org/1259333003/diff/120001/content/common/discard... File content/common/discardable_shared_memory_heap.h (right): https://codereview.chromium.org/1259333003/diff/120001/content/common/discard... content/common/discardable_shared_memory_heap.h:91: // Creates a common dump for all the sub-allocated objects from discardable On 2015/07/29 17:04:07, petrcermak wrote: > nit: I don't think there should be "the" here Done.
ssid@chromium.org changed reviewers: + reed@chromium.org, reveman@chromium.org
This is to add ownership edges between the skia dumps and and discardable or malloc dumps. PTAL. Thanks
ssid@chromium.org changed reviewers: + reed@google.com - reed@chromium.org
+reed1 -reed2
https://codereview.chromium.org/1259333003/diff/160001/base/memory/discardabl... File base/memory/discardable_shared_memory.cc (right): https://codereview.chromium.org/1259333003/diff/160001/base/memory/discardabl... base/memory/discardable_shared_memory.cc:102: "discardable/allocated_objects"; same here. I don' think this is the right place for this. https://codereview.chromium.org/1259333003/diff/160001/base/memory/discardabl... File base/memory/discardable_shared_memory.h (right): https://codereview.chromium.org/1259333003/diff/160001/base/memory/discardabl... base/memory/discardable_shared_memory.h:37: static const char kAllocatedObjectsDumpName[]; I don't think we want this here. We shouldn't assume that DiscardableMemory instances are backed by DiscardableSharedMemory. https://codereview.chromium.org/1259333003/diff/160001/content/common/discard... File content/common/discardable_shared_memory_heap.h (right): https://codereview.chromium.org/1259333003/diff/160001/content/common/discard... content/common/discardable_shared_memory_heap.h:94: base::trace_event::ProcessMemoryDump* pmd); why do we want a common dump? Would it not be more correct to have each discardable memory sub-allocation be associated with the specific segment they are part of? https://codereview.chromium.org/1259333003/diff/160001/skia/ext/skia_memory_d... File skia/ext/skia_memory_dump_provider.cc (right): https://codereview.chromium.org/1259333003/diff/160001/skia/ext/skia_memory_d... skia/ext/skia_memory_dump_provider.cc:46: : base::trace_event::MallocDumpProvider::kAllocatedObjects; Can we instead ask the SkResourceCache for the name and add plumbing all the way down to the segment to retrieve the correct source name?
Please see comments inline. https://codereview.chromium.org/1259333003/diff/160001/base/memory/discardabl... File base/memory/discardable_shared_memory.h (right): https://codereview.chromium.org/1259333003/diff/160001/base/memory/discardabl... base/memory/discardable_shared_memory.h:37: static const char kAllocatedObjectsDumpName[]; On 2015/07/29 18:32:11, reveman wrote: > I don't think we want this here. We shouldn't assume that DiscardableMemory > instances are backed by DiscardableSharedMemory. Do you think DiscardableMemoryAllocator is a better place for this? I initially placed it there, but moved it here since the kAllocatedObjectsDumpName is used only if shared memory allocator is used. https://codereview.chromium.org/1259333003/diff/160001/content/common/discard... File content/common/discardable_shared_memory_heap.h (right): https://codereview.chromium.org/1259333003/diff/160001/content/common/discard... content/common/discardable_shared_memory_heap.h:94: base::trace_event::ProcessMemoryDump* pmd); On 2015/07/29 18:32:11, reveman wrote: > why do we want a common dump? Would it not be more correct to have each > discardable memory sub-allocation be associated with the specific segment they > are part of? Replied for the other comment. https://codereview.chromium.org/1259333003/diff/160001/skia/ext/skia_memory_d... File skia/ext/skia_memory_dump_provider.cc (right): https://codereview.chromium.org/1259333003/diff/160001/skia/ext/skia_memory_d... skia/ext/skia_memory_dump_provider.cc:46: : base::trace_event::MallocDumpProvider::kAllocatedObjects; On 2015/07/29 18:32:11, reveman wrote: > Can we instead ask the SkResourceCache for the name and add plumbing all the way > down to the segment to retrieve the correct source name? Currently the SkResourceCache dump does not include each allocation from discardable. It might not be included in future also. If that is the case, The allocations can practically be from any segment. That is why I create a common allocated_objects node and dump it. Also, is it might not be very useful for the user to see multiple skia allocations from multiple segments, in sense that: Suppose gpu wants to create a sub-allocation from discardable, it would also make multiple gpu dumps under multiple segments. In this view it is not possible to see how much of discardable is skia using and how much is gpu using. But if it is kept under separate node, then It is easy to see that. Moreover, the segment-dumps in discardable to understand the fragmentation and efficiency of the discardable heap. So, keeping it as different entity won't harm the use-case.
https://codereview.chromium.org/1259333003/diff/160001/skia/ext/skia_memory_d... File skia/ext/skia_memory_dump_provider.cc (right): https://codereview.chromium.org/1259333003/diff/160001/skia/ext/skia_memory_d... skia/ext/skia_memory_dump_provider.cc:46: : base::trace_event::MallocDumpProvider::kAllocatedObjects; On 2015/07/29 at 19:36:22, ssid wrote: > On 2015/07/29 18:32:11, reveman wrote: > > Can we instead ask the SkResourceCache for the name and add plumbing all the way > > down to the segment to retrieve the correct source name? > > Currently the SkResourceCache dump does not include each allocation from discardable. It might not be included in future also. If that is the case, The allocations can practically be from any segment. That is why I create a common allocated_objects node and dump it. > > Also, is it might not be very useful for the user to see multiple skia allocations from multiple segments, in sense that: > Suppose gpu wants to create a sub-allocation from discardable, it would also make multiple gpu dumps under multiple segments. In this view it is not possible to see how much of discardable is skia using and how much is gpu using. But if it is kept under separate node, then It is easy to see that. I'm not following this. What's not possible to see when forcing suballocations to be children on segment allocations rather than a global discardable allocation? > > Moreover, the segment-dumps in discardable to understand the fragmentation and efficiency of the discardable heap. So, keeping it as different entity won't harm the use-case. I think it would be quite useful to see exactly what type of usage is causing fragmentation. I'm failing to see any negative sides with having suballocations connect to segments. Afaict it just provides more detailed information and align better with what we have to do for shared-memory/gpu-memory-buffers. Maybe VC to discuss this more?
On 2015/07/29 19:59:24, reveman wrote: > https://codereview.chromium.org/1259333003/diff/160001/skia/ext/skia_memory_d... > File skia/ext/skia_memory_dump_provider.cc (right): > > https://codereview.chromium.org/1259333003/diff/160001/skia/ext/skia_memory_d... > skia/ext/skia_memory_dump_provider.cc:46: : > base::trace_event::MallocDumpProvider::kAllocatedObjects; > On 2015/07/29 at 19:36:22, ssid wrote: > > On 2015/07/29 18:32:11, reveman wrote: > > > Can we instead ask the SkResourceCache for the name and add plumbing all the > way > > > down to the segment to retrieve the correct source name? > > > > Currently the SkResourceCache dump does not include each allocation from > discardable. It might not be included in future also. If that is the case, The > allocations can practically be from any segment. That is why I create a common > allocated_objects node and dump it. > > > > Also, is it might not be very useful for the user to see multiple skia > allocations from multiple segments, in sense that: > > Suppose gpu wants to create a sub-allocation from discardable, it would also > make multiple gpu dumps under multiple segments. In this view it is not possible > to see how much of discardable is skia using and how much is gpu using. But if > it is kept under separate node, then It is easy to see that. > > I'm not following this. What's not possible to see when forcing suballocations > to be children on segment allocations rather than a global discardable > allocation? > I meant it's not easy to the aggregation of each component that uses discardable. Isn't it better to view the total percent of discardable used by skia or other_component, than to view what causes fragmentation? More importantly, we are not planning to dump all the objects in the cache, which can make the trace huge. In that case one dump of skia can be spread across multiple segments. That's is why allocated objects dump is created in discardable. > > > > Moreover, the segment-dumps in discardable to understand the fragmentation and > efficiency of the discardable heap. So, keeping it as different entity won't > harm the use-case. > > I think it would be quite useful to see exactly what type of usage is causing > fragmentation. I'm failing to see any negative sides with having suballocations > connect to segments. Afaict it just provides more detailed information and align > better with what we have to do for shared-memory/gpu-memory-buffers. Maybe VC to > discuss this more?
On 2015/07/29 20:53:18, ssid wrote: > On 2015/07/29 19:59:24, reveman wrote: > > > https://codereview.chromium.org/1259333003/diff/160001/skia/ext/skia_memory_d... > > File skia/ext/skia_memory_dump_provider.cc (right): > > > > > https://codereview.chromium.org/1259333003/diff/160001/skia/ext/skia_memory_d... > > skia/ext/skia_memory_dump_provider.cc:46: : > > base::trace_event::MallocDumpProvider::kAllocatedObjects; > > On 2015/07/29 at 19:36:22, ssid wrote: > > > On 2015/07/29 18:32:11, reveman wrote: > > > > Can we instead ask the SkResourceCache for the name and add plumbing all > the > > way > > > > down to the segment to retrieve the correct source name? > > > > > > Currently the SkResourceCache dump does not include each allocation from > > discardable. It might not be included in future also. If that is the case, The > > allocations can practically be from any segment. That is why I create a common > > allocated_objects node and dump it. > > > > > > Also, is it might not be very useful for the user to see multiple skia > > allocations from multiple segments, in sense that: > > > Suppose gpu wants to create a sub-allocation from discardable, it would also > > make multiple gpu dumps under multiple segments. In this view it is not > possible > > to see how much of discardable is skia using and how much is gpu using. But if > > it is kept under separate node, then It is easy to see that. > > > > I'm not following this. What's not possible to see when forcing suballocations > > to be children on segment allocations rather than a global discardable > > allocation? > > > > I meant it's not easy to the aggregation of each component that uses > discardable. Isn't it better to view the total percent of discardable used by > skia or other_component, than to view what causes fragmentation? > > More importantly, we are not planning to dump all the objects in the cache, > which can make the trace huge. In that case one dump of skia can be spread > across multiple segments. That's is why allocated objects dump is created in > discardable. > > > > > > > > Moreover, the segment-dumps in discardable to understand the fragmentation > and > > efficiency of the discardable heap. So, keeping it as different entity won't > > harm the use-case. > > > > I think it would be quite useful to see exactly what type of usage is causing > > fragmentation. I'm failing to see any negative sides with having > suballocations > > connect to segments. Afaict it just provides more detailed information and > align > > better with what we have to do for shared-memory/gpu-memory-buffers. Maybe VC > to > > discuss this more? If there's a different idea, I'll set up a Vc tomorrow and discuss this. Thanks.
On 2015/07/29 at 20:53:18, ssid wrote: > On 2015/07/29 19:59:24, reveman wrote: > > https://codereview.chromium.org/1259333003/diff/160001/skia/ext/skia_memory_d... > > File skia/ext/skia_memory_dump_provider.cc (right): > > > > https://codereview.chromium.org/1259333003/diff/160001/skia/ext/skia_memory_d... > > skia/ext/skia_memory_dump_provider.cc:46: : > > base::trace_event::MallocDumpProvider::kAllocatedObjects; > > On 2015/07/29 at 19:36:22, ssid wrote: > > > On 2015/07/29 18:32:11, reveman wrote: > > > > Can we instead ask the SkResourceCache for the name and add plumbing all the > > way > > > > down to the segment to retrieve the correct source name? > > > > > > Currently the SkResourceCache dump does not include each allocation from > > discardable. It might not be included in future also. If that is the case, The > > allocations can practically be from any segment. That is why I create a common > > allocated_objects node and dump it. > > > > > > Also, is it might not be very useful for the user to see multiple skia > > allocations from multiple segments, in sense that: > > > Suppose gpu wants to create a sub-allocation from discardable, it would also > > make multiple gpu dumps under multiple segments. In this view it is not possible > > to see how much of discardable is skia using and how much is gpu using. But if > > it is kept under separate node, then It is easy to see that. > > > > I'm not following this. What's not possible to see when forcing suballocations > > to be children on segment allocations rather than a global discardable > > allocation? > > > > I meant it's not easy to the aggregation of each component that uses discardable. Isn't it better to view the total percent of discardable used by skia or other_component, than to view what causes fragmentation? > > More importantly, we are not planning to dump all the objects in the cache, which can make the trace huge. In that case one dump of skia can be spread across multiple segments. That's is why allocated objects dump is created in discardable. > hm, I guess we'd have to report total skia usage per segment to avoid dumping each object.. Ok, I think I buy the process wide discardable node idea. Just want to make sure some of the use cases I'm interested in are covered. 1. SkResourceCache usage is not going to be enough. We're going to have to split that into MaskCache, DecodeCache, ScaleCache, ShaderCache, etc. I assume that's not a problem. 2. We should be able to report the amount that is pinned vs unpinned. 3. This shouldn't stop us from also reporting each allocation. For debugging decoded image cache usage it's likely going to be very useful to see exactly what size each allocation is. E.g. it's very useful to know if one large image is taking up all memory or 50 relatively large images.
> hm, I guess we'd have to report total skia usage per segment to avoid dumping > each object.. Ok, I think I buy the process wide discardable node idea. Just > want to make sure some of the use cases I'm interested in are covered. > > 1. SkResourceCache usage is not going to be enough. We're going to have to split > that into MaskCache, DecodeCache, ScaleCache, ShaderCache, etc. I assume that's > not a problem. Yes, That is right. For this reason I am exposing the base/ memory-infra API to skia. This will enable us to implement dump providers in skia. For the next set of cache instrumentation in skia, Please see this doc :https://docs.google.com/document/d/1eejfBaKGB5zXc2okXrGsGIF5ycueeUZHPI93oK2aW5c I guess MaskCache is wrapper around the resource cache. Shader seems to be using GrTexture (from GrResourceCache), which is added in the doc. The other image caches also seem to use GrResourceCache. Please see the doc, and add comments if I can add more. I am discussing about the image cache with skia folks and I will update. Thanks. > 2. We should be able to report the amount that is pinned vs unpinned. I am not very clear about how we do this. If this is to be done, then we would have to make the SkResourceCache count the pinned and unpinnned and give numbers. I have to think a bit more on that. I'd have add some code to store the locked and unlocked allocations in the cache. Is is added in the doc, I will get to it soon. > 3. This shouldn't stop us from also reporting each allocation. For debugging > decoded image cache usage it's likely going to be very useful to see exactly > what size each allocation is. E.g. it's very useful to know if one large image > is taking up all memory or 50 relatively large images. Yes this won't stop us from adding more information available in future. Multiple ownership edges can be added to dumps.
On 2015/07/30 at 13:12:17, ssid wrote: > > hm, I guess we'd have to report total skia usage per segment to avoid dumping > > each object.. Ok, I think I buy the process wide discardable node idea. Just > > want to make sure some of the use cases I'm interested in are covered. > > > > 1. SkResourceCache usage is not going to be enough. We're going to have to split > > that into MaskCache, DecodeCache, ScaleCache, ShaderCache, etc. I assume that's > > not a problem. > > Yes, That is right. For this reason I am exposing the base/ memory-infra API to skia. This will enable us to implement dump providers in skia. > For the next set of cache instrumentation in skia, Please see this doc :https://docs.google.com/document/d/1eejfBaKGB5zXc2okXrGsGIF5ycueeUZHPI93oK2aW5c > I guess MaskCache is wrapper around the resource cache. Yes. > Shader seems to be using GrTexture (from GrResourceCache), which is added in the doc. I meant the PictureShader cache. It's also a wrapper around SkResourceCache. > The other image caches also seem to use GrResourceCache. Please see the doc, and add comments if I can add more. > I am discussing about the image cache with skia folks and I will update. > Thanks. FYI, I don't think the decoded images are accounted for in SkResourceCache. That's what most discardable memory is used for so we need to come up with a plan for that. > > > 2. We should be able to report the amount that is pinned vs unpinned. > > I am not very clear about how we do this. If this is to be done, then we would have to make the SkResourceCache count the pinned and unpinnned and give numbers. I have to think a bit more on that. I'd have add some code to store the locked and unlocked allocations in the cache. Is is added in the doc, I will get to it soon. Having this information will be very useful as it shows us how much memory is necessary to render a page vs what could be discarded. Ideally we'd never have an OOM error as a result of using too much discardable memory. A page using too much pinned memory however is something that we currently can't prevent. FYI, the problem with adding this information without having tied each allocation to a segment is that a segment is pinned if only one allocation in the segment is pinned. So it will be hard to see why 8MB of discardable memory is pinned at the lower level but only 2KB at the SkResourceCache level unless we can see that 1KB is in segment 1 and the other 1KB is in segment 2... > > > 3. This shouldn't stop us from also reporting each allocation. For debugging > > decoded image cache usage it's likely going to be very useful to see exactly > > what size each allocation is. E.g. it's very useful to know if one large image > > is taking up all memory or 50 relatively large images. > > Yes this won't stop us from adding more information available in future. Multiple ownership edges can be added to dumps. Cool. https://codereview.chromium.org/1259333003/diff/160001/base/memory/discardabl... File base/memory/discardable_shared_memory.h (right): https://codereview.chromium.org/1259333003/diff/160001/base/memory/discardabl... base/memory/discardable_shared_memory.h:37: static const char kAllocatedObjectsDumpName[]; On 2015/07/29 at 19:36:22, ssid wrote: > On 2015/07/29 18:32:11, reveman wrote: > > I don't think we want this here. We shouldn't assume that DiscardableMemory > > instances are backed by DiscardableSharedMemory. > > Do you think DiscardableMemoryAllocator is a better place for this? I initially placed it there, but moved it here since the kAllocatedObjectsDumpName is used only if shared memory allocator is used. Yes, DiscardableMemoryAllocator is better. That name we add there will be correct independent of the underlying discardable memory system. Shared memory based or not. Also, can we make the name consistent with the name for system allocator dumps and wrap it in a function? https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/m... GetDiscardableAllocatorMemoryPoolName() maybe? https://codereview.chromium.org/1259333003/diff/160001/content/common/discard... File content/common/discardable_shared_memory_heap.cc (right): https://codereview.chromium.org/1259333003/diff/160001/content/common/discard... content/common/discardable_shared_memory_heap.cc:25: const char kDiscardableSegmentsDumpName[] = "discardable/segments"; We've been avoiding explicit constants for dump name strings. Is this different or should we just use 'base::StringPrintf("discardable/segments/segment_%d", ...' below instead? Btw, it would be a bit more appropriate if the DiscardableMemoryAllocator controlled this prefix and just passed it to DiscardableSharedMemoryHeap::OnMemoryDump but no need to fix that in this CL. https://codereview.chromium.org/1259333003/diff/160001/content/common/discard... content/common/discardable_shared_memory_heap.cc:230: CreateAllocatedObjectsDump(pmd); I think it should be the responsibility of the DiscardableMemoryAllocator to create this node. https://codereview.chromium.org/1259333003/diff/160001/content/common/discard... File content/common/discardable_shared_memory_heap.h (right): https://codereview.chromium.org/1259333003/diff/160001/content/common/discard... content/common/discardable_shared_memory_heap.h:93: static void CreateAllocatedObjectsDump( I think this should be moved to the allocator implementation instead. See my other comment. https://codereview.chromium.org/1259333003/diff/160001/skia/ext/skia_memory_d... File skia/ext/skia_memory_dump_provider.cc (right): https://codereview.chromium.org/1259333003/diff/160001/skia/ext/skia_memory_d... skia/ext/skia_memory_dump_provider.cc:43: const char* source_name = When discardable memory is not used, shouldn't this be set to the return value of MemoryDumpManager::GetInstance()->system_allocator_pool_name() instead of MallocDumpProvider::kAllocatedObjects?
Made suggested changes. Thanks > Having this information will be very useful as it shows us how much memory is > necessary to render a page vs what could be discarded. Ideally we'd never have > an OOM error as a result of using too much discardable memory. A page using too > much pinned memory however is something that we currently can't prevent. > > FYI, the problem with adding this information without having tied each > allocation to a segment is that a segment is pinned if only one allocation in > the segment is pinned. So it will be hard to see why 8MB of discardable memory > is pinned at the lower level but only 2KB at the SkResourceCache level unless we > can see that 1KB is in segment 1 and the other 1KB is in segment 2... > Yes, I get your point. But taking one dump per allocated objects can be huge. Also, we might end up storing a lot of metadata along with each allocation to mark if it is locked or not. I will try to see what all can be marked, I don't think we will be able to get a complete picture of all allocations everywhere from chrome. I will look into this. Thanks. https://codereview.chromium.org/1259333003/diff/160001/base/memory/discardabl... File base/memory/discardable_shared_memory.cc (right): https://codereview.chromium.org/1259333003/diff/160001/base/memory/discardabl... base/memory/discardable_shared_memory.cc:102: "discardable/allocated_objects"; On 2015/07/29 18:32:11, reveman wrote: > same here. I don' think this is the right place for this. Done. https://codereview.chromium.org/1259333003/diff/160001/base/memory/discardabl... File base/memory/discardable_shared_memory.h (right): https://codereview.chromium.org/1259333003/diff/160001/base/memory/discardabl... base/memory/discardable_shared_memory.h:37: static const char kAllocatedObjectsDumpName[]; On 2015/07/30 16:00:59, reveman wrote: > On 2015/07/29 at 19:36:22, ssid wrote: > > On 2015/07/29 18:32:11, reveman wrote: > > > I don't think we want this here. We shouldn't assume that DiscardableMemory > > > instances are backed by DiscardableSharedMemory. > > > > Do you think DiscardableMemoryAllocator is a better place for this? I > initially placed it there, but moved it here since the kAllocatedObjectsDumpName > is used only if shared memory allocator is used. > > Yes, DiscardableMemoryAllocator is better. That name we add there will be > correct independent of the underlying discardable memory system. Shared memory > based or not. > > Also, can we make the name consistent with the name for system allocator dumps > and wrap it in a function? > https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/m... > > GetDiscardableAllocatorMemoryPoolName() maybe? Thanks I din't know this existed. https://codereview.chromium.org/1259333003/diff/160001/content/common/discard... File content/common/discardable_shared_memory_heap.cc (right): https://codereview.chromium.org/1259333003/diff/160001/content/common/discard... content/common/discardable_shared_memory_heap.cc:25: const char kDiscardableSegmentsDumpName[] = "discardable/segments"; On 2015/07/30 16:00:59, reveman wrote: > We've been avoiding explicit constants for dump name strings. Is this different > or should we just use 'base::StringPrintf("discardable/segments/segment_%d", > ...' below instead? > > Btw, it would be a bit more appropriate if the DiscardableMemoryAllocator > controlled this prefix and just passed it to > DiscardableSharedMemoryHeap::OnMemoryDump but no need to fix that in this CL. Done. https://codereview.chromium.org/1259333003/diff/160001/content/common/discard... content/common/discardable_shared_memory_heap.cc:230: CreateAllocatedObjectsDump(pmd); On 2015/07/30 16:00:59, reveman wrote: > I think it should be the responsibility of the DiscardableMemoryAllocator to > create this node. Yes, But then since i placed the method in this class, i called it here. Moved the method and the calls to allocator. https://codereview.chromium.org/1259333003/diff/160001/content/common/discard... File content/common/discardable_shared_memory_heap.h (right): https://codereview.chromium.org/1259333003/diff/160001/content/common/discard... content/common/discardable_shared_memory_heap.h:93: static void CreateAllocatedObjectsDump( On 2015/07/30 16:00:59, reveman wrote: > I think this should be moved to the allocator implementation instead. See my > other comment. I think it is needed in both implementations, so better to have it in base? https://codereview.chromium.org/1259333003/diff/160001/skia/ext/skia_memory_d... File skia/ext/skia_memory_dump_provider.cc (right): https://codereview.chromium.org/1259333003/diff/160001/skia/ext/skia_memory_d... skia/ext/skia_memory_dump_provider.cc:43: const char* source_name = On 2015/07/30 16:00:59, reveman wrote: > When discardable memory is not used, shouldn't this be set to the return value > of MemoryDumpManager::GetInstance()->system_allocator_pool_name() instead of > MallocDumpProvider::kAllocatedObjects? Done.
https://codereview.chromium.org/1259333003/diff/200001/base/memory/discardabl... File base/memory/discardable_memory_allocator.cc (right): https://codereview.chromium.org/1259333003/diff/200001/base/memory/discardabl... base/memory/discardable_memory_allocator.cc:47: pmd->CreateAllocatorDump("discardable/segments"); The use of segments is a detail of the allocator implementation in content/ and should not exist here. https://codereview.chromium.org/1259333003/diff/200001/base/memory/discardabl... base/memory/discardable_memory_allocator.cc:53: discardable_segments_dump->guid()); same here. we shouldn't be referring to segments here. https://codereview.chromium.org/1259333003/diff/200001/base/memory/discardabl... File base/memory/discardable_memory_allocator.h (right): https://codereview.chromium.org/1259333003/diff/200001/base/memory/discardabl... base/memory/discardable_memory_allocator.h:30: // Used by other allocators to expess sub-allocation from discardable memory express https://codereview.chromium.org/1259333003/diff/200001/base/memory/discardabl... base/memory/discardable_memory_allocator.h:34: // GetMemoryPoolNameForTracing()); Not sure we need this much detail in this comment. A reference to "skia" in base/ feels a bit awkward. How about simply: // Returns the memory pool name used by discardable memory allocators for tracing purposes. and the details for how to use that can be documented somewhere else. https://codereview.chromium.org/1259333003/diff/200001/base/memory/discardabl... base/memory/discardable_memory_allocator.h:43: trace_event::ProcessMemoryDump* pmd); Not sure it's worth having this here just for code reuse. See my other comment.
Removed the method. https://codereview.chromium.org/1259333003/diff/200001/base/memory/discardabl... File base/memory/discardable_memory_allocator.cc (right): https://codereview.chromium.org/1259333003/diff/200001/base/memory/discardabl... base/memory/discardable_memory_allocator.cc:47: pmd->CreateAllocatorDump("discardable/segments"); On 2015/07/30 21:27:56, reveman wrote: > The use of segments is a detail of the allocator implementation in content/ and > should not exist here. Done. https://codereview.chromium.org/1259333003/diff/200001/base/memory/discardabl... base/memory/discardable_memory_allocator.cc:53: discardable_segments_dump->guid()); On 2015/07/30 21:27:56, reveman wrote: > same here. we shouldn't be referring to segments here. Done. https://codereview.chromium.org/1259333003/diff/200001/base/memory/discardabl... File base/memory/discardable_memory_allocator.h (right): https://codereview.chromium.org/1259333003/diff/200001/base/memory/discardabl... base/memory/discardable_memory_allocator.h:30: // Used by other allocators to expess sub-allocation from discardable memory On 2015/07/30 21:27:56, reveman wrote: > express Done. https://codereview.chromium.org/1259333003/diff/200001/base/memory/discardabl... base/memory/discardable_memory_allocator.h:34: // GetMemoryPoolNameForTracing()); On 2015/07/30 21:27:56, reveman wrote: > Not sure we need this much detail in this comment. A reference to "skia" in > base/ feels a bit awkward. How about simply: > > // Returns the memory pool name used by discardable memory allocators for > tracing purposes. > > > and the details for how to use that can be documented somewhere else. Done. https://codereview.chromium.org/1259333003/diff/200001/base/memory/discardabl... base/memory/discardable_memory_allocator.h:43: trace_event::ProcessMemoryDump* pmd); On 2015/07/30 21:27:56, reveman wrote: > Not sure it's worth having this here just for code reuse. See my other comment. Moved it to the impl. Thanks |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
