[tracing] Adding class-wise memory statistics to blink gc memory dumps

[tracing] Adding class-wise memory statistics to blink gc memory dumps

This is the final version of Blink GC dump provider. This CL adds the
class-wise memory statistics after a blink GC to tracing. Since the
class-wise statistics count the same objects counted in the page-wise
snapshot, the duplicates are marked by an ownership edge between
classes->pages.

BUG=490087
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200319

[haraken, 2015-06-23 12:59:47]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2015-06-23 12:59:47]

https://codereview.chromium.org/1203493004/diff/1/Source/platform/heap/Heap.cpp
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1203493004/diff/1/Source/platform/heap/Heap.c...
Source/platform/heap/Heap.cpp:1497: size_t tag =
info.getClassTag(Heap::gcInfo(header->gcInfoIndex()));

header->gcInfoIndex() is available only when the header is not freed. In other
words, this must not be called for a header where header->isFree() is true.

You can move this call after the header->isFree() check.

[ssid, 2015-06-23 19:40:02]

https://codereview.chromium.org/1203493004/diff/1/Source/platform/heap/Heap.cpp
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1203493004/diff/1/Source/platform/heap/Heap.c...
Source/platform/heap/Heap.cpp:1497: size_t tag =
info.getClassTag(Heap::gcInfo(header->gcInfoIndex()));
On 2015/06/23 12:59:47, haraken wrote:
> 
> header->gcInfoIndex() is available only when the header is not freed. In other
> words, this must not be called for a header where header->isFree() is true.
> 
> You can move this call after the header->isFree() check.

Ah, I see. Thanks :)

[haraken, 2015-06-26 05:35:28]

https://codereview.chromium.org/1203493004/diff/40001/Source/platform/heap/He...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1203493004/diff/40001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:1723: info.liveSize[tag] += header->size();

header->size() => header->payloadSize()

header->size() is not valid for large objects.

https://codereview.chromium.org/1203493004/diff/40001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:1728: info.deadSize[tag] += header->size();

Ditto.

https://codereview.chromium.org/1203493004/diff/40001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/1203493004/diff/40001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.h:494: using ClassTagMap = HashMap<const
GCInfo*, size_t>;

How about just using gcInfoIndex for the tag? gcInfoIndex is already a tag for
GCInfo.

[ssid, 2015-06-26 10:10:27]

ssid@chromium.org changed reviewers:
+ primiano@chromium.org

[ssid, 2015-06-26 10:10:28]

I think this CL is ready for revie now. PTAL. Thanks

https://codereview.chromium.org/1203493004/diff/40001/Source/platform/heap/He...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1203493004/diff/40001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:1723: info.liveSize[tag] += header->size();
On 2015/06/26 05:35:28, haraken wrote:
> 
> header->size() => header->payloadSize()
> 
> header->size() is not valid for large objects.

Done.

https://codereview.chromium.org/1203493004/diff/40001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/1203493004/diff/40001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.h:494: using ClassTagMap = HashMap<const
GCInfo*, size_t>;
On 2015/06/26 05:35:28, haraken wrote:
> 
> How about just using gcInfoIndex for the tag? gcInfoIndex is already a tag for
> GCInfo.

Hm, I think it is better to store map of GcInfo* here since everytime
takeSnapshot() method is called it needs to get GcInfo from GcInfoIndex. 
ie, instead of key->m_className i would have to do Heap::gcInfo(key)->className.

This adds table look ups everytime dump is taken.

[haraken, 2015-06-26 13:20:36]

On 2015/06/26 10:10:28, ssid wrote:
> I think this CL is ready for revie now. PTAL. Thanks
> 
>
https://codereview.chromium.org/1203493004/diff/40001/Source/platform/heap/He...
> File Source/platform/heap/Heap.cpp (right):
> 
>
https://codereview.chromium.org/1203493004/diff/40001/Source/platform/heap/He...
> Source/platform/heap/Heap.cpp:1723: info.liveSize[tag] += header->size();
> On 2015/06/26 05:35:28, haraken wrote:
> > 
> > header->size() => header->payloadSize()
> > 
> > header->size() is not valid for large objects.
> 
> Done.
> 
>
https://codereview.chromium.org/1203493004/diff/40001/Source/platform/heap/Th...
> File Source/platform/heap/ThreadState.h (right):
> 
>
https://codereview.chromium.org/1203493004/diff/40001/Source/platform/heap/Th...
> Source/platform/heap/ThreadState.h:494: using ClassTagMap = HashMap<const
> GCInfo*, size_t>;
> On 2015/06/26 05:35:28, haraken wrote:
> > 
> > How about just using gcInfoIndex for the tag? gcInfoIndex is already a tag
for
> > GCInfo.
> 
> Hm, I think it is better to store map of GcInfo* here since everytime
> takeSnapshot() method is called it needs to get GcInfo from GcInfoIndex. 
> ie, instead of key->m_className i would have to do
Heap::gcInfo(key)->className.

