Chromium Code Reviews
|
|
DescriptionAdding heap-page statistics to blink-gc memory dump provider.
This is second version of the blink-gc dump provider. This CL adds
object sizes in each page of the heap to the memory dump. It also
dumps if the object is alive or dead or free.
BUG=490087
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197731
Patch Set 1 #Patch Set 2 : Rebase for changes. #Patch Set 3 : Nits. #Patch Set 4 : Fixing name. #
Total comments: 11
Patch Set 5 : Removing encoded_size. #Patch Set 6 : Fixing error. #
Total comments: 14
Patch Set 7 : Fixing haraken@ comments. #Patch Set 8 : Rebase and change %zu to %lu. #
Dependent Patchsets: Messages
Total messages: 19 (5 generated)
ssid@chromium.org changed reviewers: + picksi@chromium.org, primiano@chromium.org
Second version, PTAL.
Looks good, just few comments https://codereview.chromium.org/1149703002/diff/60001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1149703002/diff/60001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1353: pageName.append(String::format("/page_%zu", pageIndex)); maybe this is a bit more clear and efficient if you have String pageName = String::format("%s/page_%zu", dumpBaseName, pageIndex) https://codereview.chromium.org/1149703002/diff/60001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1368: encodedSize += header->encodedSize(); as above, let's remove this. https://codereview.chromium.org/1149703002/diff/60001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1561: pageName.append(String::format("/page_%zu", pageIndex)); ditto here https://codereview.chromium.org/1149703002/diff/60001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1582: pageDump->AddScalar("free_size", "bytes", 0u); Don't dump these if you know that they don't apply for large object pages. The UI should be smart enough to deal with missing values. https://codereview.chromium.org/1149703002/diff/60001/Source/platform/heap/He... File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/1149703002/diff/60001/Source/platform/heap/He... Source/platform/heap/Heap.h:201: size_t encodedSize() const { return m_encoded; } Any idea what is this? P.S: what is NO_SANITIZE_ADDRESS and why did you remove it? https://codereview.chromium.org/1149703002/diff/60001/Source/platform/heap/He... Source/platform/heap/Heap.h:201: size_t encodedSize() const { return m_encoded; } We had a chat offline on this. I don't think you want to expose and dump this. THis is a union of size() + other metadata.
Made changes. PTAL. https://codereview.chromium.org/1149703002/diff/60001/Source/platform/heap/He... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1149703002/diff/60001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1353: pageName.append(String::format("/page_%zu", pageIndex)); On 2015/05/28 12:47:00, Primiano Tucci wrote: > maybe this is a bit more clear and efficient if you have > String pageName = String::format("%s/page_%zu", dumpBaseName, pageIndex) With offline discussion, this seems better. https://codereview.chromium.org/1149703002/diff/60001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1368: encodedSize += header->encodedSize(); On 2015/05/28 12:47:00, Primiano Tucci wrote: > as above, let's remove this. Done. https://codereview.chromium.org/1149703002/diff/60001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1561: pageName.append(String::format("/page_%zu", pageIndex)); On 2015/05/28 12:47:00, Primiano Tucci wrote: > ditto here ditto. https://codereview.chromium.org/1149703002/diff/60001/Source/platform/heap/He... Source/platform/heap/Heap.cpp:1582: pageDump->AddScalar("free_size", "bytes", 0u); On 2015/05/28 12:47:00, Primiano Tucci wrote: > Don't dump these if you know that they don't apply for large object pages. > The UI should be smart enough to deal with missing values. Done. https://codereview.chromium.org/1149703002/diff/60001/Source/platform/heap/He... File Source/platform/heap/Heap.h (right): https://codereview.chromium.org/1149703002/diff/60001/Source/platform/heap/He... Source/platform/heap/Heap.h:201: size_t encodedSize() const { return m_encoded; } On 2015/05/28 12:47:01, Primiano Tucci wrote: > We had a chat offline on this. I don't think you want to expose and dump this. > THis is a union of size() + other metadata. Done.
primiano@chromium.org changed reviewers: + haraken@chromium.org
Looks good on my side, let's move this to the next level up. +haraken and oilpan-reviews
LGTM https://codereview.chromium.org/1149703002/diff/100001/Source/platform/heap/H... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1149703002/diff/100001/Source/platform/heap/H... Source/platform/heap/Heap.cpp:220: WebMemoryAllocatorDump* allocatorDump = BlinkGCMemoryDumpProvider::instance()->createMemoryAllocatorDumpForCurrentGC(dumpBaseName); I'm just curious but does the place we create the allocatorDump matter? i.e., is it important that we create the allocatorDump for the BaseHeap before creating allocatorDumps for BaseHeapPages? https://codereview.chromium.org/1149703002/diff/100001/Source/platform/heap/H... Source/platform/heap/Heap.cpp:223: page->takeSnapshot(dumpBaseName, pageCount); pageCount => pageIndex https://codereview.chromium.org/1149703002/diff/100001/Source/platform/heap/H... Source/platform/heap/Heap.cpp:1400: String pageName = dumpBaseName.isolatedCopy(); Instead of passing const String& and calling isolatedCopy, I'd prefer just passing 'String'. https://codereview.chromium.org/1149703002/diff/100001/Source/platform/heap/H... Source/platform/heap/Heap.cpp:1412: for (Address addr = payload(); addr < payloadEnd(); addr += header->size()) { addr => headerAddress https://codereview.chromium.org/1149703002/diff/100001/Source/platform/heap/H... Source/platform/heap/Heap.cpp:1427: pageDump->AddScalar("object_count", "objects", objectCount); Remove objectCount. The freed area is not an object, so it doesn't make sense to count up the total number of headers and report it. https://codereview.chromium.org/1149703002/diff/100001/Source/platform/heap/H... Source/platform/heap/Heap.cpp:1631: pageDump->AddScalar("object_count", "objects", 1u); Remove this.
https://codereview.chromium.org/1149703002/diff/100001/Source/platform/heap/H... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1149703002/diff/100001/Source/platform/heap/H... Source/platform/heap/Heap.cpp:220: WebMemoryAllocatorDump* allocatorDump = BlinkGCMemoryDumpProvider::instance()->createMemoryAllocatorDumpForCurrentGC(dumpBaseName); On 2015/05/29 02:11:30, haraken wrote: > > I'm just curious but does the place we create the allocatorDump matter? i.e., is > it important that we create the allocatorDump for the BaseHeap before creating > allocatorDumps for BaseHeapPages? Do you mean the order or the call site? The order in which the dumps are created is irrelevant (in fact, at the end they end up in a dictionary which will hash them). The callsite: well, in general we have to make sure that we create all our dumps before returning the outer onMemoryDump(WebProcessMemoryDump*) call. Here in BlinkGC we use an extra little trick: we create an extra WebProcessMemoryDump (see [1]) that is always available and we use as a "buffer" to create all the allocatorDumps like this one here when traversing the oilpan GC structs. At the end we simply merge (see [2]) the buffer WPMD into the real one that meminfra expects (the argument of onMemoryDump). In this way we can guarantee that even if, for whatever reason, you force a Heap::collectGarbage(takeSnapshow) outside of onMemoryDump(...), everything works fine (you will just end up collecting useless WPMD that will be dropped before the next dump). [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... [2] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://codereview.chromium.org/1149703002/diff/100001/Source/platform/heap/H... Source/platform/heap/Heap.cpp:1400: String pageName = dumpBaseName.isolatedCopy(); On 2015/05/29 02:11:30, haraken wrote: > > Instead of passing const String& and calling isolatedCopy, I'd prefer just > passing 'String'. +1
Patchset #7 (id:120001) has been deleted
Thanks, Made changes suggested. https://codereview.chromium.org/1149703002/diff/100001/Source/platform/heap/H... File Source/platform/heap/Heap.cpp (right): https://codereview.chromium.org/1149703002/diff/100001/Source/platform/heap/H... Source/platform/heap/Heap.cpp:220: WebMemoryAllocatorDump* allocatorDump = BlinkGCMemoryDumpProvider::instance()->createMemoryAllocatorDumpForCurrentGC(dumpBaseName); On 2015/05/29 02:11:30, haraken wrote: > > I'm just curious but does the place we create the allocatorDump matter? i.e., is > it important that we create the allocatorDump for the BaseHeap before creating > allocatorDumps for BaseHeapPages? Just had a chat with primiano@. It is not necessary to move this, but looks cleaner to create the root after children. https://codereview.chromium.org/1149703002/diff/100001/Source/platform/heap/H... Source/platform/heap/Heap.cpp:223: page->takeSnapshot(dumpBaseName, pageCount); On 2015/05/29 02:11:30, haraken wrote: > > pageCount => pageIndex Done. https://codereview.chromium.org/1149703002/diff/100001/Source/platform/heap/H... Source/platform/heap/Heap.cpp:223: page->takeSnapshot(dumpBaseName, pageCount); On 2015/05/29 02:11:30, haraken wrote: > > pageCount => pageIndex Done. https://codereview.chromium.org/1149703002/diff/100001/Source/platform/heap/H... Source/platform/heap/Heap.cpp:1412: for (Address addr = payload(); addr < payloadEnd(); addr += header->size()) { On 2015/05/29 02:11:30, haraken wrote: > > addr => headerAddress Done. https://codereview.chromium.org/1149703002/diff/100001/Source/platform/heap/H... Source/platform/heap/Heap.cpp:1427: pageDump->AddScalar("object_count", "objects", objectCount); On 2015/05/29 02:11:30, haraken wrote: > > Remove objectCount. The freed area is not an object, so it doesn't make sense to > count up the total number of headers and report it. Done, thanks. https://codereview.chromium.org/1149703002/diff/100001/Source/platform/heap/H... Source/platform/heap/Heap.cpp:1631: pageDump->AddScalar("object_count", "objects", 1u); On 2015/05/29 02:11:30, haraken wrote: > > Remove this. Done.
LGTM Sorry it's taking time to land the snapshot GC.
On 2015/05/29 11:09:43, haraken wrote: > LGTM > > Sorry it's taking time to land the snapshot GC. Code LGTM. P.S. I grabbed a trace and I see some odd values, for instance theverge.com shows after some seconds: Size: 1M, estimated live objects size: 27M Is this expected? Doesn't need to be fixed in this CL but would be great to understand why
On 2015/06/23 16:37:37, Primiano Tucci wrote: > On 2015/05/29 11:09:43, haraken wrote: > > LGTM > > > > Sorry it's taking time to land the snapshot GC. > > Code LGTM. > P.S. I grabbed a trace and I see some odd values, for instance http://theverge.com > shows after some seconds: Size: 1M, estimated live objects size: 27M > Is this expected? > Doesn't need to be fixed in this CL but would be great to understand why Please try with oilpan-enable=1 flag. Also the estimated love object size right now is showing both oilpan + partitionAlloc. This is being dicswd by yutak@ very soon.
On 2015/06/23 19:37:11, ssid wrote: > On 2015/06/23 16:37:37, Primiano Tucci wrote: > > On 2015/05/29 11:09:43, haraken wrote: > > > LGTM > > > > > > Sorry it's taking time to land the snapshot GC. > > > > Code LGTM. > > P.S. I grabbed a trace and I see some odd values, for instance > http://theverge.com > > shows after some seconds: Size: 1M, estimated live objects size: 27M > > Is this expected? > > Doesn't need to be fixed in this CL but would be great to understand why > > Please try with oilpan-enable=1 flag. > Also the estimated love object size right now is showing both oilpan + > partitionAlloc. This is being dicswd by yutak@ very soon. is oilpan-enable=1 a temporary thing or the final plan? what is that for? Not sure I understand the partitionalloc + oilpan part. does it mean that, right now, we doublecount stuff? What is the plan there?
On 2015/06/23 21:45:25, Primiano Tucci wrote: > On 2015/06/23 19:37:11, ssid wrote: > > On 2015/06/23 16:37:37, Primiano Tucci wrote: > > > On 2015/05/29 11:09:43, haraken wrote: > > > > LGTM > > > > > > > > Sorry it's taking time to land the snapshot GC. > > > > > > Code LGTM. > > > P.S. I grabbed a trace and I see some odd values, for instance > > http://theverge.com > > > shows after some seconds: Size: 1M, estimated live objects size: 27M > > > Is this expected? > > > Doesn't need to be fixed in this CL but would be great to understand why > > > > Please try with oilpan-enable=1 flag. > > Also the estimated love object size right now is showing both oilpan + > > partitionAlloc. This is being dicswd by yutak@ very soon. > > is oilpan-enable=1 a temporary thing or the final plan? what is that for? > Not sure I understand the partitionalloc + oilpan part. does it mean that, right > now, we doublecount stuff? What is the plan there? enable_oilpan=1 is a temporary thing. Once we move everything to oilpan by default, we can remove the flag. Please ignore the estimated size at the moment. There are a couple of known issues around the calculation and yutak@ is looking at it.
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 Link to the patchset: https://codereview.chromium.org/1149703002/#ps160001 (title: "Rebase and change %zu to %lu.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1149703002/160001
Message was sent while issue was closed.
Committed patchset #8 (id:160001) as https://src.chromium.org/viewvc/blink?view=rev&revision=197731 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
