Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1100073004:
Adding discardable memory dump provider. (Closed)
DescriptionAdding discardable memory dump provider.
This CL adds a dump provider to dump discardable memory statistics to
chrome://tracing. The ChildDiscardableSharedMemoryManager implements
MemoryDumpProvider to dump the required stats. The details about each
of the memory segment allocated by the manager is dumped.
BUG=466141
Committed: https://crrev.com/3c7fbd4be81411b4c7553813b29de982a81eec0b
Cr-Commit-Position: refs/heads/master@{#328354}
Patch Set 1 #
Total comments: 22
Patch Set 2 : Addressing comments. #
Total comments: 16
Patch Set 3 : Addressed comments. #
Total comments: 18
Patch Set 4 : Fixed comments. #
Total comments: 18
Patch Set 5 : Fixed comments. #
Total comments: 1
Patch Set 6 : Fixing tests. #
Total comments: 1
Patch Set 7 : Return value -> void. #
Total comments: 1
Patch Set 8 : Rebase for interface changes. #
Total comments: 2
Patch Set 9 : Naming the id. #
Total comments: 17
Patch Set 10 : Changing return value void -> bool. #
Total comments: 3
Patch Set 11 : Making return value void. #
Total comments: 2
Patch Set 12 : Changing c_str parameter to string. #
Messages
Total messages: 49 (8 generated)
ssid@chromium.org changed reviewers: + primiano@chromium.org, reveman@chromium.org
This adds details about each memory segment, but not have spans. PTAL
Hmm I'd use the same design that we used when handling the V8 API change, where you have two methods in discardable_memory_heap: GetMemoryStats(*output_stats) GetHeapStats(int segment_index, *output_segment_stats) Instead of asking the DMHeap to push_back into an array. This would remove any use of dynamic memory allocation in the DumpInto, which doesn't make me sleep well (allocating while dumping memory) https://codereview.chromium.org/1100073004/diff/1/content/child/child_discard... File content/child/child_discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/1100073004/diff/1/content/child/child_discard... content/child/child_discardable_shared_memory_manager.cc:96: base::trace_event::MemoryDumpManager::GetInstance()->UnregisterDumpProvider( Considering that this is not a singleton, you should specify the task_runner in the parent ctor to make sure that all the calls to this happen on the child thread. I do understand that the ChildDSMM is able to work on all threads. The problem however is that you can causes thread races during the shutdown path. Imagine the following scenario: - Child thread is shutting down - The ChildDSMM is about to be destroyed and unregistered. - Another thread requests the dump - The MDM will invoke the DUmpInto onto whatever thread, while the CDSMM is being destroyed. Even if you don't hit the race, the MDM will DCHECK that if you unregister a provider, the provider has a task_runner and you are on the right thread. https://codereview.chromium.org/1100073004/diff/1/content/common/discardable_... File content/common/discardable_shared_memory_heap.h (right): https://codereview.chromium.org/1100073004/diff/1/content/common/discardable_... content/common/discardable_shared_memory_heap.h:49: class CONTENT_EXPORT MemoryStatistics { do you really need CONTENT_EXPORT here? https://codereview.chromium.org/1100073004/diff/1/content/common/discardable_... content/common/discardable_shared_memory_heap.h:51: class SegmentStatistics { make this a struct and remove public:
https://codereview.chromium.org/1100073004/diff/1/content/child/child_discard... File content/child/child_discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/1100073004/diff/1/content/child/child_discard... content/child/child_discardable_shared_memory_manager.cc:33: const char kMemoryDumperFriendlyName[] = "ChildDiscardable"; is "Discardable" enough? we're not using the "Child" prefix for the trace counter or UMA histogram. https://codereview.chromium.org/1100073004/diff/1/content/child/child_discard... content/child/child_discardable_shared_memory_manager.cc:306: std::string segment_id = base::StringPrintf("segment_%zu", segment); nit: looking at process_memory_dump.h it seems like the current convention is "segment%zu". So no underscore. https://codereview.chromium.org/1100073004/diff/1/content/child/child_discard... content/child/child_discardable_shared_memory_manager.cc:308: pmd->CreateAllocatorDump("child_discardable_memory", nit: is "discardable" enough? omitting "child_" to be consistent with other stats such as "DiscardableMemoryUsage" trace counter and "_memory" suffix as that's implicit from the fact that we're creating a memory allocator dump. https://codereview.chromium.org/1100073004/diff/1/content/child/child_discard... File content/child/child_discardable_shared_memory_manager.h (right): https://codereview.chromium.org/1100073004/diff/1/content/child/child_discard... content/child/child_discardable_shared_memory_manager.h:39: // base::trace_event::MemoryDumpProvider implementation: nit: "Overridden from base::trace_event::MemoryDumpProvider" to be consistent with the comment above https://codereview.chromium.org/1100073004/diff/1/content/child/child_discard... content/child/child_discardable_shared_memory_manager.h:41: const char* GetFriendlyName() const override; nit: please move this functions just below the functions that implement base::DiscardableMemoryAllocator and have the order in the .cc file match. The idea is to have functions that implement different interfaces above other functions. https://codereview.chromium.org/1100073004/diff/1/content/common/discardable_... File content/common/discardable_shared_memory_heap.cc (right): https://codereview.chromium.org/1100073004/diff/1/content/common/discardable_... content/common/discardable_shared_memory_heap.cc:65: MemoryStatistics::SegmentStatistics* segment_stats) { I prefer if this was kept consistent with DiscardableSharedMemoryHeap::ReleaseMemory. ie. a private DiscardableSharedMemoryHeap function rather than a ScopedMemorySegment function. https://codereview.chromium.org/1100073004/diff/1/content/common/discardable_... File content/common/discardable_shared_memory_heap.h (right): https://codereview.chromium.org/1100073004/diff/1/content/common/discardable_... content/common/discardable_shared_memory_heap.h:108: void GetMemoryStatistics(MemoryStatistics* statistics); Why don't we just add "void DumpInto(base::trace_event::ProcessMemoryDump* pmd)" instead of having to define and build a temporary struct+vector for this? ChildDMM can just ask the heap instance to dump its stats...
Please find comments inline. https://codereview.chromium.org/1100073004/diff/1/content/common/discardable_... File content/common/discardable_shared_memory_heap.cc (right): https://codereview.chromium.org/1100073004/diff/1/content/common/discardable_... content/common/discardable_shared_memory_heap.cc:65: MemoryStatistics::SegmentStatistics* segment_stats) { On 2015/04/23 17:32:30, reveman wrote: > I prefer if this was kept consistent with > DiscardableSharedMemoryHeap::ReleaseMemory. ie. a private > DiscardableSharedMemoryHeap function rather than a ScopedMemorySegment function. shared_memory is private field here, not shared with the heap class. Do you suggest sharing the member or having friend class? Else we can have a proxy function which calls back by passing shared_memory to heap, but code wouldn't be readable. https://codereview.chromium.org/1100073004/diff/1/content/common/discardable_... File content/common/discardable_shared_memory_heap.h (right): https://codereview.chromium.org/1100073004/diff/1/content/common/discardable_... content/common/discardable_shared_memory_heap.h:108: void GetMemoryStatistics(MemoryStatistics* statistics); On 2015/04/23 17:32:30, reveman wrote: > Why don't we just add "void DumpInto(base::trace_event::ProcessMemoryDump* pmd)" > instead of having to define and build a temporary struct+vector for this? > ChildDMM can just ask the heap instance to dump its stats... This sounds like a good idea, since later we would have to get details about spans, and then the api will have to change again. primiano@ WDYT?
https://codereview.chromium.org/1100073004/diff/1/content/common/discardable_... File content/common/discardable_shared_memory_heap.cc (right): https://codereview.chromium.org/1100073004/diff/1/content/common/discardable_... content/common/discardable_shared_memory_heap.cc:65: MemoryStatistics::SegmentStatistics* segment_stats) { On 2015/04/23 at 18:03:07, ssid wrote: > On 2015/04/23 17:32:30, reveman wrote: > > I prefer if this was kept consistent with > > DiscardableSharedMemoryHeap::ReleaseMemory. ie. a private > > DiscardableSharedMemoryHeap function rather than a ScopedMemorySegment function. > > shared_memory is private field here, not shared with the heap class. Do you suggest sharing the member or having friend class? > Else we can have a proxy function which calls back by passing shared_memory to heap, but code wouldn't be readable. I think Heap class is already a friend.
Made changes, PTAL. https://codereview.chromium.org/1100073004/diff/1/content/child/child_discard... File content/child/child_discardable_shared_memory_manager.cc (right): https://codereview.chromium.org/1100073004/diff/1/content/child/child_discard... content/child/child_discardable_shared_memory_manager.cc:33: const char kMemoryDumperFriendlyName[] = "ChildDiscardable"; On 2015/04/23 17:32:30, reveman wrote: > is "Discardable" enough? we're not using the "Child" prefix for the trace > counter or UMA histogram. Done. https://codereview.chromium.org/1100073004/diff/1/content/child/child_discard... content/child/child_discardable_shared_memory_manager.cc:96: base::trace_event::MemoryDumpManager::GetInstance()->UnregisterDumpProvider( On 2015/04/23 16:11:55, Primiano Tucci wrote: > Considering that this is not a singleton, you should specify the task_runner in > the parent ctor to make sure that all the calls to this happen on the child > thread. > I do understand that the ChildDSMM is able to work on all threads. The problem > however is that you can causes thread races during the shutdown path. Imagine > the following scenario: > > - Child thread is shutting down > - The ChildDSMM is about to be destroyed and unregistered. > - Another thread requests the dump > - The MDM will invoke the DUmpInto onto whatever thread, while the CDSMM is > being destroyed. > > Even if you don't hit the race, the MDM will DCHECK that if you unregister a > provider, the provider has a task_runner and you are on the right thread. Thanks. done https://codereview.chromium.org/1100073004/diff/1/content/child/child_discard... content/child/child_discardable_shared_memory_manager.cc:306: std::string segment_id = base::StringPrintf("segment_%zu", segment); On 2015/04/23 17:32:30, reveman wrote: > nit: looking at process_memory_dump.h it seems like the current convention is > "segment%zu". So no underscore. I used '_' in V8 provider. Primiano, WDYT? https://codereview.chromium.org/1100073004/diff/1/content/child/child_discard... content/child/child_discardable_shared_memory_manager.cc:308: pmd->CreateAllocatorDump("child_discardable_memory", On 2015/04/23 17:32:30, reveman wrote: > nit: is "discardable" enough? omitting "child_" to be consistent with other > stats such as "DiscardableMemoryUsage" trace counter and "_memory" suffix as > that's implicit from the fact that we're creating a memory allocator dump. Yes discardable is enough. https://codereview.chromium.org/1100073004/diff/1/content/child/child_discard... File content/child/child_discardable_shared_memory_manager.h (right): https://codereview.chromium.org/1100073004/diff/1/content/child/child_discard... content/child/child_discardable_shared_memory_manager.h:39: // base::trace_event::MemoryDumpProvider implementation: On 2015/04/23 17:32:30, reveman wrote: > nit: "Overridden from base::trace_event::MemoryDumpProvider" to be consistent > with the comment above Done. https://codereview.chromium.org/1100073004/diff/1/content/child/child_discard... content/child/child_discardable_shared_memory_manager.h:41: const char* GetFriendlyName() const override; On 2015/04/23 17:32:30, reveman wrote: > nit: please move this functions just below the functions that implement > base::DiscardableMemoryAllocator and have the order in the .cc file match. The > idea is to have functions that implement different interfaces above other > functions. Done. https://codereview.chromium.org/1100073004/diff/1/content/common/discardable_... File content/common/discardable_shared_memory_heap.cc (right): https://codereview.chromium.org/1100073004/diff/1/content/common/discardable_... content/common/discardable_shared_memory_heap.cc:65: MemoryStatistics::SegmentStatistics* segment_stats) { On 2015/04/23 18:11:53, reveman wrote: > On 2015/04/23 at 18:03:07, ssid wrote: > > On 2015/04/23 17:32:30, reveman wrote: > > > I prefer if this was kept consistent with > > > DiscardableSharedMemoryHeap::ReleaseMemory. ie. a private > > > DiscardableSharedMemoryHeap function rather than a ScopedMemorySegment > function. > > > > shared_memory is private field here, not shared with the heap class. Do you > suggest sharing the member or having friend class? > > Else we can have a proxy function which calls back by passing shared_memory to > heap, but code wouldn't be readable. > > I think Heap class is already a friend. Made it friend now. https://codereview.chromium.org/1100073004/diff/1/content/common/discardable_... File content/common/discardable_shared_memory_heap.h (right): https://codereview.chromium.org/1100073004/diff/1/content/common/discardable_... content/common/discardable_shared_memory_heap.h:49: class CONTENT_EXPORT MemoryStatistics { On 2015/04/23 16:11:55, Primiano Tucci wrote: > do you really need CONTENT_EXPORT here? No, removed. https://codereview.chromium.org/1100073004/diff/1/content/common/discardable_... content/common/discardable_shared_memory_heap.h:51: class SegmentStatistics { On 2015/04/23 16:11:55, Primiano Tucci wrote: > make this a struct and remove public: Changed.
https://codereview.chromium.org/1100073004/diff/20001/content/common/discarda... File content/common/discardable_shared_memory_heap.cc (right): https://codereview.chromium.org/1100073004/diff/20001/content/common/discarda... content/common/discardable_shared_memory_heap.cc:218: for (size_t segment = 0; segment < memory_segments_.size(); segment++) { Would be nice to do something c++-ish like ReleaseFreeMemory here. e.g. std::for_each(memory_segments_.begin(), memory_segments_.end(), [pmd](const ScopedMemorySegment* segment) { segment->DumpInto(pmd); }), memory_segments_.end()); https://codereview.chromium.org/1100073004/diff/20001/content/common/discarda... content/common/discardable_shared_memory_heap.cc:219: std::string segment_id = base::StringPrintf("segment_%zu", segment); nit: maybe move "segment" string to a kMemoryAllocatorHeapNamePrefix constant https://codereview.chromium.org/1100073004/diff/20001/content/common/discarda... content/common/discardable_shared_memory_heap.cc:221: pmd->CreateAllocatorDump("discardable", segment_id.c_str()); nit: and maybe move this string to a kMemoryAllocatorName constant https://codereview.chromium.org/1100073004/diff/20001/content/common/discarda... content/common/discardable_shared_memory_heap.cc:223: size_t num_of_objects = 0; nit: allocated_objects_count Might as well keep this consistent with setters instead of using some other names here... https://codereview.chromium.org/1100073004/diff/20001/content/common/discarda... content/common/discardable_shared_memory_heap.cc:224: size_t used_size = 0; nit: allocated_objects_size_in_bytes https://codereview.chromium.org/1100073004/diff/20001/content/common/discarda... File content/common/discardable_shared_memory_heap.h (right): https://codereview.chromium.org/1100073004/diff/20001/content/common/discarda... content/common/discardable_shared_memory_heap.h:103: friend class DiscardableSharedMemoryHeap; hm, sorry I missed this in my previous review. What do you think about removing this friend class and instead add a public DumpInto() function that just calls heap_->DumpMemoryInto(shared_memory_.get(), size_, id_)? It's not ideal to have this type of indirection but that's what we have for IsUsed, IsResident so I think it would be the most consistent way to implement this right now. https://codereview.chromium.org/1100073004/diff/20001/content/common/discarda... content/common/discardable_shared_memory_heap.h:107: const size_t size_; I think we should add "const DiscardableSharedMemoryId id_;" here and use that as segment id. This id is unique and won't change between dumps like the vector index. https://codereview.chromium.org/1100073004/diff/20001/content/common/discarda... content/common/discardable_shared_memory_heap.h:122: size_t size); to do what I suggested above we'd need something like this here: void DumpMemoryInto(const base::DiscardableSharedMemory* shared_memory, size_t size, DiscardableSharedMemoryId id);
Made changes, PTAL. https://codereview.chromium.org/1100073004/diff/20001/content/common/discarda... File content/common/discardable_shared_memory_heap.cc (right): https://codereview.chromium.org/1100073004/diff/20001/content/common/discarda... content/common/discardable_shared_memory_heap.cc:218: for (size_t segment = 0; segment < memory_segments_.size(); segment++) { On 2015/04/24 14:19:36, reveman wrote: > Would be nice to do something c++-ish like ReleaseFreeMemory here. e.g. > > std::for_each(memory_segments_.begin(), memory_segments_.end(), > [pmd](const ScopedMemorySegment* segment) { > segment->DumpInto(pmd); > }), memory_segments_.end()); Thanks done. https://codereview.chromium.org/1100073004/diff/20001/content/common/discarda... content/common/discardable_shared_memory_heap.cc:219: std::string segment_id = base::StringPrintf("segment_%zu", segment); On 2015/04/24 14:19:36, reveman wrote: > nit: maybe move "segment" string to a kMemoryAllocatorHeapNamePrefix constant Done. https://codereview.chromium.org/1100073004/diff/20001/content/common/discarda... content/common/discardable_shared_memory_heap.cc:221: pmd->CreateAllocatorDump("discardable", segment_id.c_str()); On 2015/04/24 14:19:36, reveman wrote: > nit: and maybe move this string to a kMemoryAllocatorName constant Done. https://codereview.chromium.org/1100073004/diff/20001/content/common/discarda... content/common/discardable_shared_memory_heap.cc:223: size_t num_of_objects = 0; On 2015/04/24 14:19:36, reveman wrote: > nit: allocated_objects_count > > Might as well keep this consistent with setters instead of using some other > names here... Done. https://codereview.chromium.org/1100073004/diff/20001/content/common/discarda... content/common/discardable_shared_memory_heap.cc:224: size_t used_size = 0; On 2015/04/24 14:19:36, reveman wrote: > nit: allocated_objects_size_in_bytes Done. https://codereview.chromium.org/1100073004/diff/20001/content/common/discarda... File content/common/discardable_shared_memory_heap.h (right): https://codereview.chromium.org/1100073004/diff/20001/content/common/discarda... content/common/discardable_shared_memory_heap.h:103: friend class DiscardableSharedMemoryHeap; On 2015/04/24 14:19:36, reveman wrote: > hm, sorry I missed this in my previous review. What do you think about removing > this friend class and instead add a public DumpInto() function that just calls > heap_->DumpMemoryInto(shared_memory_.get(), size_, id_)? It's not ideal to have > this type of indirection but that's what we have for IsUsed, IsResident so I > think it would be the most consistent way to implement this right now. Yes, I tried to explain this indirection in last patch, I wasn't clear. made changes now. https://codereview.chromium.org/1100073004/diff/20001/content/common/discarda... content/common/discardable_shared_memory_heap.h:107: const size_t size_; On 2015/04/24 14:19:36, reveman wrote: > I think we should add "const DiscardableSharedMemoryId id_;" here and use that > as segment id. This id is unique and won't change between dumps like the vector > index. Done. https://codereview.chromium.org/1100073004/diff/20001/content/common/discarda... content/common/discardable_shared_memory_heap.h:122: size_t size); On 2015/04/24 14:19:36, reveman wrote: > to do what I suggested above we'd need something like this here: > > void DumpMemoryInto(const base::DiscardableSharedMemory* shared_memory, > size_t size, > DiscardableSharedMemoryId id); Also with ProcessMemoryDump* .
https://codereview.chromium.org/1100073004/diff/40001/content/common/discarda... File content/common/discardable_shared_memory_heap.cc (right): https://codereview.chromium.org/1100073004/diff/40001/content/common/discarda... content/common/discardable_shared_memory_heap.cc:111: g_next_discardable_shared_memory_id.GetNext(); Please pass the ID as a parameter instead. ChildDiscardableSharedMemoryManager has an ID for each segment already and that ID is global and known to the child process. https://codereview.chromium.org/1100073004/diff/40001/content/common/discarda... content/common/discardable_shared_memory_heap.cc:348: size_t segment_size, nit: s/segment_size/size/ to be consistent with ReleaseMemory. https://codereview.chromium.org/1100073004/diff/40001/content/common/discarda... content/common/discardable_shared_memory_heap.cc:351: std::string segment_id = nit: maybe heap_name instead https://codereview.chromium.org/1100073004/diff/40001/content/common/discarda... content/common/discardable_shared_memory_heap.cc:353: base::trace_event::MemoryAllocatorDump* segment_dump = fyi, "dump" is enough if the cleans things up, up to you. https://codereview.chromium.org/1100073004/diff/40001/content/common/discarda... content/common/discardable_shared_memory_heap.cc:354: pmd->CreateAllocatorDump(kMemoryAllocatorName, segment_id.c_str()); What if this fails? Looks like other dump providers handle that by returning false.. https://codereview.chromium.org/1100073004/diff/40001/content/common/discarda... content/common/discardable_shared_memory_heap.cc:374: static_cast<uint64>(allocated_objects_size_in_bytes)); are these static_casts really needed? https://codereview.chromium.org/1100073004/diff/40001/content/common/discarda... File content/common/discardable_shared_memory_heap.h (right): https://codereview.chromium.org/1100073004/diff/40001/content/common/discarda... content/common/discardable_shared_memory_heap.h:103: void DumpMemoryStatisticsInto( nit: maybe use the same name as MemoryDumpProvider (just "DumpInto") so we remember to keep this consistent in case MemoryDumpProvider change.. https://codereview.chromium.org/1100073004/diff/40001/content/common/discarda... content/common/discardable_shared_memory_heap.h:126: void DumpSegmentStatistics(const base::DiscardableSharedMemory* shared_memory, nit: same here, maybe keep this consistent: DumpInto
Thanks made changes. https://codereview.chromium.org/1100073004/diff/40001/content/common/discarda... File content/common/discardable_shared_memory_heap.cc (right): https://codereview.chromium.org/1100073004/diff/40001/content/common/discarda... content/common/discardable_shared_memory_heap.cc:111: g_next_discardable_shared_memory_id.GetNext(); On 2015/04/27 13:28:11, reveman wrote: > Please pass the ID as a parameter instead. ChildDiscardableSharedMemoryManager > has an ID for each segment already and that ID is global and known to the child > process. Oh i dint realize, thanks. https://codereview.chromium.org/1100073004/diff/40001/content/common/discarda... content/common/discardable_shared_memory_heap.cc:348: size_t segment_size, On 2015/04/27 13:28:11, reveman wrote: > nit: s/segment_size/size/ to be consistent with ReleaseMemory. Done. https://codereview.chromium.org/1100073004/diff/40001/content/common/discarda... content/common/discardable_shared_memory_heap.cc:351: std::string segment_id = On 2015/04/27 13:28:11, reveman wrote: > nit: maybe heap_name instead Done. https://codereview.chromium.org/1100073004/diff/40001/content/common/discarda... content/common/discardable_shared_memory_heap.cc:353: base::trace_event::MemoryAllocatorDump* segment_dump = On 2015/04/27 13:28:11, reveman wrote: > fyi, "dump" is enough if the cleans things up, up to you. Done. https://codereview.chromium.org/1100073004/diff/40001/content/common/discarda... content/common/discardable_shared_memory_heap.cc:354: pmd->CreateAllocatorDump(kMemoryAllocatorName, segment_id.c_str()); On 2015/04/27 13:28:11, reveman wrote: > What if this fails? Looks like other dump providers handle that by returning > false.. The check is unnecessary, will be removed in other dump providers. https://codereview.chromium.org/1100073004/diff/40001/content/common/discarda... content/common/discardable_shared_memory_heap.cc:374: static_cast<uint64>(allocated_objects_size_in_bytes)); On 2015/04/27 13:28:11, reveman wrote: > are these static_casts really needed? yes, since windows build gives conversion error from size_t to int. https://codereview.chromium.org/1100073004/diff/40001/content/common/discarda... File content/common/discardable_shared_memory_heap.h (right): https://codereview.chromium.org/1100073004/diff/40001/content/common/discarda... content/common/discardable_shared_memory_heap.h:103: void DumpMemoryStatisticsInto( On 2015/04/27 13:28:11, reveman wrote: > nit: maybe use the same name as MemoryDumpProvider (just "DumpInto") so we > remember to keep this consistent in case MemoryDumpProvider change.. Done. https://codereview.chromium.org/1100073004/diff/40001/content/common/discarda... content/common/discardable_shared_memory_heap.h:126: void DumpSegmentStatistics(const base::DiscardableSharedMemory* shared_memory, On 2015/04/27 13:28:11, reveman wrote: > nit: same here, maybe keep this consistent: DumpInto Done.
lgtm after fixing this last set of nits https://codereview.chromium.org/1100073004/diff/40001/content/common/discarda... File content/common/discardable_shared_memory_heap.cc (right): https://codereview.chromium.org/1100073004/diff/40001/content/common/discarda... content/common/discardable_shared_memory_heap.cc:354: pmd->CreateAllocatorDump(kMemoryAllocatorName, segment_id.c_str()); On 2015/04/27 14:27:01, ssid wrote: > On 2015/04/27 13:28:11, reveman wrote: > > What if this fails? Looks like other dump providers handle that by returning > > false.. > > The check is unnecessary, will be removed in other dump providers. Acknowledged. https://codereview.chromium.org/1100073004/diff/40001/content/common/discarda... content/common/discardable_shared_memory_heap.cc:374: static_cast<uint64>(allocated_objects_size_in_bytes)); On 2015/04/27 14:27:01, ssid wrote: > On 2015/04/27 13:28:11, reveman wrote: > > are these static_casts really needed? > > yes, since windows build gives conversion error from size_t to int. hm, I would expect size_t to uint64 to always be ok https://codereview.chromium.org/1100073004/diff/60001/content/common/discarda... File content/common/discardable_shared_memory_heap.cc (right): https://codereview.chromium.org/1100073004/diff/60001/content/common/discarda... content/common/discardable_shared_memory_heap.cc:9: #include "base/atomic_sequence_num.h" dont' need this anymore https://codereview.chromium.org/1100073004/diff/60001/content/common/discarda... content/common/discardable_shared_memory_heap.cc:16: const char kMemoryAllocatorHeapNamePrefix[] = "segment_%zu"; nit: maybe have the prefix be "segment" and the printf string be "%s_%zu" instead https://codereview.chromium.org/1100073004/diff/60001/content/common/discarda... content/common/discardable_shared_memory_heap.cc:43: DiscardableSharedMemoryId id, int32_t to avoid circular dep https://codereview.chromium.org/1100073004/diff/60001/content/common/discarda... content/common/discardable_shared_memory_heap.cc:90: DiscardableSharedMemoryId id, int32_t to avoid circular dep https://codereview.chromium.org/1100073004/diff/60001/content/common/discarda... File content/common/discardable_shared_memory_heap.h (right): https://codereview.chromium.org/1100073004/diff/60001/content/common/discarda... content/common/discardable_shared_memory_heap.h:15: #include "content/common/host_discardable_shared_memory_manager.h" nit: this might be a circular dependency in the future as the host might want to use this class. Let's avoid it be just using int32_t below instead for now. Ideally we'd define this somewhere else but I don't think we have to worry about that for now. https://codereview.chromium.org/1100073004/diff/60001/content/common/discarda... content/common/discardable_shared_memory_heap.h:58: DiscardableSharedMemoryId id, int32_t to avoid circular dep https://codereview.chromium.org/1100073004/diff/60001/content/common/discarda... content/common/discardable_shared_memory_heap.h:98: DiscardableSharedMemoryId id, int32_t to avoid circular dep https://codereview.chromium.org/1100073004/diff/60001/content/common/discarda... content/common/discardable_shared_memory_heap.h:131: size_t segment_size, nit: s/segment_size/size/ https://codereview.chromium.org/1100073004/diff/60001/content/common/discarda... content/common/discardable_shared_memory_heap.h:132: DiscardableSharedMemoryId id, int32_t to avoid circular dep
Thanks. https://codereview.chromium.org/1100073004/diff/60001/content/common/discarda... File content/common/discardable_shared_memory_heap.cc (right): https://codereview.chromium.org/1100073004/diff/60001/content/common/discarda... content/common/discardable_shared_memory_heap.cc:9: #include "base/atomic_sequence_num.h" On 2015/04/27 15:08:58, reveman wrote: > dont' need this anymore Done. https://codereview.chromium.org/1100073004/diff/60001/content/common/discarda... content/common/discardable_shared_memory_heap.cc:16: const char kMemoryAllocatorHeapNamePrefix[] = "segment_%zu"; On 2015/04/27 15:08:58, reveman wrote: > nit: maybe have the prefix be "segment" and the printf string be "%s_%zu" > instead Done. https://codereview.chromium.org/1100073004/diff/60001/content/common/discarda... content/common/discardable_shared_memory_heap.cc:43: DiscardableSharedMemoryId id, On 2015/04/27 15:08:58, reveman wrote: > int32_t to avoid circular dep Done. https://codereview.chromium.org/1100073004/diff/60001/content/common/discarda... content/common/discardable_shared_memory_heap.cc:90: DiscardableSharedMemoryId id, On 2015/04/27 15:08:58, reveman wrote: > int32_t to avoid circular dep Done. https://codereview.chromium.org/1100073004/diff/60001/content/common/discarda... File content/common/discardable_shared_memory_heap.h (right): https://codereview.chromium.org/1100073004/diff/60001/content/common/discarda... content/common/discardable_shared_memory_heap.h:15: #include "content/common/host_discardable_shared_memory_manager.h" On 2015/04/27 15:08:58, reveman wrote: > nit: this might be a circular dependency in the future as the host might want to > use this class. Let's avoid it be just using int32_t below instead for now. > Ideally we'd define this somewhere else but I don't think we have to worry about > that for now. Done. https://codereview.chromium.org/1100073004/diff/60001/content/common/discarda... content/common/discardable_shared_memory_heap.h:58: DiscardableSharedMemoryId id, On 2015/04/27 15:08:58, reveman wrote: > int32_t to avoid circular dep Done. https://codereview.chromium.org/1100073004/diff/60001/content/common/discarda... content/common/discardable_shared_memory_heap.h:98: DiscardableSharedMemoryId id, On 2015/04/27 15:08:58, reveman wrote: > int32_t to avoid circular dep Done. https://codereview.chromium.org/1100073004/diff/60001/content/common/discarda... content/common/discardable_shared_memory_heap.h:131: size_t segment_size, On 2015/04/27 15:08:58, reveman wrote: > nit: s/segment_size/size/ Done. https://codereview.chromium.org/1100073004/diff/60001/content/common/discarda... content/common/discardable_shared_memory_heap.h:132: DiscardableSharedMemoryId id, On 2015/04/27 15:08:58, reveman wrote: > int32_t to avoid circular dep Done.
reveman@chromium.org changed reviewers: + avi@chromium.org
+avi for owner review
lgtm https://codereview.chromium.org/1100073004/diff/80001/content/common/discarda... File content/common/discardable_shared_memory_heap.cc (right): https://codereview.chromium.org/1100073004/diff/80001/content/common/discarda... content/common/discardable_shared_memory_heap.cc:368: static_cast<uint64>(allocated_objects_size_in_bytes)); uint64_t. I know MemoryAllocatorDump uses uint64, but they're typedefs, and we're trying to remove those, so we might as well use the correct types here.
one nit but still lgtm https://codereview.chromium.org/1100073004/diff/100001/content/common/discard... File content/common/discardable_shared_memory_heap.h (right): https://codereview.chromium.org/1100073004/diff/100001/content/common/discard... content/common/discardable_shared_memory_heap.h:89: bool DumpInto(base::trace_event::ProcessMemoryDump* pmd); nit: the return value is not used so you can remove it
On 2015/04/27 19:04:00, reveman wrote: > one nit but still lgtm > > https://codereview.chromium.org/1100073004/diff/100001/content/common/discard... > File content/common/discardable_shared_memory_heap.h (right): > > https://codereview.chromium.org/1100073004/diff/100001/content/common/discard... > content/common/discardable_shared_memory_heap.h:89: bool > DumpInto(base::trace_event::ProcessMemoryDump* pmd); > nit: the return value is not used so you can remove it Fixed thanks.
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, reveman@chromium.org Link to the patchset: https://codereview.chromium.org/1100073004/#ps120001 (title: "Return value -> void.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1100073004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/1100073004/diff/120001/content/common/discard... File content/common/discardable_shared_memory_heap.cc (right): https://codereview.chromium.org/1100073004/diff/120001/content/common/discard... content/common/discardable_shared_memory_heap.cc:233: return true; this function doesn't have a return value anymore
This needs to be rebased against crrev.com/1107093004 + crrev.com/1105143003 (Which is landing right now). Essentially the changes required are: - Rename DumpInto -> OnMemoryDump - The task_runner is now an argument of RegisterDumpProvider - Get rid of friendly names. - Use mad->AddScalar rather then set_allocated_object_sizes. See this as an example: https://codereview.chromium.org/1105143003/diff/40001/base/trace_event/winhea... I will review the memory-dump related parts of the patch once you rebase. Thanks.
Made changes PTAL.
picksi@google.com changed reviewers: + picksi@google.com
https://codereview.chromium.org/1100073004/diff/140001/content/common/discard... File content/common/discardable_shared_memory_heap_perftest.cc (right): https://codereview.chromium.org/1100073004/diff/140001/content/common/discard... content/common/discardable_shared_memory_heap_perftest.cc:41: heap.Grow(memory.Pass(), segment_size, 0, base::Bind(NullTask)).Pass()); Is 0 a special ID? Can we pass it as a named static ("kDefaultMemoryID"?) ... I had to find the function definition to understand what this 0 was. This is passed on to a lower level function; is there anything inside that function that defines '0'? If so let's use that.
https://codereview.chromium.org/1100073004/diff/140001/content/common/discard... File content/common/discardable_shared_memory_heap_perftest.cc (right): https://codereview.chromium.org/1100073004/diff/140001/content/common/discard... content/common/discardable_shared_memory_heap_perftest.cc:41: heap.Grow(memory.Pass(), segment_size, 0, base::Bind(NullTask)).Pass()); On 2015/04/29 at 15:30:15, picksi wrote: > Is 0 a special ID? Can we pass it as a named static ("kDefaultMemoryID"?) ... I had to find the function definition to understand what this 0 was. > > This is passed on to a lower level function; is there anything inside that function that defines '0'? If so let's use that. Maybe just add a next_discardable_shared_memory_id variable that we increment
PTAL.
still lgtm
Some nits. Feel free to ignore if you already discussed that with David and I didn't realize. https://codereview.chromium.org/1100073004/diff/160001/content/common/discard... File content/common/discardable_shared_memory_heap.cc (right): https://codereview.chromium.org/1100073004/diff/160001/content/common/discard... content/common/discardable_shared_memory_heap.cc:227: void DiscardableSharedMemoryHeap::OnMemoryDump( this should be a bool and propagate the failures for the individual segment's OnMD https://codereview.chromium.org/1100073004/diff/160001/content/common/discard... content/common/discardable_shared_memory_heap.cc:229: std::for_each(memory_segments_.begin(), memory_segments_.end(), Uh? can't this just be: for(const ScopedMemorySegment* segment : memory_segments_) instead of using the lambda? https://codereview.chromium.org/1100073004/diff/160001/content/common/discard... content/common/discardable_shared_memory_heap.cc:339: void DiscardableSharedMemoryHeap::OnMemoryDump( this should return bool https://codereview.chromium.org/1100073004/diff/160001/content/common/discard... content/common/discardable_shared_memory_heap.cc:341: size_t size, this should be uint64 https://codereview.chromium.org/1100073004/diff/160001/content/common/discard... content/common/discardable_shared_memory_heap.cc:347: pmd->CreateAllocatorDump(heap_name.c_str()); you should check if dump != null and return false. https://codereview.chromium.org/1100073004/diff/160001/content/common/discard... content/common/discardable_shared_memory_heap.cc:349: size_t allocated_objects_count = 0; this should be uint64 (not uint64_t) https://codereview.chromium.org/1100073004/diff/160001/content/common/discard... content/common/discardable_shared_memory_heap.cc:350: size_t allocated_objects_size_in_bytes = 0; this should be uint64 (not uint64_t) https://codereview.chromium.org/1100073004/diff/160001/content/common/discard... content/common/discardable_shared_memory_heap.cc:351: size_t offset = this should be uintptr_t https://codereview.chromium.org/1100073004/diff/160001/content/common/discard... content/common/discardable_shared_memory_heap.cc:353: size_t end = offset + size / block_size_; same here (uintptr_t) https://codereview.chromium.org/1100073004/diff/160001/content/common/discard... content/common/discardable_shared_memory_heap.cc:365: static_cast<uint64_t>(size)); remove all these casts, the variables should be already of the right size if you follow my previous suggestions https://codereview.chromium.org/1100073004/diff/160001/content/common/discard... File content/common/discardable_shared_memory_heap.h (right): https://codereview.chromium.org/1100073004/diff/160001/content/common/discard... content/common/discardable_shared_memory_heap.h:89: void OnMemoryDump(base::trace_event::ProcessMemoryDump* pmd); this should be a bool (See my next comment)
https://codereview.chromium.org/1100073004/diff/160001/content/common/discard... File content/common/discardable_shared_memory_heap.cc (right): https://codereview.chromium.org/1100073004/diff/160001/content/common/discard... content/common/discardable_shared_memory_heap.cc:229: std::for_each(memory_segments_.begin(), memory_segments_.end(), On 2015/04/29 at 17:32:02, Primiano Tucci wrote: > Uh? can't this just be: > for(const ScopedMemorySegment* segment : memory_segments_) instead of using the lambda? It can but I prefer a lamda as it's more consistent with other functions that process each element in the memory_segments_ vector. We can use std::find_if (with a comment to use std::all_of when allowed) here instead if we need to propagate failures. https://codereview.chromium.org/1100073004/diff/160001/content/common/discard... content/common/discardable_shared_memory_heap.cc:341: size_t size, On 2015/04/29 at 17:32:02, Primiano Tucci wrote: > this should be uint64 hm, this is the size of the shared memory segment and size_t is used everywhere in chromium for that. We should change a lot of code, starting at base::SharedMemory::SharedMemoryCreateOptions::size, if we want to change this. https://codereview.chromium.org/1100073004/diff/160001/content/common/discard... content/common/discardable_shared_memory_heap.cc:349: size_t allocated_objects_count = 0; On 2015/04/29 at 17:32:02, Primiano Tucci wrote: > this should be uint64 (not uint64_t) See avi@'s comment. https://codereview.chromium.org/1100073004/diff/160001/content/common/discard... content/common/discardable_shared_memory_heap.cc:351: size_t offset = On 2015/04/29 at 17:32:02, Primiano Tucci wrote: > this should be uintptr_t That might be more correct but make sure to change it throughout the whole file and not just here. I'd leave this as size_t for now and instead make the change in a follow up patch but either way is fine with me.
https://codereview.chromium.org/1100073004/diff/160001/content/common/discard... File content/common/discardable_shared_memory_heap.h (right): https://codereview.chromium.org/1100073004/diff/160001/content/common/discard... content/common/discardable_shared_memory_heap.h:89: void OnMemoryDump(base::trace_event::ProcessMemoryDump* pmd); On 2015/04/29 at 17:32:02, Primiano Tucci wrote: > this should be a bool (See my next comment) Btw, I was under the impression that CreateAllocatorDump would be changed to never fail (never be a reason for OnMemoryDump to return false). Is that not the case?
Nevermind, I missed Avi's comment. Ignore my comments then. > Btw, I was under the impression that CreateAllocatorDump would be changed to > never fail (never be a reason for OnMemoryDump to return false). Is that not the > case? It is already the case today that it never fails (unless OOM, but in that case we don't care too much). I'd just to keep the door open for the future. Rationale, if allocating while dumping turns out to be a problem in future, I can rewrite the implementation of CreateAllocatorDump to use an internal static arena (which might hence run out of space) and nothing else in the clients code will change. I have no plans to do that anytime soon, so it is probably fine as it is.
On 2015/04/29 at 18:35:02, primiano wrote: > Nevermind, I missed Avi's comment. Ignore my comments then. > > > Btw, I was under the impression that CreateAllocatorDump would be changed to > > never fail (never be a reason for OnMemoryDump to return false). Is that not the > > case? > It is already the case today that it never fails (unless OOM, but in that case we don't care too much). I'd just to keep the door open for the future. > Rationale, if allocating while dumping turns out to be a problem in future, I can rewrite the implementation of CreateAllocatorDump to use an internal static arena (which might hence run out of space) and nothing else in the clients code will change. > > I have no plans to do that anytime soon, so it is probably fine as it is. Ok, thanks for explaining. Makes sense. ssid@, how about we keep the bool return value everywhere but always return true from DiscardableSharedMemoryHeap::OnMemoryDump for now and DCHECK that return value from CreateAllocatorDump is not null? That would limit this assumption to just one function and we can fix it in the future if we have to.
On 2015/04/29 18:53:10, reveman wrote: > On 2015/04/29 at 18:35:02, primiano wrote: > > Nevermind, I missed Avi's comment. Ignore my comments then. > > > > > Btw, I was under the impression that CreateAllocatorDump would be changed to > > > never fail (never be a reason for OnMemoryDump to return false). Is that not > the > > > case? > > It is already the case today that it never fails (unless OOM, but in that case > we don't care too much). I'd just to keep the door open for the future. > > Rationale, if allocating while dumping turns out to be a problem in future, I > can rewrite the implementation of CreateAllocatorDump to use an internal static > arena (which might hence run out of space) and nothing else in the clients code > will change. > > > > I have no plans to do that anytime soon, so it is probably fine as it is. > > Ok, thanks for explaining. Makes sense. > > ssid@, how about we keep the bool return value everywhere but always return true > from DiscardableSharedMemoryHeap::OnMemoryDump for now and DCHECK that return > value from CreateAllocatorDump is not null? That would limit this assumption to > just one function and we can fix it in the future if we have to. Made just these changes, PTAL.
https://codereview.chromium.org/1100073004/diff/180001/content/common/discard... File content/common/discardable_shared_memory_heap.cc (right): https://codereview.chromium.org/1100073004/diff/180001/content/common/discard... content/common/discardable_shared_memory_heap.cc:234: is_dump_valid &= segment->OnMemoryDump(pmd); It seems bad to keep trying to dump memory stats after a failure. I think it would be better to just return true at the end of this function for now and not deal with the failure condition at a lower level than this. ie. no return value from ScopedMemorySegment::OnMemoryDump or OnMemoryDump below.
https://codereview.chromium.org/1100073004/diff/160001/content/common/discard... File content/common/discardable_shared_memory_heap.cc (right): https://codereview.chromium.org/1100073004/diff/160001/content/common/discard... content/common/discardable_shared_memory_heap.cc:341: size_t size, On 2015/04/29 18:27:43, reveman wrote: > On 2015/04/29 at 17:32:02, Primiano Tucci wrote: > > this should be uint64 > > hm, this is the size of the shared memory segment and size_t is used everywhere > in chromium for that. We should change a lot of code, starting at > base::SharedMemory::SharedMemoryCreateOptions::size, if we want to change this. AH k, makes sense https://codereview.chromium.org/1100073004/diff/180001/content/common/discard... File content/common/discardable_shared_memory_heap.cc (right): https://codereview.chromium.org/1100073004/diff/180001/content/common/discard... content/common/discardable_shared_memory_heap.cc:234: is_dump_valid &= segment->OnMemoryDump(pmd); On 2015/04/30 16:36:49, reveman wrote: > It seems bad to keep trying to dump memory stats after a failure. I think it > would be better to just return true at the end of this function for now and not > deal with the failure condition at a lower level than this. ie. no return value > from ScopedMemorySegment::OnMemoryDump or OnMemoryDump below. Uhm, not sure I follow. Isn't the other way round? If you fail dumping (and you detect by the boolean retval of OnMemoryDump down there) you are *not* going to keep trying? Or probably I am missing something else here.
https://codereview.chromium.org/1100073004/diff/180001/content/common/discard... File content/common/discardable_shared_memory_heap.cc (right): https://codereview.chromium.org/1100073004/diff/180001/content/common/discard... content/common/discardable_shared_memory_heap.cc:234: is_dump_valid &= segment->OnMemoryDump(pmd); On 2015/04/30 at 16:59:35, Primiano Tucci wrote: > On 2015/04/30 16:36:49, reveman wrote: > > It seems bad to keep trying to dump memory stats after a failure. I think it > > would be better to just return true at the end of this function for now and not > > deal with the failure condition at a lower level than this. ie. no return value > > from ScopedMemorySegment::OnMemoryDump or OnMemoryDump below. > > Uhm, not sure I follow. Isn't the other way round? If you fail dumping (and you detect by the boolean retval of OnMemoryDump down there) you are *not* going to keep trying? Or probably I am missing something else here. std::for_each doesn't abort. we'll call segment->OnMemoryDump on remaining segments after a failure.
I can't find a way to break for_each from lambda function. I have removed the bool return values as suggested. PTAL.
On 2015/05/05 at 10:24:32, ssid wrote: > I can't find a way to break for_each from lambda function. I have removed the bool return values as suggested. PTAL. lgtm
LGTM % 1 comment https://codereview.chromium.org/1100073004/diff/200001/content/common/discard... File content/common/discardable_shared_memory_heap.cc (right): https://codereview.chromium.org/1100073004/diff/200001/content/common/discard... content/common/discardable_shared_memory_heap.cc:349: pmd->CreateAllocatorDump(heap_name.c_str()); No need to c_str here, or you will you convert back and forth between string and char* (creating extra useless copies)
addressed comments. https://codereview.chromium.org/1100073004/diff/200001/content/common/discard... File content/common/discardable_shared_memory_heap.cc (right): https://codereview.chromium.org/1100073004/diff/200001/content/common/discard... content/common/discardable_shared_memory_heap.cc:349: pmd->CreateAllocatorDump(heap_name.c_str()); On 2015/05/05 15:37:15, Primiano Tucci wrote: > No need to c_str here, or you will you convert back and forth between string and > char* (creating extra useless copies) When the patch was uploaded it was char* I guess. Changed.
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, primiano@chromium.org, reveman@chromium.org Link to the patchset: https://codereview.chromium.org/1100073004/#ps220001 (title: "Changing c_str parameter to string.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1100073004/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/3c7fbd4be81411b4c7553813b29de982a81eec0b Cr-Commit-Position: refs/heads/master@{#328354} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
