Bug 103232 – Web Inspector: NMI introduce heap graph recording API

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Ilya Tikhonovsky]

Currently we have a plain memory usage statistic based on types.
Unfortunately this 'type' to 'size' information couldn't help to web developers.
They need an information about instances and owners.

As example even about:blank page has ~5MB of Page.Image.
This fact confuses the developers. Actually this image is a part of Inspector backend
because it creates and uses a canvas in InspectorOverlay.

After long offline discussion we came to the conclusion that we need to record a heap graph.

We need to have 2 methods for that. reportNode and reportEdge.
But due to c++ multiple inheritance we need a helper method reportAssociation which associates a pointer to parent class with pointer to descendant class.

patch to follow.

[Ilya Tikhonovsky]

Created attachment 175966 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 175966 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175966&action=review

Let's split the patch into smaller parts.

> Source/WTF/ChangeLog:8
> +        reportNode, reportEdge and reportAssociation were added to the client API

There is no reportAssociation.

> Source/WTF/ChangeLog:10
> +        InstrumentedPointerBase and InstrumentedPointer<T> wer renamed to PointerBase

wer-> were

> Source/WTF/ChangeLog:13
> +        Pointer<T>::m_pointer was upstreamed to PointerBase because it needs to PointerBase::process method.

it needs to PointerBase::process method ->  PointerBase::process uses it

[Build Bot]

Comment on attachment 175966 [details]
Patch

Attachment 175966 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14981638

[Build Bot]

Comment on attachment 175966 [details]
Patch

Attachment 175966 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14992601

[Yury Semikhatsky]

Created attachment 177282 [details]
WIP

[Ilya Tikhonovsky]

Comment on attachment 177282 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=177282&action=review

> Source/WTF/wtf/MemoryInstrumentation.h:45
> +class String;

unnecessary line

> Source/WTF/wtf/MemoryInstrumentation.h:169
> +    template<typename T> void addObjectImpl(const T* const&, MemoryObjectInfo*, MemoryOwningType, const char* name);

I think it should be called edgeName.

[Build Bot]

Comment on attachment 177282 [details]
WIP

Attachment 177282 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15120419

[Build Bot]

Comment on attachment 177282 [details]
WIP

Attachment 177282 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15132023

[Ilya Tikhonovsky]

Comment on attachment 177282 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=177282&action=review

> Source/WTF/wtf/MemoryInstrumentation.h:63
> +    virtual void reportHeapObject(const MemoryObjectInfo&) { }

I prefer reportNode method name.

> Source/WTF/wtf/MemoryInstrumentation.h:64
> +    virtual void reportEdge(const void* source, const void* target, const char* name) { }

looks like we need a method for leafs: reportNodeLeaf
which will be used for reporting private buffers with no pointer. 
it is required because we have no pointer and have to report the size together with edge.

[Yury Semikhatsky]

Created attachment 177371 [details]
WIP

[Yury Semikhatsky]

(In reply to comment #9)
> (From update of attachment 177282 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177282&action=review
> 
> > Source/WTF/wtf/MemoryInstrumentation.h:63
> > +    virtual void reportHeapObject(const MemoryObjectInfo&) { }
> 
> I prefer reportNode method name.
> 
Done.

> > Source/WTF/wtf/MemoryInstrumentation.h:64
> > +    virtual void reportEdge(const void* source, const void* target, const char* name) { }
> 
> looks like we need a method for leafs: reportNodeLeaf
> which will be used for reporting private buffers with no pointer. 
> it is required because we have no pointer and have to report the size together with edge.
Added reportLeaf for that.

[Ilya Tikhonovsky]

Comment on attachment 177371 [details]
WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=177371&action=review

> Source/WTF/wtf/MemoryInstrumentation.cpp:115
> +    m_memoryInstrumentation->addRawBuffer(m_memoryObjectInfo->reportedPointer(), buffer, m_objectType, size);

I think we need a node name argument here with default value 0

> Source/WTF/wtf/MemoryInstrumentation.cpp:125
> +    m_memoryInstrumentation->reportLinkToBuffer(m_memoryObjectInfo->reportedPointer(), 0, ownerObjectType, size);

ditto

> Source/WTF/wtf/MemoryInstrumentation.h:45
> +class String;

unnecessary forward declaration

> Source/WTF/wtf/MemoryInstrumentation.h:146
> +    template<typename T> void addObject(const T& t, MemoryObjectInfo* ownerObjectInfo, const char* name) { OwningTraits<T>::addObject(this, t, ownerObjectInfo, name); }

name -> edgeName

[Yury Semikhatsky]

Created attachment 177380 [details]
Patch

[Yury Semikhatsky]

(In reply to comment #12)
> (From update of attachment 177371 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177371&action=review
> 
> > Source/WTF/wtf/MemoryInstrumentation.cpp:115
> > +    m_memoryInstrumentation->addRawBuffer(m_memoryObjectInfo->reportedPointer(), buffer, m_objectType, size);
> 
> I think we need a node name argument here with default value 0
> 
Done.

> > Source/WTF/wtf/MemoryInstrumentation.cpp:125
> > +    m_memoryInstrumentation->reportLinkToBuffer(m_memoryObjectInfo->reportedPointer(), 0, ownerObjectType, size);
> 
> ditto
> 
Done.

> > Source/WTF/wtf/MemoryInstrumentation.h:45
> > +class String;
> 
> unnecessary forward declaration
> 
Done.

> > Source/WTF/wtf/MemoryInstrumentation.h:146
> > +    template<typename T> void addObject(const T& t, MemoryObjectInfo* ownerObjectInfo, const char* name) { OwningTraits<T>::addObject(this, t, ownerObjectInfo, name); }
> 
> name -> edgeName
Done.

[Ilya Tikhonovsky]

Comment on attachment 177380 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=177380&action=review

> Source/WTF/wtf/MemoryInstrumentation.cpp:71
> +    memoryObjectInfo.reportObjectInfo(buffer, ownerObjectType, size);

What about assigning nodeName?

[Build Bot]

Comment on attachment 177380 [details]
Patch

Attachment 177380 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15135001

[Ilya Tikhonovsky]

Comment on attachment 177380 [details]
Patch

otherwise looks good

[Yury Semikhatsky]

Created attachment 177388 [details]
Patch

[Build Bot]

Comment on attachment 177388 [details]
Patch

Attachment 177388 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15098851

[Yury Semikhatsky]

Committed r136563: <http://trac.webkit.org/changeset/136563>