Heap::gcInfo(key) should be very fast. It just looks up an array.

[ssid, 2015-06-26 18:52:58]

primiano@ I have added a light and heavy type dump since the class names are big
and the trace becomes very large. Once tracing UI supports light and heavy dumps
this can be enabled. The double counting is again fixed by adding the total
values to the appropriate root dump.

haraken@ If you want to use it in mean while, you could use by changing the type
to HeapSnapshotWithClasses.

This patch is rebased currently on crrev.com/1209293005 since that fix should
land before this.

[haraken, 2015-06-27 07:27:40]

https://codereview.chromium.org/1203493004/diff/80001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/1203493004/diff/80001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.h:493: // Map from gcInfoIndex to a snapshot
class-ids (used as index below).

I'm wondering why you simply can't use gcInfoIndex as a tag. It is guaranteed
that gcInfoIndex returns an index in [0, # of classes actually used in this
execution).

https://codereview.chromium.org/1203493004/diff/80001/Source/platform/heap/Vi...
File Source/platform/heap/Visitor.h (right):

https://codereview.chromium.org/1203493004/diff/80001/Source/platform/heap/Vi...
Source/platform/heap/Visitor.h:397: void setHostInfo(void* object, const String&
name)

Why do we need this?

https://codereview.chromium.org/1203493004/diff/80001/Source/platform/heap/Vi...
Source/platform/heap/Visitor.h:403: inline bool isGlobalMarkingVisitor() const {
return m_isGlobalMarkingVisitor; }

Why do we need this?

[ssid, 2015-08-10 21:20:29]

ssid@chromium.org changed reviewers:
- primiano@chromium.org

[ssid, 2015-08-10 21:20:29]

Fixed this CL after light dumps support is added. PTAL.

[haraken, 2015-08-10 23:54:48]

Thanks for being persistent on this!

LGTM with comments.

https://codereview.chromium.org/1203493004/diff/100001/Source/platform/heap/H...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1203493004/diff/100001/Source/platform/heap/H...
Source/platform/heap/Heap.cpp:1579: size_t tag = header->gcInfoIndex();

tag => gcInfoIndex

https://codereview.chromium.org/1203493004/diff/100001/Source/platform/heap/H...
Source/platform/heap/Heap.cpp:1586: size_t tag = header->gcInfoIndex();

Ditto.

https://codereview.chromium.org/1203493004/diff/100001/Source/platform/heap/H...
Source/platform/heap/Heap.cpp:1787: size_t tag = header->gcInfoIndex();

Ditto.

https://codereview.chromium.org/1203493004/diff/100001/Source/platform/heap/H...
Source/platform/heap/Heap.cpp:1792: info.liveSize[tag] += header->size();

Yeah, it is inconsistent we mix header->size() and header->payloadSize(). I
think we should use header->size() at line 1790 but it's ok to leave it as is in
this CL. I'll fix in a follow-up.

https://codereview.chromium.org/1203493004/diff/100001/Source/platform/heap/T...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1203493004/diff/100001/Source/platform/heap/T...
Source/platform/heap/ThreadState.cpp:384: :
liveCount(Vector<int>(numObjectTypes))

You can just use GCInfoTable::maxIndex instead of passing numObjectTypes.

https://codereview.chromium.org/1203493004/diff/100001/Source/platform/heap/T...
Source/platform/heap/ThreadState.cpp:1426: String threadDumpName =
String::format("blink_gc/thread_%lu", (unsigned long)m_thread);

(unsigned long) => static_cast

https://codereview.chromium.org/1203493004/diff/100001/Source/platform/heap/T...
Source/platform/heap/ThreadState.cpp:1467: if (!GCInfoTable::gcInfoIndex())

This check won't be needed.

https://codereview.chromium.org/1203493004/diff/100001/Source/platform/heap/T...
Source/platform/heap/ThreadState.cpp:1475: String dumpName = classesDumpName +
"/" + Heap::gcInfo(i)->m_className;

We should add a getter for m_className.

[ssid, 2015-08-11 09:03:03]

Made changes / replied to comments. Thanks

