Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1149673002:
Adding blink gc memory dump infrastructure for thread specific dumps. (Closed)
DescriptionAdding blink gc memory dump infrastructure for thread specific dumps.
This CL adds infrastructure to dump memory stats from blink GC. A
snapshot is stored after each GC and is merged into the given memory
dump at the time of dump request (from MemoryDumpManager).
BUG=490087
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197635
Patch Set 1 #Patch Set 2 : Fixing dump name. #
Total comments: 22
Patch Set 3 : Addressing comments. #Patch Set 4 : Few nits. #
Total comments: 8
Patch Set 5 : Rebasing for primiano's future changes. #
Total comments: 22
Patch Set 6 : Moving the dump provider impl. #Patch Set 7 : Fixing nits. #
Total comments: 20
Patch Set 8 : Moving the last dump to dump provider. #Patch Set 9 : Fixing nits. #
Total comments: 18
Patch Set 10 : Fixing nits. #
Total comments: 13
Patch Set 11 : Making changes for forced GC. #
Total comments: 10
Patch Set 12 : Adding suggested comments. #Patch Set 13 : Fixing tests (moving dump->clear to collectGarbage). #Patch Set 14 : Rebase. #Patch Set 15 : Adding a check for number of heaps reported. #Patch Set 16 : Test fix. #Patch Set 17 : Build fix. #Patch Set 18 : Fixing android. #Patch Set 19 : Rebase. #Patch Set 20 : Rebase and replacing main thread with thread id. #
Messages
Total messages: 47 (16 generated)
ssid@chromium.org changed reviewers: + primiano@chromium.org
PTAL.
OK this seems going in the right direction. Adding some comments here (some of them are more questions you'll need to forward to the oilpan team). At this point let's do this: - let me finish the TakeDumpsFrom() API so this CL can become real :) - while I do this you and petr can try to figure out my questions below looking at the oilpan code, and if unclear just ask somebody from the oilpan team. - Once we get the TakeDumpFrom API and we adjust the CL we add haraken@ for review here. +petrcermak, can you give a second look to see if everything makes sense to you? https://codereview.chromium.org/1149673002/diff/20001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1149673002/diff/20001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:218: void BaseHeap::snapshotMemory(WebProcessMemoryDump* memoryDump, const char* heapName) I wonder if we should start calling our functions with dump* instead of snapshot*, so it will be clear once we have finished which stuff is ours and which is the old oilpan heap visualizer. i.e. this could be called dumpMemoryInto https://codereview.chromium.org/1149673002/diff/20001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:220: String allocatorName = String::format("blink_gc/thread_%p/heaps/%s", threadState(), heapName); for the thread name, it would be great if: 1) for the we could have "thread_main" instead of thread_randomnumber if we know that this is the main thread (see threadState()->isMainThread()) 2) We could dump the thread id instead of the object PTR. I think you could use currentThread() (under the assumption that we are in the right thread). 2 reminds me: can you add a DCHECK(threadState().checkThread()) to check that we are always dumping on the right thread? https://codereview.chromium.org/1149673002/diff/20001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:220: String allocatorName = String::format("blink_gc/thread_%p/heaps/%s", threadState(), heapName); s/allocatorName/dumpName/ https://codereview.chromium.org/1149673002/diff/20001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:225: allocatorDump->AddScalar("page_count", "objects", pageCount); we have to find a slightly different name here (idea?). The TL;DR is that these are not "pages" as in system 4k pages, but pretty much random size "blinkgc" pages If I am a random engineer and see this column in chrome://tracing I would think "Ok, this is just using page_count * 4k bytes" which is definitely not true. +petrcermak can help you find a right name here :) https://codereview.chromium.org/1149673002/diff/20001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:226: } some questions here (I guess will be mostly for questions for haraken and team): - should we skip the count / do something special if page->isEmpty()? - Should we also add-up the page->size() into a counter (very likely outer_size for the heap?) - how do these oilpan "pages" map to "system pages?" are they always multiples of 4k? - how is the memory of these pages allocated? Does it always go via mmap (in which case we are good)? Can be ever be allocated via malloc / partAlloc (in which case we should add a TODO and keep track of the cross link through the upcoming graph model)? https://codereview.chromium.org/1149673002/diff/20001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1149673002/diff/20001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:142: checkThread(); keep checkThread on the top, if this ends up in the wrong thread, we want to know ASAP https://codereview.chromium.org/1149673002/diff/20001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:391: void ThreadState::snapshotMemoryIfNecessary() This is a good name. I wonder though if we really need the other ::snapshotMemory of if we can just put everything here. https://codereview.chromium.org/1149673002/diff/20001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:394: TRACE_EVENT_CATEGORY_GROUP_ENABLED(TRACE_DISABLED_BY_DEFAULT("memory-infra"), &enabled); I see why you did it (it's the only way to tell today whether memory infra is enabled) but I think I prefer exposiing a method here and not having to knowing the "memory-infra" string inside of blink. I will think (and expose an API) and let you know https://codereview.chromium.org/1149673002/diff/20001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:405: // m_lastProcessMemoryDump.reset(Platform::current()->getExtraProcessMemoryDump()); I think we need to do some kind of dual buffering here: keep always two pmd: one is the last good one, and another is the one you are currently populating. So that when we get the onMemoryDump call we don't have to worry if the pmd is being populated, and we can rely on the fact that we always have a "last good one" (unless we get the onMemoryDUmp too early and we don't have one at all) https://codereview.chromium.org/1149673002/diff/20001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:424: #undef SNAPSHOT_FREE_LIST I think you meant to undef SNAPSHOT_HEAP here :) https://codereview.chromium.org/1149673002/diff/20001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:1282: // memoryDump->swap(m_lastProcessMemoryDump.get()); The code is not fully there yet, but the final name will be "TakeAllDumpsFrom" not swap See crrev.com/1151603004 Also, TakeAllDumpsFrom will gurarrantee that the argument will be emptied and re-usable after the call (so you will not have to create a new one)
Made changes necessary and added comments. https://codereview.chromium.org/1149673002/diff/20001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1149673002/diff/20001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:218: void BaseHeap::snapshotMemory(WebProcessMemoryDump* memoryDump, const char* heapName) On 2015/05/21 09:56:33, Primiano Tucci wrote: > I wonder if we should start calling our functions with dump* instead of > snapshot*, so it will be clear once we have finished which stuff is ours and > which is the old oilpan heap visualizer. > i.e. this could be called dumpMemoryInto Done. https://codereview.chromium.org/1149673002/diff/20001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:220: String allocatorName = String::format("blink_gc/thread_%p/heaps/%s", threadState(), heapName); On 2015/05/21 09:56:33, Primiano Tucci wrote: > s/allocatorName/dumpName/ Done. https://codereview.chromium.org/1149673002/diff/20001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:220: String allocatorName = String::format("blink_gc/thread_%p/heaps/%s", threadState(), heapName); On 2015/05/21 09:56:33, Primiano Tucci wrote: > for the thread name, it would be great if: > 1) for the we could have "thread_main" instead of thread_randomnumber if we know > that this is the main thread (see threadState()->isMainThread()) done. > 2) We could dump the thread id instead of the object PTR. I think you could use > currentThread() (under the assumption that we are in the right thread). > done. > 2 reminds me: can you add a DCHECK(threadState().checkThread()) to check that we > are always dumping on the right thread? Added dcheck to the function in ThreadState. https://codereview.chromium.org/1149673002/diff/20001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:226: } On 2015/05/21 09:56:33, Primiano Tucci wrote: > some questions here (I guess will be mostly for questions for haraken and team): > - should we skip the count / do something special if page->isEmpty()? Empty pages are still taking space in the heap. The next CL plans to give count of empty, live and dead pages and their sizes. > - Should we also add-up the page->size() into a counter (very likely outer_size > for the heap?) These numbers are not easily available and these are planned for next CL. This CL olny adds page count and sets up infrastructure. If curioud please check how to get these numbers in the next CL: crrev.com/1149703002 > - how do these oilpan "pages" map to "system pages?" are they always multiples > of 4k? > - how is the memory of these pages allocated? Does it always go via mmap (in > which case we are good)? Can be ever be allocated via malloc / partAlloc (in > which case we should add a TODO and keep track of the cross link through the > upcoming graph model)? As far as i can see, the pages are allocated by mmap. It uses WTF::PageAllocator, not PartitionAlloc. +haraken for answer. https://codereview.chromium.org/1149673002/diff/20001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1149673002/diff/20001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:142: checkThread(); On 2015/05/21 09:56:34, Primiano Tucci wrote: > keep checkThread on the top, if this ends up in the wrong thread, we want to > know ASAP Done. https://codereview.chromium.org/1149673002/diff/20001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:391: void ThreadState::snapshotMemoryIfNecessary() On 2015/05/21 09:56:34, Primiano Tucci wrote: > This is a good name. > I wonder though if we really need the other ::snapshotMemory of if we can just > put everything here. Not sure, your call. https://codereview.chromium.org/1149673002/diff/20001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:394: TRACE_EVENT_CATEGORY_GROUP_ENABLED(TRACE_DISABLED_BY_DEFAULT("memory-infra"), &enabled); On 2015/05/21 09:56:33, Primiano Tucci wrote: > I see why you did it (it's the only way to tell today whether memory infra is > enabled) but I think I prefer exposiing a method here and not having to knowing > the "memory-infra" string inside of blink. > I will think (and expose an API) and let you know Not sure if i should add a TODO here? https://codereview.chromium.org/1149673002/diff/20001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:405: // m_lastProcessMemoryDump.reset(Platform::current()->getExtraProcessMemoryDump()); On 2015/05/21 09:56:34, Primiano Tucci wrote: > I think we need to do some kind of dual buffering here: keep always two pmd: > one is the last good one, and another is the one you are currently populating. > So that when we get the onMemoryDump call we don't have to worry if the pmd is > being populated, and we can rely on the fact that we always have a "last good > one" (unless we get the onMemoryDUmp too early and we don't have one at all) Yes this check is for the case that it isn't available at all. I don't think we need a last good one, since only one of them can happen at a time, either a snapshot or a memodyDump, in the same thread. Am I missing something? https://codereview.chromium.org/1149673002/diff/20001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:424: #undef SNAPSHOT_FREE_LIST On 2015/05/21 09:56:33, Primiano Tucci wrote: > I think you meant to undef SNAPSHOT_HEAP here :) Done. https://codereview.chromium.org/1149673002/diff/20001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:1282: // memoryDump->swap(m_lastProcessMemoryDump.get()); On 2015/05/21 09:56:33, Primiano Tucci wrote: > The code is not fully there yet, but the final name will be "TakeAllDumpsFrom" > not swap > See crrev.com/1151603004 > > Also, TakeAllDumpsFrom will gurarrantee that the argument will be emptied and > re-usable after the call (so you will not have to create a new one) Done.
petrcermak@chromium.org changed reviewers: + petrcermak@chromium.org
Just 3 small comments/suggestions. Thanks, Petr https://codereview.chromium.org/1149673002/diff/60001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1149673002/diff/60001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:229: for (BasePage* page = m_firstPage; page; page = page->next(), ++pageCount) { } I would personally put the increment in the body of the loop: for (BasePage* page = m_firstPage; page; page = page->next()) { ++pageCount; } I think it makes the code more readable. https://codereview.chromium.org/1149673002/diff/60001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:230: allocatorDump->AddScalar("blink_page_count", "objects", pageCount); The only alternative I can think of is "base_page_count" (because it's the BasePage class), but I think "blink_page_count" sounds fine (long name is better than ambiguous name). https://codereview.chromium.org/1149673002/diff/60001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/1149673002/diff/60001/Source/platform/heap/Th... Source/platform/heap/ThreadState.h:461: void dumpMemoryIfNecessary(); Maybe add one or two one-line comments explaining what the new methods do?
picksi@google.com changed reviewers: + picksi@google.com
Looks good. I agree with Petr's moving the pageCount++ into the loop. https://codereview.chromium.org/1149673002/diff/60001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1149673002/diff/60001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:229: for (BasePage* page = m_firstPage; page; page = page->next(), ++pageCount) { } +1
Addressed all comments except for adding two dumps, one for updating and one for swapping. This can happen in multiple threads. https://codereview.chromium.org/1149673002/diff/20001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1149673002/diff/20001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:225: allocatorDump->AddScalar("page_count", "objects", pageCount); On 2015/05/21 09:56:33, Primiano Tucci wrote: > we have to find a slightly different name here (idea?). > The TL;DR is that these are not "pages" as in system 4k pages, but pretty much > random size "blinkgc" pages > > If I am a random engineer and see this column in chrome://tracing I would think > "Ok, this is just using page_count * 4k bytes" which is definitely not true. > > +petrcermak can help you find a right name here :) Done. https://codereview.chromium.org/1149673002/diff/60001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1149673002/diff/60001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:229: for (BasePage* page = m_firstPage; page; page = page->next(), ++pageCount) { } On 2015/05/21 15:33:49, picksi wrote: > +1 Done. https://codereview.chromium.org/1149673002/diff/60001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:230: allocatorDump->AddScalar("blink_page_count", "objects", pageCount); On 2015/05/21 13:15:57, petrcermak wrote: > The only alternative I can think of is "base_page_count" (because it's the > BasePage class), but I think "blink_page_count" sounds fine (long name is better > than ambiguous name). Acknowledged. https://codereview.chromium.org/1149673002/diff/60001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1149673002/diff/60001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:404: ASSERT(checkThread()); This check is not required here, since method can be called on any thread. In this case we need to add two dumps, one which is being updated and one which is being swapped. Do you think there should be a lock? 2 threads can do the swapping at the same time. https://codereview.chromium.org/1149673002/diff/60001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/1149673002/diff/60001/Source/platform/heap/Th... Source/platform/heap/ThreadState.h:461: void dumpMemoryIfNecessary(); On 2015/05/21 13:15:57, petrcermak wrote: > Maybe add one or two one-line comments explaining what the new methods do? Done.
Some comments after our long offline discussion. MultiThreading is tough but, if our understandings are confirmed from oilpan experts, we should be able to easily get what we need. https://codereview.chromium.org/1149673002/diff/80001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1149673002/diff/80001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:218: void BaseHeap::dumpMemoryInto(WebProcessMemoryDump* memoryDump, const String* allocatorBaseName) maybe s/allocatorBaseName/dumpBaseName/... or just dumpName, unless you plan to extend it in the future (I guess that is the case) ah and const* -> const& https://codereview.chromium.org/1149673002/diff/80001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:222: return; Hmm I'd much rather prefer that you dump a zero in this case (or you might cause headaches to Peter's UI) Which looks like, just remove these two lines and the for () loop will take care as it is https://codereview.chromium.org/1149673002/diff/80001/Source/platform/heap/He... File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/1149673002/diff/80001/Source/platform/heap/He... Source/platform/heap/Heap.h:683: void dumpMemoryInto(WebProcessMemoryDump*, const String* allocatorBaseName); nit: const String& ? https://codereview.chromium.org/1149673002/diff/80001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1149673002/diff/80001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:395: TRACE_EVENT_CATEGORY_GROUP_ENABLED(TRACE_DISABLED_BY_DEFAULT("memory-infra"), &enabled); Ok to this as a temporary hack, but plz add a cleanup TODO here saying that this should be exposed by meminfra soon. https://codereview.chromium.org/1149673002/diff/80001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:413: SNAPSHOT_HEAP(NormalPage1); You either want to remove the ; here (otherwise some compilers will complain about empty statement), or turn the #defines into #define SNAPSHOT_HEAP(HeapType) \ do { ... stuff ... } while(0) So the expansion ends up with a ; after the while(0). Not sure what is the most common patter, go figure out in cs :) https://codereview.chromium.org/1149673002/diff/80001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:429: // (crbug.com/490087). Maybe add a line here saying: For the moment the dump is periodically invoked by the MemoryDumpManager every X secs. https://codereview.chromium.org/1149673002/diff/80001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:1282: return true; Ok for now, but this should really return false. If we return true we will confuse the UI. it is ok for the moment, as this is unlikely but should be fixed soon. Please add a TODO and reference crbug.com/490783 (I've just created) and reword the comment as. // TODO(ssid): Temporary workaround while crbug.com/490783 gets fixed. This should return false in the unlikely case that we didn't see a GC yet, which in turn will cause the dump to be invalidated. https://codereview.chromium.org/1149673002/diff/80001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/1149673002/diff/80001/Source/platform/heap/Th... Source/platform/heap/ThreadState.h:168: class PLATFORM_EXPORT ThreadState : public WebMemoryDumpProvider { We discussed this offline (there is a thread in the internal oilpan ML pending). There seems to be one ThreadState per therad, but PostGC seems to be called once on a single (probably main) thread, and scans all the threads. As a consequence, having this as the dumper is odd because: - Will give the false expectation that the onMemoryDump is called on the same Thread of this ThreadState, while that will not be the case. - Will end up register one dumper per thread, and it seems that we don't really need it. We should have an external dumper class (just one). All the thread states should dumpInto a shared WebProcessMemoryDump (obtained through the new API that is landing), and later on the dumper should just merge (takeAllDumps()) that dump into the WPMD passed to onMemoryDump. If my understandings of the oilpan threading model are correct, we can do this by using just one extra WPMD, no locks, and no double-buffering. https://codereview.chromium.org/1149673002/diff/80001/Source/platform/heap/Th... Source/platform/heap/ThreadState.h:463: void dumpMemoryIfNecessary(); Had a second thought about this, please don't hate me, I'm very terrible in naming decisions. How about dumpIfMemoryProfilingIsEnabled() ? "necessary" is a bit generic :) If you are not convinced, don't worry, probably in the next patchset I will propose another different name :P https://codereview.chromium.org/1149673002/diff/80001/Source/platform/heap/Th... Source/platform/heap/ThreadState.h:618: bool onMemoryDump(WebProcessMemoryDump*) override; This should go away from here, as per discussion up. https://codereview.chromium.org/1149673002/diff/80001/Source/platform/heap/Th... Source/platform/heap/ThreadState.h:717: OwnPtr<WebProcessMemoryDump> m_lastProcessMemoryDump; And this should be just one, shared by all the ThreadState(s).
Looks like a different CL now :) PTAL. https://codereview.chromium.org/1149673002/diff/80001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1149673002/diff/80001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:218: void BaseHeap::dumpMemoryInto(WebProcessMemoryDump* memoryDump, const String* allocatorBaseName) On 2015/05/21 18:43:10, Primiano Tucci wrote: > maybe s/allocatorBaseName/dumpBaseName/... or just dumpName, unless you plan to > extend it in the future (I guess that is the case) > ah and const* -> const& Done. https://codereview.chromium.org/1149673002/diff/80001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:222: return; On 2015/05/21 18:43:10, Primiano Tucci wrote: > Hmm I'd much rather prefer that you dump a zero in this case (or you might cause > headaches to Peter's UI) > Which looks like, just remove these two lines and the for () loop will take care > as it is Done. https://codereview.chromium.org/1149673002/diff/80001/Source/platform/heap/He... File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/1149673002/diff/80001/Source/platform/heap/He... Source/platform/heap/Heap.h:683: void dumpMemoryInto(WebProcessMemoryDump*, const String* allocatorBaseName); On 2015/05/21 18:43:10, Primiano Tucci wrote: > nit: const String& ? Done. https://codereview.chromium.org/1149673002/diff/80001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1149673002/diff/80001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:395: TRACE_EVENT_CATEGORY_GROUP_ENABLED(TRACE_DISABLED_BY_DEFAULT("memory-infra"), &enabled); On 2015/05/21 18:43:10, Primiano Tucci wrote: > Ok to this as a temporary hack, but plz add a cleanup TODO here saying that this > should be exposed by meminfra soon. Done. Check heap.cpp https://codereview.chromium.org/1149673002/diff/80001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:413: SNAPSHOT_HEAP(NormalPage1); On 2015/05/21 18:43:10, Primiano Tucci wrote: > You either want to remove the ; here (otherwise some compilers will complain > about empty statement), or turn the #defines into > #define SNAPSHOT_HEAP(HeapType) \ > do { ... stuff ... } while(0) > > So the expansion ends up with a ; after the while(0). > Not sure what is the most common patter, go figure out in cs :) This is the style in the same file. function ThreadState::snapshot(). https://codereview.chromium.org/1149673002/diff/80001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:429: // (crbug.com/490087). On 2015/05/21 18:43:10, Primiano Tucci wrote: > Maybe add a line here saying: For the moment the dump is periodically invoked by > the MemoryDumpManager every X secs. done at heap.cpp https://codereview.chromium.org/1149673002/diff/80001/Source/platform/heap/Th... Source/platform/heap/ThreadState.cpp:1282: return true; On 2015/05/21 18:43:10, Primiano Tucci wrote: > Ok for now, but this should really return false. > If we return true we will confuse the UI. it is ok for the moment, as this is > unlikely but should be fixed soon. > > Please add a TODO and reference crbug.com/490783 (I've just created) and reword > the comment as. > // TODO(ssid): Temporary workaround while crbug.com/490783 gets fixed. This > should return false in the unlikely case that we didn't see a GC yet, which in > turn will cause the dump to be invalidated. Not sure if this is relevant any more. https://codereview.chromium.org/1149673002/diff/80001/Source/platform/heap/Th... File Source/platform/heap/ThreadState.h (right): https://codereview.chromium.org/1149673002/diff/80001/Source/platform/heap/Th... Source/platform/heap/ThreadState.h:168: class PLATFORM_EXPORT ThreadState : public WebMemoryDumpProvider { On 2015/05/21 18:43:10, Primiano Tucci wrote: > We discussed this offline (there is a thread in the internal oilpan ML pending). > There seems to be one ThreadState per therad, but PostGC seems to be called once > on a single (probably main) thread, and scans all the threads. > As a consequence, having this as the dumper is odd because: > - Will give the false expectation that the onMemoryDump is called on the same > Thread of this ThreadState, while that will not be the case. > - Will end up register one dumper per thread, and it seems that we don't really > need it. > > We should have an external dumper class (just one). > All the thread states should dumpInto a shared WebProcessMemoryDump (obtained > through the new API that is landing), and later on the dumper should just merge > (takeAllDumps()) that dump into the WPMD passed to onMemoryDump. > If my understandings of the oilpan threading model are correct, we can do this > by using just one extra WPMD, no locks, and no double-buffering. Done. https://codereview.chromium.org/1149673002/diff/80001/Source/platform/heap/Th... Source/platform/heap/ThreadState.h:463: void dumpMemoryIfNecessary(); On 2015/05/21 18:43:10, Primiano Tucci wrote: > Had a second thought about this, please don't hate me, I'm very terrible in > naming decisions. > How about dumpIfMemoryProfilingIsEnabled() ? > "necessary" is a bit generic :) > If you are not convinced, don't worry, probably in the next patchset I will > propose another different name :P Haha now there is no need to think about it. :) Peace https://codereview.chromium.org/1149673002/diff/80001/Source/platform/heap/Th... Source/platform/heap/ThreadState.h:618: bool onMemoryDump(WebProcessMemoryDump*) override; On 2015/05/21 18:43:10, Primiano Tucci wrote: > This should go away from here, as per discussion up. Done. https://codereview.chromium.org/1149673002/diff/80001/Source/platform/heap/Th... Source/platform/heap/ThreadState.h:717: OwnPtr<WebProcessMemoryDump> m_lastProcessMemoryDump; On 2015/05/21 18:43:10, Primiano Tucci wrote: > And this should be just one, shared by all the ThreadState(s). Done.
Sorry for the long backs and forths, I think we are close. I know this CL is a bit of effort, but once we get this right all the rest will be much easier. This seems to go in the right direction. I have very few nits here and there (see comments inline). The major changes that I would do here are: - moving the extra WPMD to the BlinkGCMemoryDumpProvider: in this way our impact on Oilpan code is minimal, and we hide all the dust (all the boilerplate needed) under our carpet (BlinkGCMemoryDumpProvider). - You just made me ralize that the heap has init and shutdown methods. But BlinkGCMemoryDumpProvider is initialized in platform::init and platform::shutdown. How do those two relate? probably we should register/unregister BlinkGCMemoryDumpProvider in heap::init/shutdown? The important is that we make sure that we never try to access Heap before is initialized and after it is shutdown. - You made me realize that I forgot to add a .clear() method to WPMD. This is really my sole fault. In the meanwhile it's ok to to the new/free as a temporary hack, but plz file a p1 bug to me saying "PMD/WPMD should have a .clear method" and assign it to me, i'll fix it once I'm back. Thanks https://codereview.chromium.org/1149673002/diff/120001/Source/platform/heap/B... File Source/platform/heap/BlinkGCMemoryDumpProvider.cpp (left): https://codereview.chromium.org/1149673002/diff/120001/Source/platform/heap/B... Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:34: BlinkGCMemoryDumpProvider::~BlinkGCMemoryDumpProvider() Is this intended? I think you accidentally removed the dtor (it's still there in the header) https://codereview.chromium.org/1149673002/diff/120001/Source/platform/heap/B... File Source/platform/heap/BlinkGCMemoryDumpProvider.cpp (right): https://codereview.chromium.org/1149673002/diff/120001/Source/platform/heap/B... Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:35: memoryDump->takeAllDumpsFrom(Heap::getLastProcessMemoryDump()); Ok here you need the TODO pointing to crbug.com/490783 and you should have an "else: return false" once 490783 gets fixed. https://codereview.chromium.org/1149673002/diff/120001/Source/platform/heap/H... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1149673002/diff/120001/Source/platform/heap/H... Source/platform/heap/Heap.cpp:1673: if (s_lastProcessMemoryDump) { See my other consideration. If you move this to BlinkGCMemoryDumpProvider, these lines go away. https://codereview.chromium.org/1149673002/diff/120001/Source/platform/heap/H... Source/platform/heap/Heap.cpp:1885: bool enabled; s /enabled/memory_tracing_enabled/ https://codereview.chromium.org/1149673002/diff/120001/Source/platform/heap/H... Source/platform/heap/Heap.cpp:2158: void Heap::clearLastProcessMemoryDump() Ouch, this is really my fault, I forgot that WPMD should have add a .clear() method. Let's do this: if you follow my suggestion to move the extra WPMD to BlinkGCMemoryDumpProvider, you can remove this here and add a method to BlinkGCMemoryDumpProvider like: BlinkGCMemoryDumpProvider::clearProcessMemoryDumpForLastGC(). For the moment you will do the delete and re-create (but in BlinkGCMemoryDumpProvider instead of this file), and once I fix the api you will just use clear(). Rationale: I want to avoid doing news and frees as mucha s possible. https://codereview.chromium.org/1149673002/diff/120001/Source/platform/heap/H... Source/platform/heap/Heap.cpp:2284: WebProcessMemoryDump* Heap::s_lastProcessMemoryDump; This also goes away :) https://codereview.chromium.org/1149673002/diff/120001/Source/platform/heap/H... File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/1149673002/diff/120001/Source/platform/heap/H... Source/platform/heap/Heap.h:1000: static WebProcessMemoryDump* getLastProcessMemoryDump(); Hmm I think it would be cleaner if we keep the extra WPMD in the BlinkGCMemoryDumpProvider, so this file stays cleaner. In essence you should have BlinkGCMemoryDumpProvider::getProcessMemoryDumpForLastGC() (no need to make it static, BlinkGCMemoryDumpProvider is already singleton), and remove these two lines here. https://codereview.chromium.org/1149673002/diff/120001/Source/platform/heap/H... Source/platform/heap/Heap.h:1043: static WebProcessMemoryDump* s_lastProcessMemoryDump; Same here, if you move the WPMD to the GCDumpprovider, this goes away https://codereview.chromium.org/1149673002/diff/120001/Source/platform/heap/T... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1149673002/diff/120001/Source/platform/heap/T... Source/platform/heap/ThreadState.cpp:385: { Should we have an ASSERT(isInGC()) here, WDYT? https://codereview.chromium.org/1149673002/diff/120001/Source/platform/heap/T... Source/platform/heap/ThreadState.cpp:389: if (isMainThread()) { \ nit: the \ on line 389 is misaligned :)
Moving the object to dump provider sounds good. Regarding registering the dump provider, it is better to have it at Heap::init, which is done by blinkPlatformImpl::initialize after platform is initialized. So, it is safer. https://codereview.chromium.org/1149673002/diff/120001/Source/platform/heap/B... File Source/platform/heap/BlinkGCMemoryDumpProvider.cpp (left): https://codereview.chromium.org/1149673002/diff/120001/Source/platform/heap/B... Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:34: BlinkGCMemoryDumpProvider::~BlinkGCMemoryDumpProvider() On 2015/05/22 15:50:21, Primiano Tucci wrote: > Is this intended? I think you accidentally removed the dtor (it's still there in > the header) It is moved up to maintain the function order in header. https://codereview.chromium.org/1149673002/diff/120001/Source/platform/heap/B... File Source/platform/heap/BlinkGCMemoryDumpProvider.cpp (right): https://codereview.chromium.org/1149673002/diff/120001/Source/platform/heap/B... Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:35: memoryDump->takeAllDumpsFrom(Heap::getLastProcessMemoryDump()); On 2015/05/22 15:50:21, Primiano Tucci wrote: > Ok here you need the TODO pointing to crbug.com/490783 and you should have an > "else: return false" once 490783 gets fixed. But we are dumping some overall numbers even if the last gc dump isn't available. Also, there can be cases where we are dumping empty dumps, which can't be detected here. ie, no GC happened in between and we still swap the empty object. https://codereview.chromium.org/1149673002/diff/120001/Source/platform/heap/T... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1149673002/diff/120001/Source/platform/heap/T... Source/platform/heap/ThreadState.cpp:385: { On 2015/05/22 15:50:21, Primiano Tucci wrote: > Should we have an ASSERT(isInGC()) here, WDYT? The method which calls this (Heap::postGC()) already has the assert. I don't think it is required here.
Made changes. PTAL. https://codereview.chromium.org/1149673002/diff/120001/Source/platform/heap/H... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1149673002/diff/120001/Source/platform/heap/H... Source/platform/heap/Heap.cpp:1673: if (s_lastProcessMemoryDump) { On 2015/05/22 15:50:21, Primiano Tucci wrote: > See my other consideration. If you move this to BlinkGCMemoryDumpProvider, these > lines go away. Done. https://codereview.chromium.org/1149673002/diff/120001/Source/platform/heap/H... Source/platform/heap/Heap.cpp:1885: bool enabled; On 2015/05/22 15:50:21, Primiano Tucci wrote: > s /enabled/memory_tracing_enabled/ Done. https://codereview.chromium.org/1149673002/diff/120001/Source/platform/heap/H... Source/platform/heap/Heap.cpp:2158: void Heap::clearLastProcessMemoryDump() On 2015/05/22 15:50:21, Primiano Tucci wrote: > Ouch, this is really my fault, I forgot that WPMD should have add a .clear() > method. > Let's do this: if you follow my suggestion to move the extra WPMD to > BlinkGCMemoryDumpProvider, you can remove this here and add a method to > BlinkGCMemoryDumpProvider like: > BlinkGCMemoryDumpProvider::clearProcessMemoryDumpForLastGC(). > > For the moment you will do the delete and re-create (but in > BlinkGCMemoryDumpProvider instead of this file), and once I fix the api you will > just use clear(). > Rationale: I want to avoid doing news and frees as mucha s possible. Yes i know. Looked like this file had lots of new(s) and free(s), so i just did this. moved to ownptr now. https://codereview.chromium.org/1149673002/diff/120001/Source/platform/heap/H... Source/platform/heap/Heap.cpp:2284: WebProcessMemoryDump* Heap::s_lastProcessMemoryDump; On 2015/05/22 15:50:21, Primiano Tucci wrote: > This also goes away :) Done. https://codereview.chromium.org/1149673002/diff/120001/Source/platform/heap/H... File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/1149673002/diff/120001/Source/platform/heap/H... Source/platform/heap/Heap.h:1000: static WebProcessMemoryDump* getLastProcessMemoryDump(); On 2015/05/22 15:50:21, Primiano Tucci wrote: > Hmm I think it would be cleaner if we keep the extra WPMD in the > BlinkGCMemoryDumpProvider, so this file stays cleaner. > In essence you should have > BlinkGCMemoryDumpProvider::getProcessMemoryDumpForLastGC() (no need to make it > static, BlinkGCMemoryDumpProvider is already singleton), and remove these two > lines here. Done. https://codereview.chromium.org/1149673002/diff/120001/Source/platform/heap/H... Source/platform/heap/Heap.h:1043: static WebProcessMemoryDump* s_lastProcessMemoryDump; On 2015/05/22 15:50:21, Primiano Tucci wrote: > Same here, if you move the WPMD to the GCDumpprovider, this goes away Done. https://codereview.chromium.org/1149673002/diff/120001/Source/platform/heap/T... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1149673002/diff/120001/Source/platform/heap/T... Source/platform/heap/ThreadState.cpp:389: if (isMainThread()) { \ On 2015/05/22 15:50:21, Primiano Tucci wrote: > nit: the \ on line 389 is misaligned :) Done.
LGTM from my side with some final nits, thanks for the patience, I think we are good to go for the next level-up with some final small changes :) Fix the nits and add back the CC list (oilpan-reviews & co) + haraken. Note that you have some nits in the commit message: "the the given memory dump" > A global dump request is called each time a snapshot is taken, so that the memory snapshot taken will be used in the tracing. Uh? This is not true, that's a TODO for the future, right? I'd make the commit message a bit more concise and straightforward, and less implementaiton-detail oriented. How about: This CL adds infrastructure to dump memory stats from blink GC. Each time a GC happens, a dump is kept around in the BlinkGC...Provider (but only if memory tracing is enabled). When the onMemoryDump request comes from the MemoryInfra coordinator, the dump for the last GC is attached to the global memory dump. https://codereview.chromium.org/1149673002/diff/160001/Source/platform/heap/B... File Source/platform/heap/BlinkGCMemoryDumpProvider.cpp (right): https://codereview.chromium.org/1149673002/diff/160001/Source/platform/heap/B... Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:29: WebMemoryAllocatorDump* allocatorDump = memoryDump->createMemoryAllocatorDump("blink_gc"); No need to do in this CL, but I think that, for the sake of consistency, in the next CLs this should be moved to a public method in this class, and invoked by ThreadState::PostGC, so we dump everything at the same time. https://codereview.chromium.org/1149673002/diff/160001/Source/platform/heap/B... Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:35: if (m_lastProcessMemoryDump.get()) nit: OwnPtr, like scoped_ptr, has the implicit-conversion-to-bool operator, no need of the .get() here https://codereview.chromium.org/1149673002/diff/160001/Source/platform/heap/B... Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:47: BlinkGCMemoryDumpProvider::BlinkGCMemoryDumpProvider() micro-nit: I think it was right to keep this on the top, as it respect the decl order (also people tend to expect ctors in the beginning) https://codereview.chromium.org/1149673002/diff/160001/Source/platform/heap/B... Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:57: void BlinkGCMemoryDumpProvider::clearLastProcessMemoryDump() Nit: +GC (i.e. clearLastGC...) https://codereview.chromium.org/1149673002/diff/160001/Source/platform/heap/H... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1149673002/diff/160001/Source/platform/heap/H... Source/platform/heap/Heap.cpp:46: #include "public/platform/WebProcessMemoryDump.h" I think you don't need this anymore https://codereview.chromium.org/1149673002/diff/160001/Source/platform/heap/H... Source/platform/heap/Heap.cpp:1642: if (Platform::current()->mainThread()) Question for haraken@: for production (i.e. non tests) is the global Heap:: always initialized and torn down on the main thread? https://codereview.chromium.org/1149673002/diff/160001/Source/platform/heap/H... Source/platform/heap/Heap.cpp:1887: bool enabled; Can you encapsulate this into a "static inline bool" method of BlinkGCMemoryDumpProvider, so we hide this dust under our class, and in future just fix ours? P.S. I know that BlinkGCMemoryDumpProvider is a singleton, but just for this one use a "static inline bool" as we want this to be a fastpath. https://codereview.chromium.org/1149673002/diff/160001/Source/platform/heap/H... File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/1149673002/diff/160001/Source/platform/heap/H... Source/platform/heap/Heap.h:98: class WebProcessMemoryDump; I think you don't need this anymore. https://codereview.chromium.org/1149673002/diff/160001/Source/platform/heap/T... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1149673002/diff/160001/Source/platform/heap/T... Source/platform/heap/ThreadState.cpp:42: #include "public/platform/WebProcessMemoryDump.h" I think you don't need this anymore
Made changes, can we add haraken? https://codereview.chromium.org/1149673002/diff/160001/Source/platform/heap/B... File Source/platform/heap/BlinkGCMemoryDumpProvider.cpp (right): https://codereview.chromium.org/1149673002/diff/160001/Source/platform/heap/B... Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:29: WebMemoryAllocatorDump* allocatorDump = memoryDump->createMemoryAllocatorDump("blink_gc"); On 2015/05/22 19:45:57, Primiano Tucci wrote: > No need to do in this CL, but I think that, for the sake of consistency, in the > next CLs this should be moved to a public method in this class, and invoked by > ThreadState::PostGC, so we dump everything at the same time. Done. https://codereview.chromium.org/1149673002/diff/160001/Source/platform/heap/B... Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:35: if (m_lastProcessMemoryDump.get()) On 2015/05/22 19:45:57, Primiano Tucci wrote: > nit: OwnPtr, like scoped_ptr, has the implicit-conversion-to-bool operator, no > need of the .get() here Done. https://codereview.chromium.org/1149673002/diff/160001/Source/platform/heap/B... Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:47: BlinkGCMemoryDumpProvider::BlinkGCMemoryDumpProvider() On 2015/05/22 19:45:57, Primiano Tucci wrote: > micro-nit: I think it was right to keep this on the top, as it respect the decl > order (also people tend to expect ctors in the beginning) Keeping it in decl order. https://codereview.chromium.org/1149673002/diff/160001/Source/platform/heap/B... Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:57: void BlinkGCMemoryDumpProvider::clearLastProcessMemoryDump() On 2015/05/22 19:45:57, Primiano Tucci wrote: > Nit: +GC (i.e. clearLastGC...) Done. https://codereview.chromium.org/1149673002/diff/160001/Source/platform/heap/H... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1149673002/diff/160001/Source/platform/heap/H... Source/platform/heap/Heap.cpp:46: #include "public/platform/WebProcessMemoryDump.h" On 2015/05/22 19:45:57, Primiano Tucci wrote: > I think you don't need this anymore Done. https://codereview.chromium.org/1149673002/diff/160001/Source/platform/heap/H... Source/platform/heap/Heap.cpp:1642: if (Platform::current()->mainThread()) On 2015/05/22 19:45:57, Primiano Tucci wrote: > Question for haraken@: for production (i.e. non tests) is the global Heap:: > always initialized and torn down on the main thread? changed it. https://codereview.chromium.org/1149673002/diff/160001/Source/platform/heap/H... Source/platform/heap/Heap.cpp:1887: bool enabled; On 2015/05/22 19:45:57, Primiano Tucci wrote: > Can you encapsulate this into a "static inline bool" method of > BlinkGCMemoryDumpProvider, so we hide this dust under our class, and in future > just fix ours? > P.S. I know that BlinkGCMemoryDumpProvider is a singleton, but just for this one > use a "static inline bool" as we want this to be a fastpath. Done. https://codereview.chromium.org/1149673002/diff/160001/Source/platform/heap/H... File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/1149673002/diff/160001/Source/platform/heap/H... Source/platform/heap/Heap.h:98: class WebProcessMemoryDump; On 2015/05/22 19:45:57, Primiano Tucci wrote: > I think you don't need this anymore. Done. https://codereview.chromium.org/1149673002/diff/160001/Source/platform/heap/T... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1149673002/diff/160001/Source/platform/heap/T... Source/platform/heap/ThreadState.cpp:42: #include "public/platform/WebProcessMemoryDump.h" On 2015/05/22 19:45:57, Primiano Tucci wrote: > I think you don't need this anymore Done.
primiano@chromium.org changed reviewers: + haraken@chromium.org
+haraken for the next level up :) LGTM from my side with some final small nits. Thanks for all the hard work! https://codereview.chromium.org/1149673002/diff/180001/Source/platform/heap/B... File Source/platform/heap/BlinkGCMemoryDumpProvider.cpp (right): https://codereview.chromium.org/1149673002/diff/180001/Source/platform/heap/B... Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:54: WebMemoryAllocatorDump* BlinkGCMemoryDumpProvider::createMemoryAllocatorDumpForLastGC(const String& absoluteName) I think this should be PassOwnPtr<WebMemoryAllocatorDump> to make explicit the fact that you want the caller to take the ownership of the WMAD https://codereview.chromium.org/1149673002/diff/180001/Source/platform/heap/B... Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:59: void BlinkGCMemoryDumpProvider::clearLastGCProcessMemoryDump() And since you are here, I just realized that a better name for this could be clearAllMemoryAllocatorDumpsForLastGC(), for naming consistency with the above https://codereview.chromium.org/1149673002/diff/180001/Source/platform/heap/H... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1149673002/diff/180001/Source/platform/heap/H... Source/platform/heap/Heap.cpp:1884: bool enabled = BlinkGCMemoryDumpProvider::isMemoryTracingEnabled(); nit: const bool isMemoryTracingEnabled = BlinkGCMemoryDumpProvider::isMemoryTracingEnabled();
https://codereview.chromium.org/1149673002/diff/180001/Source/platform/heap/B... File Source/platform/heap/BlinkGCMemoryDumpProvider.cpp (right): https://codereview.chromium.org/1149673002/diff/180001/Source/platform/heap/B... Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:36: // If available, the last GC memory statistics are added. I'm not sure if this is a good design. I think we have two options: Option 1: Take the memory dump at each GC and return the latest dump when onMemoryDump is requested. Option 2: When onMemoryDump is requested, force a GC (which disables sweeping) and get the dump. I guess Option 2 would be more straightforward. As you pointed out in the above comment, the Option 1 will end up with the inconsistency between "inner_size", "outer_size" etc and the dump taken in the latest GC. https://codereview.chromium.org/1149673002/diff/180001/Source/platform/heap/H... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1149673002/diff/180001/Source/platform/heap/H... Source/platform/heap/Heap.cpp:1891: state->dumpMemory(); postGC wouldn't be a good timing to take memory dump, because it runs _before_ collecting objects. As commented in the other place, I guess a better way to take the dump would be to forced a sweep-disabled version of GC when onMemoryDump is requested. Then the concern about the dump timing is gone -- onMemoryDump can just take the dump when it is requested. If you want, I can implement Heap::collectGarbageWithoutSweeping for you. https://codereview.chromium.org/1149673002/diff/180001/Source/platform/heap/T... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1149673002/diff/180001/Source/platform/heap/T... Source/platform/heap/ThreadState.cpp:399: SNAPSHOT_HEAP(NormalPage4); We recently added "EagerSweep" :) Please just add it here.
Thanks for the low-latency reply Kentaro! See my comments inline. https://codereview.chromium.org/1149673002/diff/180001/Source/platform/heap/B... File Source/platform/heap/BlinkGCMemoryDumpProvider.cpp (right): https://codereview.chromium.org/1149673002/diff/180001/Source/platform/heap/B... Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:36: // If available, the last GC memory statistics are added. > I'm not sure if this is a good design. I think we have two options: See my previous comment on this [1]. Regardless of what option we choose, we should dump all these stats at the same time. [1] https://codereview.chromium.org/1149673002/diff/160001/Source/platform/heap/B... > Option 1: Take the memory dump at each GC and return the latest dump when > onMemoryDump is requested. This is a good option. I think we can smooth this a bit and say: request a dump when a GC happens. The dump might happen or not (the manager might decide to rate-limit), and if it happens we know that all the oilpan numbers are taken at the same time. This is sub-optimal (because the oilpan numbers might be earlier than other dumpers' numbers) but probably easier to get in the short term. > Option 2: When onMemoryDump is requested, force a GC (which disables sweeping) > and get the dump. No idea of how much complex would be to implement this. What happens if the other threads are active? Does it mean that the thread on which onMemoryDump is invoked (the main thread) is going to wait for the other thread to enter a safe point and proceed? > I guess Option 2 would be more straightforward. If this is not too complex to implement we can go straight to option 2. Otherwise we could get started with Option 1 (% fixing the consistency issue) and then migrate to the on-demand GC when available. > As you pointed out in the above > comment, the Option 1 will end up with the inconsistency between "inner_size", > "outer_size" etc and the dump taken in the latest GC. Yeah, I agree with this. Regardless of the option, let's dump everything together. https://codereview.chromium.org/1149673002/diff/180001/Source/platform/heap/H... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1149673002/diff/180001/Source/platform/heap/H... Source/platform/heap/Heap.cpp:1891: state->dumpMemory(); On 2015/05/26 12:21:59, haraken wrote: > > postGC wouldn't be a good timing to take memory dump, because it runs _before_ > collecting objects. Oh, I think did it in PostGC because the existing HeapVisualizer code was creating snapshots there. > As commented in the other place, I guess a better way to > take the dump would be to forced a sweep-disabled version of GC when > onMemoryDump is requested. Then the concern about the dump timing is gone -- > onMemoryDump can just take the dump when it is requested. See my comment there. If we go for the Option1-first-then-Option2 plan what is the right place to collect the stats for the last GC? > > If you want, I can implement Heap::collectGarbageWithoutSweeping for you. See my comments above. Depending on how much time this would take, we might get started with a suboptimal (but locally consistent) dump @ GC, and the move to the on-demand one. But if collectGarbageWithoutSweeping is a matter of small refactoring, we can just do it soon.
On 2015/05/26 14:18:03, Primiano Tucci wrote: > Thanks for the low-latency reply Kentaro! > See my comments inline. > > https://codereview.chromium.org/1149673002/diff/180001/Source/platform/heap/B... > File Source/platform/heap/BlinkGCMemoryDumpProvider.cpp (right): > > https://codereview.chromium.org/1149673002/diff/180001/Source/platform/heap/B... > Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:36: // If available, the last > GC memory statistics are added. > > I'm not sure if this is a good design. I think we have two options: > See my previous comment on this [1]. Regardless of what option we choose, we > should dump all these stats at the same time. > > [1] > https://codereview.chromium.org/1149673002/diff/160001/Source/platform/heap/B... > > > Option 1: Take the memory dump at each GC and return the latest dump when > > onMemoryDump is requested. > This is a good option. I think we can smooth this a bit and say: request a dump > when a GC happens. The dump might happen or not (the manager might decide to > rate-limit), and if it happens we know that all the oilpan numbers are taken at > the same time. > This is sub-optimal (because the oilpan numbers might be earlier than other > dumpers' numbers) but probably easier to get in the short term. > > > > > Option 2: When onMemoryDump is requested, force a GC (which disables sweeping) > > and get the dump. > > No idea of how much complex would be to implement this. What happens if the > other threads are active? Does it mean that the thread on which onMemoryDump is > invoked (the main thread) is going to wait for the other thread to enter a safe > point and proceed? We need to wait for all threads to enter safe points, but it won't take much time in common cases. If it takes a lot of time, it is already practically problematic because it means that the main thread cannot start a GC when it wants to start :) > https://codereview.chromium.org/1149673002/diff/180001/Source/platform/heap/H... > Source/platform/heap/Heap.cpp:1891: state->dumpMemory(); > On 2015/05/26 12:21:59, haraken wrote: > > > > postGC wouldn't be a good timing to take memory dump, because it runs _before_ > > collecting objects. > > Oh, I think did it in PostGC because the existing HeapVisualizer code was > creating snapshots there. HeapVisualizer is taking the snapshot very carefully so that it counts only live objects by scanning headers of all objects etc. If you want to dump the memory stats in postGC, you need to do a similar thing. I guess that will be complex and it will be easier to go with the Option 2. > > If you want, I can implement Heap::collectGarbageWithoutSweeping for you. > > See my comments above. Depending on how much time this would take, we might get > started with a suboptimal (but locally consistent) dump @ GC, and the move to > the on-demand one. > But if collectGarbageWithoutSweeping is a matter of small refactoring, we can > just do it soon. I'll take a look tomorrow, but it wouldn't be that hard to implement Heap::collectGarbageWithoutSweeping.
Patchset #11 (id:200001) has been deleted
Patchset #11 (id:220001) has been deleted
Made changes for forcing GC and taking snapshot. PTAL. https://codereview.chromium.org/1149673002/diff/180001/Source/platform/heap/B... File Source/platform/heap/BlinkGCMemoryDumpProvider.cpp (right): https://codereview.chromium.org/1149673002/diff/180001/Source/platform/heap/B... Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:54: WebMemoryAllocatorDump* BlinkGCMemoryDumpProvider::createMemoryAllocatorDumpForLastGC(const String& absoluteName) On 2015/05/26 11:24:06, Primiano Tucci wrote: > I think this should be PassOwnPtr<WebMemoryAllocatorDump> to make explicit the > fact that you want the caller to take the ownership of the WMAD No, the caller can't take the ownership of the ptr because it can delete the ptr which will result in the memoryDump not being able to use the dump. https://codereview.chromium.org/1149673002/diff/180001/Source/platform/heap/B... Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:59: void BlinkGCMemoryDumpProvider::clearLastGCProcessMemoryDump() On 2015/05/26 11:24:06, Primiano Tucci wrote: > And since you are here, I just realized that a better name for this could be > clearAllMemoryAllocatorDumpsForLastGC(), for naming consistency with the above removed method. https://codereview.chromium.org/1149673002/diff/180001/Source/platform/heap/H... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1149673002/diff/180001/Source/platform/heap/H... Source/platform/heap/Heap.cpp:1884: bool enabled = BlinkGCMemoryDumpProvider::isMemoryTracingEnabled(); On 2015/05/26 11:24:06, Primiano Tucci wrote: > nit: const bool isMemoryTracingEnabled = > BlinkGCMemoryDumpProvider::isMemoryTracingEnabled(); Done. https://codereview.chromium.org/1149673002/diff/180001/Source/platform/heap/T... File Source/platform/heap/ThreadState.cpp (right): https://codereview.chromium.org/1149673002/diff/180001/Source/platform/heap/T... Source/platform/heap/ThreadState.cpp:399: SNAPSHOT_HEAP(NormalPage4); On 2015/05/26 12:21:59, haraken wrote: > > We recently added "EagerSweep" :) Please just add it here. Done.
Made changes for forcing GC and taking snapshot. PTAL.
Looks good from the memory infra side with very minor comments (mostly comments on comments :) ). https://codereview.chromium.org/1149673002/diff/180001/Source/platform/heap/B... File Source/platform/heap/BlinkGCMemoryDumpProvider.cpp (right): https://codereview.chromium.org/1149673002/diff/180001/Source/platform/heap/B... Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:54: WebMemoryAllocatorDump* BlinkGCMemoryDumpProvider::createMemoryAllocatorDumpForLastGC(const String& absoluteName) On 2015/05/27 13:15:06, ssid wrote: > On 2015/05/26 11:24:06, Primiano Tucci wrote: > > I think this should be PassOwnPtr<WebMemoryAllocatorDump> to make explicit the > > fact that you want the caller to take the ownership of the WMAD > > No, the caller can't take the ownership of the ptr because it can delete the ptr > which will result in the memoryDump not being able to use the dump. Oh right, I didn't realize this comes from createMemoryAllocatorDump, which owns the dump. A raw pointer it's fine then. https://codereview.chromium.org/1149673002/diff/240001/Source/platform/heap/B... File Source/platform/heap/BlinkGCMemoryDumpProvider.cpp (right): https://codereview.chromium.org/1149673002/diff/240001/Source/platform/heap/B... Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:29: Heap::collectGarbage(ThreadState::NoHeapPointersOnStack, ThreadState::TakeSnapshot, Heap::ForcedGC); Can you add a comment before this line clarifing that the collectGarbage will create WMADs via createMemoryAllocatorDumpForCurrentGC in its call stack. https://codereview.chromium.org/1149673002/diff/240001/Source/platform/heap/B... Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:36: memoryDump->takeAllDumpsFrom(m_currentProcessMemoryDump.get()); Or alternatively you can comment here saying: merge all the dumps generated during the Heap::collectGarbage invocation https://codereview.chromium.org/1149673002/diff/240001/Source/platform/heap/B... Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:47: m_currentProcessMemoryDump = adoptPtr(Platform::current()->createProcessMemoryDump()); Can you do this in the ctor initializer list? Otherwise I wonder if the clang checked will falsely barf about m_currentProcessMemoryDump being not initialized. https://codereview.chromium.org/1149673002/diff/240001/Source/platform/heap/B... File Source/platform/heap/BlinkGCMemoryDumpProvider.h (right): https://codereview.chromium.org/1149673002/diff/240001/Source/platform/heap/B... Source/platform/heap/BlinkGCMemoryDumpProvider.h:9: #include "platform/TraceEvent.h" do you really need this or is just a leftover from the previous patchsets? https://codereview.chromium.org/1149673002/diff/240001/Source/platform/heap/B... Source/platform/heap/BlinkGCMemoryDumpProvider.h:25: WebMemoryAllocatorDump* createMemoryAllocatorDumpForCurrentGC(const String& absoluteName); Can you add a comment clarifying that: - the returned WebMemoryAllocatorDump is owned by BlinkGCMemoryDumpProvider - the returned pointer should not be retained (just used to dump in the current call stack).
Made changes, PTAL https://codereview.chromium.org/1149673002/diff/240001/Source/platform/heap/B... File Source/platform/heap/BlinkGCMemoryDumpProvider.cpp (right): https://codereview.chromium.org/1149673002/diff/240001/Source/platform/heap/B... Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:29: Heap::collectGarbage(ThreadState::NoHeapPointersOnStack, ThreadState::TakeSnapshot, Heap::ForcedGC); On 2015/05/27 14:19:30, Primiano Tucci wrote: > Can you add a comment before this line clarifing that the collectGarbage will > create WMADs via createMemoryAllocatorDumpForCurrentGC in its call stack. Added below. https://codereview.chromium.org/1149673002/diff/240001/Source/platform/heap/B... Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:36: memoryDump->takeAllDumpsFrom(m_currentProcessMemoryDump.get()); On 2015/05/27 14:19:30, Primiano Tucci wrote: > Or alternatively you can comment here saying: > merge all the dumps generated during the Heap::collectGarbage invocation Done. https://codereview.chromium.org/1149673002/diff/240001/Source/platform/heap/B... Source/platform/heap/BlinkGCMemoryDumpProvider.cpp:47: m_currentProcessMemoryDump = adoptPtr(Platform::current()->createProcessMemoryDump()); On 2015/05/27 14:19:30, Primiano Tucci wrote: > Can you do this in the ctor initializer list? Otherwise I wonder if the clang > checked will falsely barf about m_currentProcessMemoryDump being not > initialized. Done. https://codereview.chromium.org/1149673002/diff/240001/Source/platform/heap/B... File Source/platform/heap/BlinkGCMemoryDumpProvider.h (right): https://codereview.chromium.org/1149673002/diff/240001/Source/platform/heap/B... Source/platform/heap/BlinkGCMemoryDumpProvider.h:9: #include "platform/TraceEvent.h" On 2015/05/27 14:19:30, Primiano Tucci wrote: > do you really need this or is just a leftover from the previous patchsets? Forgot to upload the last patch. https://codereview.chromium.org/1149673002/diff/240001/Source/platform/heap/B... Source/platform/heap/BlinkGCMemoryDumpProvider.h:25: WebMemoryAllocatorDump* createMemoryAllocatorDumpForCurrentGC(const String& absoluteName); On 2015/05/27 14:19:31, Primiano Tucci wrote: > Can you add a comment clarifying that: > - the returned WebMemoryAllocatorDump is owned by BlinkGCMemoryDumpProvider > - the returned pointer should not be retained (just used to dump in the current > call stack). Done.
LGTM
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 Link to the patchset: https://codereview.chromium.org/1149673002/#ps260001 (title: "Adding suggested comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1149673002/260001
The CQ bit was unchecked by ssid@chromium.org
On 2015/05/28 12:33:19, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1149673002/260001 Can this patch land now? Is the forced GC stable enough?
Patchset #15 (id:320001) has been deleted
On 2015/06/09 20:41:20, ssid wrote: > On 2015/05/28 12:33:19, commit-bot: I haz the power wrote: > > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/patch-status/1149673002/260001 > > Can this patch land now? Is the forced GC stable enough? Haraken, can we land this now or are other changes to oilpan required?
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, primiano@chromium.org Link to the patchset: https://codereview.chromium.org/1149673002/#ps420001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1149673002/420001
The CQ bit was unchecked by ssid@chromium.org
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, primiano@chromium.org Link to the patchset: https://codereview.chromium.org/1149673002/#ps440001 (title: "Rebase and replacing main thread with thread id.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1149673002/440001
Message was sent while issue was closed.
Committed patchset #20 (id:440001) as https://src.chromium.org/viewvc/blink?view=rev&revision=197635
Message was sent while issue was closed.
xhwang@chromium.org changed reviewers: + xhwang@chromium.org
Message was sent while issue was closed.
Just synced and I am getting this assert: ASSERTION FAILED: m_currentProcessMemoryDump ../../third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp(57) : blink::BlinkGCMemoryDumpProvider::BlinkGCMemoryDumpProvider()
Message was sent while issue was closed.
I am without a laptop now, but I am pretty sure that fadi Samuel (fsamuel@ iirc) landed a fix in blink very recently. maybe you just need it to roll (look at blink tot, it was a one line change removing the assert ) On 23 Jun 2015 6:31 p.m., <xhwang@chromium.org> wrote: > Just synced and I am getting this assert: > > ASSERTION FAILED: m_currentProcessMemoryDump > > ../../third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp(57) > : blink::BlinkGCMemoryDumpProvider::BlinkGCMemoryDumpProvider() > > > https://codereview.chromium.org/1149673002/ > To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2015/06/23 17:31:54, xhwang wrote: > Just synced and I am getting this assert: > > ASSERTION FAILED: m_currentProcessMemoryDump > ../../third_party/WebKit/Source/platform/heap/BlinkGCMemoryDumpProvider.cpp(57) > : blink::BlinkGCMemoryDumpProvider::BlinkGCMemoryDumpProvider() I am without a laptop now, but I am pretty sure that fadi Samuel (fsamuel@ iirc) landed a fix in blink very recently. maybe you just need it to roll (look at blink tot, it was a one line change removing the assert ) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
