Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1088683003:
Adding v8_isolate_memory_dump_provider. (Closed)
DescriptionAdding v8_isolate_memory_dump_provider.
This CL adds a dump provider for dumping V8 heap memory statistics into
the tracing. The dump provider gets statistics from isolate and adds to
the dump. Each isolate when created, adds a dump provider. The dump
provider here is owned by IsolateHolder and gets registered when it
IsolateHolder is created and unregistered when IsolateHolder is
destroyed.
BUG=476013
Committed: https://crrev.com/83aa5be892018f800328c1b7d03f6fc37bc22d5b
Cr-Commit-Position: refs/heads/master@{#328946}
Patch Set 1 #Patch Set 2 : #
Total comments: 22
Patch Set 3 : Addressing primiano's comments. #Patch Set 4 : Addressed all comments. #Patch Set 5 : Rebase for primiano's changes. #
Total comments: 12
Patch Set 6 : Fixing build. #
Total comments: 6
Patch Set 7 : Fixed all comments. #Patch Set 8 : Nit. #Patch Set 9 : more nits. #
Total comments: 16
Patch Set 10 : Addressing rmcilroy's comments. #Patch Set 11 : Rebase for using locker. #
Total comments: 6
Patch Set 12 : Fixing TODO. #Patch Set 13 : Fixing build. #Patch Set 14 : Rebase for interface changes and fixing tests. #
Total comments: 2
Patch Set 15 : Fixing message loop check. #
Total comments: 2
Patch Set 16 : Adding dummy task runner. #Patch Set 17 : Moving test fixes to different CL, removing unnecessary casts. #
Total comments: 6
Patch Set 18 : Fixing nits. #
Total comments: 2
Patch Set 19 : Adding heap_spaces child. #
Messages
Total messages: 61 (19 generated)
ssid@chromium.org changed reviewers: + picksi@chromium.org, primiano@chromium.org
Looks great, just have some nits but makes perfect sense. Thanks https://codereview.chromium.org/1088683003/diff/20001/gin/isolate_holder.cc File gin/isolate_holder.cc (right): https://codereview.chromium.org/1088683003/diff/20001/gin/isolate_holder.cc#n... gin/isolate_holder.cc:36: isolate_memory_dump_provider_.reset(new V8IsolateMemoryDumpProvider(this)); I am not sure what isolate_data is, but feel safer that you create this at the very end of this CTOR. https://codereview.chromium.org/1088683003/diff/20001/gin/isolate_holder.cc#n... gin/isolate_holder.cc:66: isolate_memory_dump_provider_.reset(); symmetrically, move this at the very first https://codereview.chromium.org/1088683003/diff/20001/gin/v8_isolate_memory_d... File gin/v8_isolate_memory_dump_provider.cc (right): https://codereview.chromium.org/1088683003/diff/20001/gin/v8_isolate_memory_d... gin/v8_isolate_memory_dump_provider.cc:15: don't need this extra \n https://codereview.chromium.org/1088683003/diff/20001/gin/v8_isolate_memory_d... gin/v8_isolate_memory_dump_provider.cc:17: const char kDumperRootName[] = "v8_root"; This should be just v8 https://codereview.chromium.org/1088683003/diff/20001/gin/v8_isolate_memory_d... gin/v8_isolate_memory_dump_provider.cc:17: const char kDumperRootName[] = "v8_root"; kRootDumpName https://codereview.chromium.org/1088683003/diff/20001/gin/v8_isolate_memory_d... gin/v8_isolate_memory_dump_provider.cc:18: const char kDumperName[] = "v8_isolate"; THis should be kIsolateDumpName https://codereview.chromium.org/1088683003/diff/20001/gin/v8_isolate_memory_d... gin/v8_isolate_memory_dump_provider.cc:19: const char kDumperOtherSpaces[] = "other_spaces"; THis you can just put the string inline as you use it only once https://codereview.chromium.org/1088683003/diff/20001/gin/v8_isolate_memory_d... gin/v8_isolate_memory_dump_provider.cc:25: IsolateHolder* isolate_holder) { After my change here you will need a call to the parent ctor as follows: : MemoryDumpProvider(base::MessageLoop::current()->task_runner()) https://codereview.chromium.org/1088683003/diff/20001/gin/v8_isolate_memory_d... gin/v8_isolate_memory_dump_provider.cc:38: // Called at trace dump point time. Creates a snapshot the memory counters for snapshot +with the... https://codereview.chromium.org/1088683003/diff/20001/gin/v8_isolate_memory_d... gin/v8_isolate_memory_dump_provider.cc:39: // the current process. this is actually the current isolate,not process https://codereview.chromium.org/1088683003/diff/20001/gin/v8_isolate_memory_d... gin/v8_isolate_memory_dump_provider.cc:52: std::stringstream dumper_name; even if it works anyways, here you need an #include for whoever defines stringstream https://codereview.chromium.org/1088683003/diff/20001/gin/v8_isolate_memory_d... gin/v8_isolate_memory_dump_provider.cc:53: dumper_name << kDumperName << thread_id; probably add an underscore as separator here. ALso please add a TODO, the thread_id is soon not going to be unique enough if we get more isolates per thread. https://codereview.chromium.org/1088683003/diff/20001/gin/v8_isolate_memory_d... gin/v8_isolate_memory_dump_provider.cc:60: for (int space = 0; space < number_of_spaces; space++) { probably this should be size_t (match the API) https://codereview.chromium.org/1088683003/diff/20001/gin/v8_isolate_memory_d... gin/v8_isolate_memory_dump_provider.cc:71: pmd->CreateAllocatorDump(space_statistics.space_name(), isolate_dump); Note to myself (primiano): probably this will need to be changed when I'll change the CreateAllocatorDump API. https://codereview.chromium.org/1088683003/diff/20001/gin/v8_isolate_memory_d... gin/v8_isolate_memory_dump_provider.cc:77: space_dump->SetExtraAttribute(kPhysicalSizeAttribute, Maybe add a TODO here, probably this should be our physical_size_in_bytes and the other the attrib. Let's discuss this with a V8 owner. https://codereview.chromium.org/1088683003/diff/20001/gin/v8_isolate_memory_d... gin/v8_isolate_memory_dump_provider.cc:81: base::trace_event::MemoryAllocatorDump* space_dump = Add a comment explaining that here you do the compute the final diff. https://codereview.chromium.org/1088683003/diff/20001/gin/v8_isolate_memory_d... File gin/v8_isolate_memory_dump_provider.h (right): https://codereview.chromium.org/1088683003/diff/20001/gin/v8_isolate_memory_d... gin/v8_isolate_memory_dump_provider.h:5: #ifndef GIN_PUBLIC_V8_ISOLATE_MEMORY_DUMP_PROVIDER_H_ Fix this (remove PUBLIC) https://codereview.chromium.org/1088683003/diff/20001/gin/v8_isolate_memory_d... gin/v8_isolate_memory_dump_provider.h:8: #include <istream> I don't think you need this https://codereview.chromium.org/1088683003/diff/20001/gin/v8_isolate_memory_d... gin/v8_isolate_memory_dump_provider.h:10: #include "base/memory/scoped_ptr.h" don't need this either https://codereview.chromium.org/1088683003/diff/20001/gin/v8_isolate_memory_d... gin/v8_isolate_memory_dump_provider.h:11: #include "base/trace_event/memory_dump_manager.h" Neither this. https://codereview.chromium.org/1088683003/diff/20001/gin/v8_isolate_memory_d... gin/v8_isolate_memory_dump_provider.h:19: // Dump provider which collects process-wide memory stats. I'd probably word this as: // Memory dump provider for the chrome://tracing infrastructure. It dumps summarized memory stats about the V8 Isolates. https://codereview.chromium.org/1088683003/diff/20001/gin/v8_isolate_memory_d... gin/v8_isolate_memory_dump_provider.h:33: IsolateHolder* isolate_holder_; It is sufficient to say: IsolateHolder* isolate_holder_; // Not owned.
+rmcilroy Can you look at the v8_isolate_memory_dump_provider.cc file. This has the data finally shown in the tracing dump. Should the physical_size_in_bytes be from CommittedMemory or CommittedPhysicalMemory?
primiano@ PTAL.
Please rebase this, I made some small changes to the MDProvider class for the attributes (the DeclareAllocatorAttribute method)
PTAL.
Just few comments, looks great! https://codereview.chromium.org/1088683003/diff/80001/gin/v8_isolate_memory_d... File gin/v8_isolate_memory_dump_provider.cc (right): https://codereview.chromium.org/1088683003/diff/80001/gin/v8_isolate_memory_d... gin/v8_isolate_memory_dump_provider.cc:21: const char kIsolateDumpName[] = "isolate"; I think that you could have a string format here (see my suggesiton below), e.g.: kIsolateHeapSpaceFmt = "isolate_%d/%s" https://codereview.chromium.org/1088683003/diff/80001/gin/v8_isolate_memory_d... gin/v8_isolate_memory_dump_provider.cc:52: isolate_name << kIsolateDumpName << '_' << thread_id; You can simplify this by using StringPrintf (#include "base/strings/stringprintf.h") Do just this below: std::string isolate_name = StringPrintf"isolate_%d/heap%d", thread_id, space_name()) https://codereview.chromium.org/1088683003/diff/80001/gin/v8_isolate_memory_d... gin/v8_isolate_memory_dump_provider.cc:86: base::trace_event::MemoryAllocatorDump* space_dump = s/space_dump/other_spaces_dump/ https://codereview.chromium.org/1088683003/diff/80001/gin/v8_isolate_memory_d... gin/v8_isolate_memory_dump_provider.cc:93: Can you please do the accounting of the two extra properties as well? https://codereview.chromium.org/1088683003/diff/80001/gin/v8_isolate_memory_d... File gin/v8_isolate_memory_dump_provider.h (right): https://codereview.chromium.org/1088683003/diff/80001/gin/v8_isolate_memory_d... gin/v8_isolate_memory_dump_provider.h:20: V8IsolateMemoryDumpProvider(IsolateHolder* isolate_holder); When you have a ctor with one argument, you want "explicit V8IsolateMDP(....)", otherwise C++ will derive an implicit cast operator between IsolateHolder and V8IsolateMDP, which you don't really want. https://codereview.chromium.org/1088683003/diff/80001/gin/v8_isolate_memory_d... gin/v8_isolate_memory_dump_provider.h:28: IsolateHolder* isolate_holder_; // Not owned. maybe say: // Not owned (IsolateHolder owns this)
rmcilroy@chromium.org changed reviewers: + rmcilroy@chromium.org
not looked at at the dumper code yet, but a couple of comments while I was looking at this with primiano. https://codereview.chromium.org/1088683003/diff/100001/gin/isolate_holder.cc File gin/isolate_holder.cc (right): https://codereview.chromium.org/1088683003/diff/100001/gin/isolate_holder.cc#... gin/isolate_holder.cc:47: isolate_memory_dump_provider_.reset(new V8IsolateMemoryDumpProvider(this)); nit - move to below isolate_data_.reset https://codereview.chromium.org/1088683003/diff/100001/gin/isolate_holder.cc#... gin/isolate_holder.cc:51: isolate_memory_dump_provider_.reset(); Do you need this reset? The scoped_ptr will delete the referenced object at the end of the destructor anyway so this is only needed if you explicitly need to delete it before the destructor code.
https://codereview.chromium.org/1088683003/diff/100001/gin/v8_isolate_memory_... File gin/v8_isolate_memory_dump_provider.cc (right): https://codereview.chromium.org/1088683003/diff/100001/gin/v8_isolate_memory_... gin/v8_isolate_memory_dump_provider.cc:81: space_dump->SetAttribute( Had a chat with Ross, just delete this attribute as it is a bit misleading and will very likely confuse people .
https://codereview.chromium.org/1088683003/diff/100001/gin/isolate_holder.cc File gin/isolate_holder.cc (right): https://codereview.chromium.org/1088683003/diff/100001/gin/isolate_holder.cc#... gin/isolate_holder.cc:51: isolate_memory_dump_provider_.reset(); On 2015/04/21 17:13:03, rmcilroy wrote: > Do you need this reset? The scoped_ptr will delete the referenced object at the > end of the destructor anyway so this is only needed if you explicitly need to > delete it before the destructor code. It was added here so that before the object gets unregistered, if the isolate gets destroyed, then it will cause a crash.
On 2015/04/21 17:17:49, ssid wrote: > https://codereview.chromium.org/1088683003/diff/100001/gin/isolate_holder.cc > File gin/isolate_holder.cc (right): > > https://codereview.chromium.org/1088683003/diff/100001/gin/isolate_holder.cc#... > gin/isolate_holder.cc:51: isolate_memory_dump_provider_.reset(); > On 2015/04/21 17:13:03, rmcilroy wrote: > > Do you need this reset? The scoped_ptr will delete the referenced object at > the > > end of the destructor anyway so this is only needed if you explicitly need to > > delete it before the destructor code. > It was added here so that before the dumper gets unregistered before isolate gets destroyed. Else it might cause a crash while tracing.
Made changes. PTAL. https://codereview.chromium.org/1088683003/diff/80001/gin/v8_isolate_memory_d... File gin/v8_isolate_memory_dump_provider.cc (right): https://codereview.chromium.org/1088683003/diff/80001/gin/v8_isolate_memory_d... gin/v8_isolate_memory_dump_provider.cc:21: const char kIsolateDumpName[] = "isolate"; On 2015/04/21 16:43:42, Primiano Tucci wrote: > I think that you could have a string format here (see my suggesiton below), > e.g.: > > kIsolateHeapSpaceFmt = "isolate_%d/%s" Done. https://codereview.chromium.org/1088683003/diff/80001/gin/v8_isolate_memory_d... gin/v8_isolate_memory_dump_provider.cc:52: isolate_name << kIsolateDumpName << '_' << thread_id; On 2015/04/21 16:43:42, Primiano Tucci wrote: > You can simplify this by using StringPrintf (#include > "base/strings/stringprintf.h") > > Do just this below: > std::string isolate_name = StringPrintf"isolate_%d/heap%d", thread_id, > space_name()) Done. https://codereview.chromium.org/1088683003/diff/80001/gin/v8_isolate_memory_d... gin/v8_isolate_memory_dump_provider.cc:86: base::trace_event::MemoryAllocatorDump* space_dump = On 2015/04/21 16:43:42, Primiano Tucci wrote: > s/space_dump/other_spaces_dump/ Note: other spaces are named as : new_space, old_space, etc ... https://codereview.chromium.org/1088683003/diff/80001/gin/v8_isolate_memory_d... gin/v8_isolate_memory_dump_provider.cc:93: On 2015/04/21 16:43:42, Primiano Tucci wrote: > Can you please do the accounting of the two extra properties as well? Cant be done for available memory because, there is no total available space returned by getheapstatistics(). https://codereview.chromium.org/1088683003/diff/80001/gin/v8_isolate_memory_d... File gin/v8_isolate_memory_dump_provider.h (right): https://codereview.chromium.org/1088683003/diff/80001/gin/v8_isolate_memory_d... gin/v8_isolate_memory_dump_provider.h:20: V8IsolateMemoryDumpProvider(IsolateHolder* isolate_holder); On 2015/04/21 16:43:42, Primiano Tucci wrote: > When you have a ctor with one argument, you want "explicit V8IsolateMDP(....)", > otherwise C++ will derive an implicit cast operator between IsolateHolder and > V8IsolateMDP, which you don't really want. Done. https://codereview.chromium.org/1088683003/diff/80001/gin/v8_isolate_memory_d... gin/v8_isolate_memory_dump_provider.h:28: IsolateHolder* isolate_holder_; // Not owned. On 2015/04/21 16:43:43, Primiano Tucci wrote: > maybe say: > > // Not owned (IsolateHolder owns this) Done. https://codereview.chromium.org/1088683003/diff/100001/gin/isolate_holder.cc File gin/isolate_holder.cc (right): https://codereview.chromium.org/1088683003/diff/100001/gin/isolate_holder.cc#... gin/isolate_holder.cc:47: isolate_memory_dump_provider_.reset(new V8IsolateMemoryDumpProvider(this)); On 2015/04/21 17:13:03, rmcilroy wrote: > nit - move to below isolate_data_.reset Done. https://codereview.chromium.org/1088683003/diff/100001/gin/v8_isolate_memory_... File gin/v8_isolate_memory_dump_provider.cc (right): https://codereview.chromium.org/1088683003/diff/100001/gin/v8_isolate_memory_... gin/v8_isolate_memory_dump_provider.cc:81: space_dump->SetAttribute( On 2015/04/21 17:13:39, Primiano Tucci wrote: > Had a chat with Ross, just delete this attribute as it is a bit misleading and > will very likely confuse people . Done.
primiano@chromium.org changed reviewers: + jochen@chromium.org
LGTM, +rmcilroy for a final look and +jochen for OWNERS stamp
https://codereview.chromium.org/1088683003/diff/160001/gin/v8_isolate_memory_... File gin/v8_isolate_memory_dump_provider.cc (right): https://codereview.chromium.org/1088683003/diff/160001/gin/v8_isolate_memory_... gin/v8_isolate_memory_dump_provider.cc:25: : MemoryDumpProvider(base::MessageLoop::current()->task_runner()) { I think you should be using base::ThreadTaskRunnerHandle::Get() instead of base::MessageLoop::current()->task_runner() (Sami has been trying to move things to this API to ensure that the correct scheduler driven task runner is used wherever possible rather than posting directly to the message loop). https://codereview.chromium.org/1088683003/diff/160001/gin/v8_isolate_memory_... gin/v8_isolate_memory_dump_provider.cc:29: isolate_holder_ = isolate_holder; nit - move this to the initializer list. https://codereview.chromium.org/1088683003/diff/160001/gin/v8_isolate_memory_... gin/v8_isolate_memory_dump_provider.cc:58: base::StringPrintf(kIsolateDumpName, isolate_holder_->isolate(), nit - I would prefer you just had the string literal here rather than using a constant so that it is obvious what is being formatted. https://codereview.chromium.org/1088683003/diff/160001/gin/v8_isolate_memory_... gin/v8_isolate_memory_dump_provider.cc:63: space_dump->set_allocated_objects_count(0); Add a todo to add support for objects_count? It should be possible in V8 to at least get a count of the number of live objects at the last GC if it is something you would want to track? https://codereview.chromium.org/1088683003/diff/160001/gin/v8_isolate_memory_... gin/v8_isolate_memory_dump_provider.cc:80: heap_statistics.used_heap_size() - known_spaces_used_size); Should you also add kAvailableSizeAttribute as zero currently with a todo to add support to the V8 API for Available in HeapStatistics? (it would be easy to add since there is already an Heap::Available() method in V8. https://codereview.chromium.org/1088683003/diff/160001/gin/v8_isolate_memory_... gin/v8_isolate_memory_dump_provider.cc:81: Primiano: I don't know how the hierarchy of allocators works for these dumpers, but as I remember the original plan had a top-level node with the total heap use/committed, then branched down to per-space sizes - is this something which is calculated offline now or should this dumper be explicitly adding the top-level node for heap_statistics.total_heap_size() and heap_statistics.used_heap_size() https://codereview.chromium.org/1088683003/diff/160001/gin/v8_isolate_memory_... File gin/v8_isolate_memory_dump_provider.h (right): https://codereview.chromium.org/1088683003/diff/160001/gin/v8_isolate_memory_... gin/v8_isolate_memory_dump_provider.h:16: // summarized memory stats about the V8 Isolates. nit /s/the V8 Isolates/a V8 isolate/ https://codereview.chromium.org/1088683003/diff/160001/gin/v8_isolate_memory_... gin/v8_isolate_memory_dump_provider.h:28: IsolateHolder* isolate_holder_; // Not owned (IsolateHolder owns this). IsolateHolder doesn't own itself. Just drop the "(IsolateHolder owns this)"
Made changes. PTAL. https://codereview.chromium.org/1088683003/diff/160001/gin/v8_isolate_memory_... File gin/v8_isolate_memory_dump_provider.cc (right): https://codereview.chromium.org/1088683003/diff/160001/gin/v8_isolate_memory_... gin/v8_isolate_memory_dump_provider.cc:25: : MemoryDumpProvider(base::MessageLoop::current()->task_runner()) { On 2015/04/21 21:57:26, rmcilroy wrote: > I think you should be using base::ThreadTaskRunnerHandle::Get() instead of > base::MessageLoop::current()->task_runner() (Sami has been trying to move things > to this API to ensure that the correct scheduler driven task runner is used > wherever possible rather than posting directly to the message loop). Thanks, changed. https://codereview.chromium.org/1088683003/diff/160001/gin/v8_isolate_memory_... gin/v8_isolate_memory_dump_provider.cc:29: isolate_holder_ = isolate_holder; On 2015/04/21 21:57:25, rmcilroy wrote: > nit - move this to the initializer list. Done. https://codereview.chromium.org/1088683003/diff/160001/gin/v8_isolate_memory_... gin/v8_isolate_memory_dump_provider.cc:58: base::StringPrintf(kIsolateDumpName, isolate_holder_->isolate(), On 2015/04/21 21:57:25, rmcilroy wrote: > nit - I would prefer you just had the string literal here rather than using a > constant so that it is obvious what is being formatted. This is kept as const at the top because each dump provider we have the names at the start of the file. (The root and the main child dumpers). This is general style I followed. Also it is used at 2 places. https://codereview.chromium.org/1088683003/diff/160001/gin/v8_isolate_memory_... gin/v8_isolate_memory_dump_provider.cc:63: space_dump->set_allocated_objects_count(0); On 2015/04/21 21:57:26, rmcilroy wrote: > Add a todo to add support for objects_count? It should be possible in V8 to at > least get a count of the number of live objects at the last GC if it is > something you would want to track? Done. https://codereview.chromium.org/1088683003/diff/160001/gin/v8_isolate_memory_... gin/v8_isolate_memory_dump_provider.cc:80: heap_statistics.used_heap_size() - known_spaces_used_size); On 2015/04/21 21:57:25, rmcilroy wrote: > Should you also add kAvailableSizeAttribute as zero currently with a todo to add > support to the V8 API for Available in HeapStatistics? (it would be easy to add > since there is already an Heap::Available() method in V8. Done. https://codereview.chromium.org/1088683003/diff/160001/gin/v8_isolate_memory_... File gin/v8_isolate_memory_dump_provider.h (right): https://codereview.chromium.org/1088683003/diff/160001/gin/v8_isolate_memory_... gin/v8_isolate_memory_dump_provider.h:16: // summarized memory stats about the V8 Isolates. On 2015/04/21 21:57:26, rmcilroy wrote: > nit /s/the V8 Isolates/a V8 isolate/ Isn't it like soon we are going to have multiple isolates per thread? https://codereview.chromium.org/1088683003/diff/160001/gin/v8_isolate_memory_... gin/v8_isolate_memory_dump_provider.h:28: IsolateHolder* isolate_holder_; // Not owned (IsolateHolder owns this). On 2015/04/21 21:57:26, rmcilroy wrote: > IsolateHolder doesn't own itself. Just drop the "(IsolateHolder owns this)" Done.
lgtm
lgtm, thanks. https://codereview.chromium.org/1088683003/diff/160001/gin/v8_isolate_memory_... File gin/v8_isolate_memory_dump_provider.h (right): https://codereview.chromium.org/1088683003/diff/160001/gin/v8_isolate_memory_... gin/v8_isolate_memory_dump_provider.h:16: // summarized memory stats about the V8 Isolates. On 2015/04/22 10:41:35, ssid wrote: > On 2015/04/21 21:57:26, rmcilroy wrote: > > nit /s/the V8 Isolates/a V8 isolate/ > > Isn't it like soon we are going to have multiple isolates per thread? No, there is only ever a single isolate per thread generally. In any case, it doesn't matter - each V8IsolateMemoryDumpProvider only dumps memory for a single isolate - even if there are multiple isolates per thread each would have a separate V8IsolateMemoryDumpProvider.
On 2015/04/22 at 15:41:06, rmcilroy wrote: > lgtm, thanks. > > https://codereview.chromium.org/1088683003/diff/160001/gin/v8_isolate_memory_... > File gin/v8_isolate_memory_dump_provider.h (right): > > https://codereview.chromium.org/1088683003/diff/160001/gin/v8_isolate_memory_... > gin/v8_isolate_memory_dump_provider.h:16: // summarized memory stats about the V8 Isolates. > On 2015/04/22 10:41:35, ssid wrote: > > On 2015/04/21 21:57:26, rmcilroy wrote: > > > nit /s/the V8 Isolates/a V8 isolate/ > > > > Isn't it like soon we are going to have multiple isolates per thread? > > No, there is only ever a single isolate per thread generally. > > In any case, it doesn't matter - each V8IsolateMemoryDumpProvider only dumps memory for a single isolate - even if there are multiple isolates per thread each would have a separate V8IsolateMemoryDumpProvider. Note however that a single isolate can jump around between threads (in the proxy resolver)
On 2015/04/22 15:50:45, jochen wrote: > On 2015/04/22 at 15:41:06, rmcilroy wrote: > > lgtm, thanks. > > > > > https://codereview.chromium.org/1088683003/diff/160001/gin/v8_isolate_memory_... > > File gin/v8_isolate_memory_dump_provider.h (right): > > > > > https://codereview.chromium.org/1088683003/diff/160001/gin/v8_isolate_memory_... > > gin/v8_isolate_memory_dump_provider.h:16: // summarized memory stats about the > V8 Isolates. > > On 2015/04/22 10:41:35, ssid wrote: > > > On 2015/04/21 21:57:26, rmcilroy wrote: > > > > nit /s/the V8 Isolates/a V8 isolate/ > > > > > > Isn't it like soon we are going to have multiple isolates per thread? > > > > No, there is only ever a single isolate per thread generally. > > > > In any case, it doesn't matter - each V8IsolateMemoryDumpProvider only dumps > memory for a single isolate - even if there are multiple isolates per thread > each would have a separate V8IsolateMemoryDumpProvider. > > Note however that a single isolate can jump around between threads (in the proxy > resolver) Do I need to worry about the proxy resolver? This code assumes that when IsolateHolder is created in a thread, then DumpInto() will be called in the same thread when needed. When isolate jumps between threads the IsolateHolder gets destroyed and recreated in another thread, or the same IsolateHolder is passed between threads? If this is passed then we would have to change the thread in which the DumpInto is called to access isolate, when it is passed.
On 2015/04/22 16:00:34, ssid wrote: > On 2015/04/22 15:50:45, jochen wrote: > > On 2015/04/22 at 15:41:06, rmcilroy wrote: > > > lgtm, thanks. > > > > > > > > > https://codereview.chromium.org/1088683003/diff/160001/gin/v8_isolate_memory_... > > > File gin/v8_isolate_memory_dump_provider.h (right): > > > > > > > > > https://codereview.chromium.org/1088683003/diff/160001/gin/v8_isolate_memory_... > > > gin/v8_isolate_memory_dump_provider.h:16: // summarized memory stats about > the > > V8 Isolates. > > > On 2015/04/22 10:41:35, ssid wrote: > > > > On 2015/04/21 21:57:26, rmcilroy wrote: > > > > > nit /s/the V8 Isolates/a V8 isolate/ > > > > > > > > Isn't it like soon we are going to have multiple isolates per thread? > > > > > > No, there is only ever a single isolate per thread generally. > > > > > > In any case, it doesn't matter - each V8IsolateMemoryDumpProvider only dumps > > memory for a single isolate - even if there are multiple isolates per thread > > each would have a separate V8IsolateMemoryDumpProvider. > > > > Note however that a single isolate can jump around between threads (in the > proxy > > resolver) > > Do I need to worry about the proxy resolver? > This code assumes that when IsolateHolder is created in a thread, then > DumpInto() will be called in the same thread when needed. > When isolate jumps between threads the IsolateHolder gets destroyed and > recreated in another thread, or the same IsolateHolder is passed between > threads? > If this is passed then we would have to change the thread in which the DumpInto > is called to access isolate, when it is passed. I had a chat offline with Jochen. The patch we should follow seems to be: - Add a bool to the isolate holder ctor to tell whether the client of the IsolateHolder will use v8::Locker to access the isolate or not. - ProxyResolver seems to be the only case that uses lockers. - In the dumper side: - If the isolate_holder->use_locks() -> we have to take a lock before accessing the heap stats - Keep doing what we do if !use_locks() Jochen, does the plan seem right? If so, my suggestion is to keep this patch frozen, do the +bool refactor in another CL, and then come back and rebase this patch
Patchset #13 (id:240001) has been deleted
Patchset #12 (id:220001) has been deleted
Patchset #11 (id:200001) has been deleted
Patchset #11 (id:260001) has been deleted
On 2015/04/22 17:31:00, Primiano Tucci wrote: > On 2015/04/22 16:00:34, ssid wrote: > > On 2015/04/22 15:50:45, jochen wrote: > > > On 2015/04/22 at 15:41:06, rmcilroy wrote: > > > > lgtm, thanks. > > > > > > > > > > > > > > https://codereview.chromium.org/1088683003/diff/160001/gin/v8_isolate_memory_... > > > > File gin/v8_isolate_memory_dump_provider.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1088683003/diff/160001/gin/v8_isolate_memory_... > > > > gin/v8_isolate_memory_dump_provider.h:16: // summarized memory stats about > > the > > > V8 Isolates. > > > > On 2015/04/22 10:41:35, ssid wrote: > > > > > On 2015/04/21 21:57:26, rmcilroy wrote: > > > > > > nit /s/the V8 Isolates/a V8 isolate/ > > > > > > > > > > Isn't it like soon we are going to have multiple isolates per thread? > > > > > > > > No, there is only ever a single isolate per thread generally. > > > > > > > > In any case, it doesn't matter - each V8IsolateMemoryDumpProvider only > dumps > > > memory for a single isolate - even if there are multiple isolates per thread > > > each would have a separate V8IsolateMemoryDumpProvider. > > > > > > Note however that a single isolate can jump around between threads (in the > > proxy > > > resolver) > > > > Do I need to worry about the proxy resolver? > > This code assumes that when IsolateHolder is created in a thread, then > > DumpInto() will be called in the same thread when needed. > > When isolate jumps between threads the IsolateHolder gets destroyed and > > recreated in another thread, or the same IsolateHolder is passed between > > threads? > > If this is passed then we would have to change the thread in which the > DumpInto > > is called to access isolate, when it is passed. > > I had a chat offline with Jochen. > The patch we should follow seems to be: > - Add a bool to the isolate holder ctor to tell whether the client of the > IsolateHolder will use v8::Locker to access the isolate or not. > - ProxyResolver seems to be the only case that uses lockers. > - In the dumper side: > - If the isolate_holder->use_locks() -> we have to take a lock before > accessing the heap stats > - Keep doing what we do if !use_locks() > > Jochen, does the plan seem right? > If so, my suggestion is to keep this patch frozen, do the +bool refactor in > another CL, and then come back and rebase this patch Rebase for changes. PTAL.
Cool, lgtm
Just some final nits, thanks https://codereview.chromium.org/1088683003/diff/280001/gin/v8_isolate_memory_... File gin/v8_isolate_memory_dump_provider.cc (right): https://codereview.chromium.org/1088683003/diff/280001/gin/v8_isolate_memory_... gin/v8_isolate_memory_dump_provider.cc:77: space_dump->set_physical_size_in_bytes(static_cast<int>((space_size))); set_physical_size_in_bytes takes a uint64, this should be casted to uint64 https://codereview.chromium.org/1088683003/diff/280001/gin/v8_isolate_memory_... gin/v8_isolate_memory_dump_provider.cc:79: // after the last GC. Hmm not really sure I understand what is the message inside the TODO. Does it suggest that you will possibly add more extra attribute in future? if so file a bug, link it to the existing one and add a reference to it here. https://codereview.chromium.org/1088683003/diff/280001/gin/v8_isolate_memory_... gin/v8_isolate_memory_dump_provider.cc:82: static_cast<int>(space_used_size)); ditto here for int <> uint64 https://codereview.chromium.org/1088683003/diff/280001/gin/v8_isolate_memory_... gin/v8_isolate_memory_dump_provider.cc:97: // TODO(ssid): Extend GetHeapStatistics api to return available size. Add a blank line before the TODO
Patchset #12 (id:300001) has been deleted
Fixed comments, PTAL. https://codereview.chromium.org/1088683003/diff/280001/gin/v8_isolate_memory_... File gin/v8_isolate_memory_dump_provider.cc (right): https://codereview.chromium.org/1088683003/diff/280001/gin/v8_isolate_memory_... gin/v8_isolate_memory_dump_provider.cc:77: space_dump->set_physical_size_in_bytes(static_cast<int>((space_size))); On 2015/04/27 08:41:03, Primiano Tucci wrote: > set_physical_size_in_bytes takes a uint64, this should be casted to uint64 Done. https://codereview.chromium.org/1088683003/diff/280001/gin/v8_isolate_memory_... gin/v8_isolate_memory_dump_provider.cc:79: // after the last GC. On 2015/04/27 08:41:03, Primiano Tucci wrote: > Hmm not really sure I understand what is the message inside the TODO. > Does it suggest that you will possibly add more extra attribute in future? > if so file a bug, link it to the existing one and add a reference to it here. Done.
LGTM thanks. P.S. DId you fix the reporting issue where the other heaps was under-flowing in the negative range (and reporting TB) ?
On 2015/04/27 12:34:03, Primiano Tucci wrote: > LGTM thanks. > P.S. DId you fix the reporting issue where the other heaps was under-flowing in > the negative range (and reporting TB) ? Yes, fixed https://codereview.chromium.org/1095393005/.
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rmcilroy@chromium.org, jochen@chromium.org Link to the patchset: https://codereview.chromium.org/1088683003/#ps320001 (title: "Fixing TODO.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1088683003/320001
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, jochen@chromium.org, rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/1088683003/#ps340001 (title: "Fixing build.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1088683003/340001
The CQ bit was unchecked by ssid@chromium.org
Rebased for interface changes. PTAL.
skyostil@chromium.org changed reviewers: + skyostil@chromium.org
Thanks, lgtm with a small tweak. https://codereview.chromium.org/1088683003/diff/360001/content/test/test_blin... File content/test/test_blink_web_unit_test_support.cc (right): https://codereview.chromium.org/1088683003/diff/360001/content/test/test_blin... content/test/test_blink_web_unit_test_support.cc:60: if (base::MessageLoopProxy::current()) { Please remove this guard since it's now unnecessary.
https://codereview.chromium.org/1088683003/diff/380001/content/test/test_blin... File content/test/test_blink_web_unit_test_support.cc (right): https://codereview.chromium.org/1088683003/diff/380001/content/test/test_blin... content/test/test_blink_web_unit_test_support.cc:48: base::MessageLoop message_loop; Hmm this will destroy the messageloop after the ctor. It will probably work for your purpose (you'll still get a taskrunner handle) but if somebody in future will try to post tasks to the ML they will get dropped on the floor because the ML has ended. You want this to be a field of the class.
https://codereview.chromium.org/1088683003/diff/380001/content/test/test_blin... File content/test/test_blink_web_unit_test_support.cc (right): https://codereview.chromium.org/1088683003/diff/380001/content/test/test_blin... content/test/test_blink_web_unit_test_support.cc:48: base::MessageLoop message_loop; On 2015/04/29 17:45:11, Primiano Tucci wrote: > Hmm this will destroy the messageloop after the ctor. > It will probably work for your purpose (you'll still get a taskrunner handle) > but if somebody in future will try to post tasks to the ML they will get dropped > on the floor because the ML has ended. > You want this to be a field of the class. Ah, sorry for not picking up on this. In my head we were entering a nested message loop at the bottom of this function, which isn't the case.
Thanks for the hard work. LGTM with 3 final micro-nits. Wait for the isolate holder CL to stage (like the good whisky) on the FYI bots and than ship this :) https://codereview.chromium.org/1088683003/diff/420001/gin/v8_isolate_memory_... File gin/v8_isolate_memory_dump_provider.cc (right): https://codereview.chromium.org/1088683003/diff/420001/gin/v8_isolate_memory_... gin/v8_isolate_memory_dump_provider.cc:60: size_t space_size = space_statistics.space_size(); here and below: s/size_t/const size_t/ https://codereview.chromium.org/1088683003/diff/420001/gin/v8_isolate_memory_... gin/v8_isolate_memory_dump_provider.cc:70: pmd->CreateAllocatorDump(allocator_name.c_str()); you don't need c_str() here, CreateAllocatorDump takes a std::string https://codereview.chromium.org/1088683003/diff/420001/gin/v8_isolate_memory_... File gin/v8_isolate_memory_dump_provider.h (right): https://codereview.chromium.org/1088683003/diff/420001/gin/v8_isolate_memory_... gin/v8_isolate_memory_dump_provider.h:29: void DumpMemoryStatistics_(base::trace_event::ProcessMemoryDump* pmd); You don't need the trailing underscore for private methods.
Done. https://codereview.chromium.org/1088683003/diff/360001/content/test/test_blin... File content/test/test_blink_web_unit_test_support.cc (right): https://codereview.chromium.org/1088683003/diff/360001/content/test/test_blin... content/test/test_blink_web_unit_test_support.cc:60: if (base::MessageLoopProxy::current()) { On 2015/04/29 16:15:38, Sami wrote: > Please remove this guard since it's now unnecessary. Done. https://codereview.chromium.org/1088683003/diff/420001/gin/v8_isolate_memory_... File gin/v8_isolate_memory_dump_provider.cc (right): https://codereview.chromium.org/1088683003/diff/420001/gin/v8_isolate_memory_... gin/v8_isolate_memory_dump_provider.cc:60: size_t space_size = space_statistics.space_size(); On 2015/05/07 16:25:43, Primiano Tucci wrote: > here and below: > s/size_t/const size_t/ Done. https://codereview.chromium.org/1088683003/diff/420001/gin/v8_isolate_memory_... gin/v8_isolate_memory_dump_provider.cc:70: pmd->CreateAllocatorDump(allocator_name.c_str()); On 2015/05/07 16:25:43, Primiano Tucci wrote: > you don't need c_str() here, CreateAllocatorDump takes a std::string Done. https://codereview.chromium.org/1088683003/diff/420001/gin/v8_isolate_memory_... File gin/v8_isolate_memory_dump_provider.h (right): https://codereview.chromium.org/1088683003/diff/420001/gin/v8_isolate_memory_... gin/v8_isolate_memory_dump_provider.h:29: void DumpMemoryStatistics_(base::trace_event::ProcessMemoryDump* pmd); On 2015/05/07 16:25:43, Primiano Tucci wrote: > You don't need the trailing underscore for private methods. Done.
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, rmcilroy@chromium.org, skyostil@chromium.org, primiano@chromium.org Link to the patchset: https://codereview.chromium.org/1088683003/#ps440001 (title: "Fixing nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1088683003/440001
The CQ bit was unchecked by primiano@chromium.org
Very sorry for unticking at the last moment. I just realized we might need an extra 4 char patchset to be future-proof (See my comment below) https://codereview.chromium.org/1088683003/diff/440001/gin/v8_isolate_memory_... File gin/v8_isolate_memory_dump_provider.cc (right): https://codereview.chromium.org/1088683003/diff/440001/gin/v8_isolate_memory_... gin/v8_isolate_memory_dump_provider.cc:67: "%s/%s_%p/%s", kRootDumpName, kIsolateDumpName, I just realized that, in light of our offline conversations, this should be "%s/%s_%p/heap/%s", i.e. "v8/isolate_0x1234/heaps/xxx_space"? So that this is going to be future-proof with the extended objects stats you are going to introduce.
On 2015/05/08 08:50:35, Primiano Tucci wrote: > Very sorry for unticking at the last moment. > I just realized we might need an extra 4 char patchset to be future-proof (See > my comment below) > > https://codereview.chromium.org/1088683003/diff/440001/gin/v8_isolate_memory_... > File gin/v8_isolate_memory_dump_provider.cc (right): > > https://codereview.chromium.org/1088683003/diff/440001/gin/v8_isolate_memory_... > gin/v8_isolate_memory_dump_provider.cc:67: "%s/%s_%p/%s", kRootDumpName, > kIsolateDumpName, > I just realized that, in light of our offline conversations, this should be > "%s/%s_%p/heap/%s", i.e. "v8/isolate_0x1234/heaps/xxx_space"? > So that this is going to be future-proof with the extended objects stats you are > going to introduce. I was just making another patch for that change. Will do it here then.
Done. https://codereview.chromium.org/1088683003/diff/440001/gin/v8_isolate_memory_... File gin/v8_isolate_memory_dump_provider.cc (right): https://codereview.chromium.org/1088683003/diff/440001/gin/v8_isolate_memory_... gin/v8_isolate_memory_dump_provider.cc:67: "%s/%s_%p/%s", kRootDumpName, kIsolateDumpName, On 2015/05/08 08:50:34, Primiano Tucci wrote: > I just realized that, in light of our offline conversations, this should be > "%s/%s_%p/heap/%s", i.e. "v8/isolate_0x1234/heaps/xxx_space"? > So that this is going to be future-proof with the extended objects stats you are > going to introduce. Done.
still LGTM, thanks
The CQ bit was checked by primiano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, rmcilroy@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/1088683003/#ps460001 (title: "Adding heap_spaces child.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1088683003/460001
Message was sent while issue was closed.
Committed patchset #19 (id:460001)
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/83aa5be892018f800328c1b7d03f6fc37bc22d5b Cr-Commit-Position: refs/heads/master@{#328946} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