https://codereview.chromium.org/1203493004/diff/100001/Source/platform/heap/H...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1203493004/diff/100001/Source/platform/heap/H...
Source/platform/heap/Heap.cpp:1579: size_t tag = header->gcInfoIndex();
On 2015/08/10 23:54:48, haraken wrote:
> 
> tag => gcInfoIndex

Done.

https://codereview.chromium.org/1203493004/diff/100001/Source/platform/heap/H...
Source/platform/heap/Heap.cpp:1787: size_t tag = header->gcInfoIndex();
On 2015/08/10 23:54:48, haraken wrote:
> 
> Ditto.

Done.

https://codereview.chromium.org/1203493004/diff/100001/Source/platform/heap/H...
Source/platform/heap/Heap.cpp:1792: info.liveSize[tag] += header->size();
On 2015/08/10 23:54:48, haraken wrote:
> 
> Yeah, it is inconsistent we mix header->size() and header->payloadSize(). I
> think we should use header->size() at line 1790 but it's ok to leave it as is
in
> this CL. I'll fix in a follow-up.

Sorry, I keep getting confused between these two. I am leaving it as size(),
Thanks.

https://codereview.chromium.org/1203493004/diff/100001/Source/platform/heap/T...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1203493004/diff/100001/Source/platform/heap/T...
Source/platform/heap/ThreadState.cpp:384: :
liveCount(Vector<int>(numObjectTypes))
On 2015/08/10 23:54:48, haraken wrote:
> 
> You can just use GCInfoTable::maxIndex instead of passing numObjectTypes.

um, We will be allocating 4 vectors of size 2^14 everytime, even if there are
only 5 object types. That is why i made it to use object types.

https://codereview.chromium.org/1203493004/diff/100001/Source/platform/heap/T...
Source/platform/heap/ThreadState.cpp:1426: String threadDumpName =
String::format("blink_gc/thread_%lu", (unsigned long)m_thread);
On 2015/08/10 23:54:48, haraken wrote:
> 
> (unsigned long) => static_cast

Done.

https://codereview.chromium.org/1203493004/diff/100001/Source/platform/heap/T...
Source/platform/heap/ThreadState.cpp:1467: if (!GCInfoTable::gcInfoIndex())
On 2015/08/10 23:54:48, haraken wrote:
> 
> This check won't be needed.

I added this to make sure it does not create the default root dumps if there are
not objects dumped. unnecessarily creates 3 dumps. with 0 values. Is it always
non-zero when chrome is running, even without oilpan-enable?

https://codereview.chromium.org/1203493004/diff/100001/Source/platform/heap/T...
Source/platform/heap/ThreadState.cpp:1475: String dumpName = classesDumpName +
"/" + Heap::gcInfo(i)->m_className;
On 2015/08/10 23:54:48, haraken wrote:
> 
> We should add a getter for m_className.

Done.

[haraken, 2015-08-11 11:07:37]

LGTM

https://codereview.chromium.org/1203493004/diff/100001/Source/platform/heap/T...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1203493004/diff/100001/Source/platform/heap/T...
Source/platform/heap/ThreadState.cpp:384: :
liveCount(Vector<int>(numObjectTypes))
On 2015/08/11 09:03:03, ssid wrote:
> On 2015/08/10 23:54:48, haraken wrote:
> > 
> > You can just use GCInfoTable::maxIndex instead of passing numObjectTypes.
> 
> um, We will be allocating 4 vectors of size 2^14 everytime, even if there are
> only 5 object types. That is why i made it to use object types.

ah, makes sense.

https://codereview.chromium.org/1203493004/diff/100001/Source/platform/heap/T...
Source/platform/heap/ThreadState.cpp:1467: if (!GCInfoTable::gcInfoIndex())
On 2015/08/11 09:03:03, ssid wrote:
> On 2015/08/10 23:54:48, haraken wrote:
> > 
> > This check won't be needed.
> 
> I added this to make sure it does not create the default root dumps if there
are
> not objects dumped. unnecessarily creates 3 dumps. with 0 values. Is it always
> non-zero when chrome is running, even without oilpan-enable?

We've already shipped oilpan for a bunch of objects. So it won't happen that
GCInfoTable::gcInfoIndex() is 0.

[ssid, 2015-08-11 14:17:12]

The CQ bit was checked by ssid@chromium.org

[ssid, 2015-08-11 14:17:13]

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/1203493004/#ps160001
(title: "Remove useless check.")

[commit-bot: I haz the power, 2015-08-11 14:17:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1203493004/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1203493004/160001

[commit-bot: I haz the power, 2015-08-11 14:22:17]

Committed patchset #9 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200319