Bug 89568 – Web Inspector: partially instrument DOM Tree native memory

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

[Ilya Tikhonovsky]

This patch is a part of native memory instrumentation.

It is not possible to do this in a single patch because DOM structure has too many different classes.
The idea is to count the size of each reported object and call reportMemoryUsage if such method was defined in the object's class.
All the instrumented classes will report their type DOM or CSS, not instrumented classes will be counted as Unknown.

the sample test output:

RESULT native-memory-snapshot: DOM= 400 kB
RESULT native-memory-snapshot: DOMTreeUnknown= 46 kB
RESULT native-memory-snapshot: DOMTreeDOM= 337 kB
RESULT native-memory-snapshot: DOMTreeCSS= 15 kB

[Ilya Tikhonovsky]

Created attachment 148556 [details]
Patch

[Ilya Tikhonovsky]

Created attachment 148561 [details]
Patch

[Ilya Tikhonovsky]

(In reply to comment #2)
> Created an attachment (id=148561) [details]
> Patch

Unknown -> Other
GTK & Qt & MSVS build systems were ajusted

[Ilya Tikhonovsky]

*** Bug 55587 has been marked as a duplicate of this bug. ***

[Build Bot]

Comment on attachment 148561 [details]
Patch

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

[Yury Semikhatsky]

Comment on attachment 148561 [details]
Patch

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

> Source/WebCore/dom/ElementAttributeData.h:30
> +#include "InspectorMemoryInstrumentation.h"

We can avoid this include in the headers of instrumented classes by moving ObjectMemoryAggregator out of InspectorMemoryInstrumentation.

> Source/WebCore/dom/Node.h:30
> +#include "InspectorMemoryInstrumentation.h"

You can avoid this include in the headers of instrumented classes by moving ObjectMemoryAggregator out of InspectorMemoryInstrumentation.

[Gustavo Noronha (kov)]

Comment on attachment 148561 [details]
Patch

Attachment 148561 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12999204

[Build Bot]

Comment on attachment 148561 [details]
Patch

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

[Ilya Tikhonovsky]

Created attachment 148573 [details]
build errors fix

[Build Bot]

Comment on attachment 148573 [details]
build errors fix

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

[Ilya Tikhonovsky]

Created attachment 148579 [details]
build errors fix 2

[Build Bot]

Comment on attachment 148579 [details]
build errors fix 2

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

[Andrey Adaikin]

You need to update WebCore.xcodeproj/project.pbxproj

[Andrey Adaikin]

Comment on attachment 148579 [details]
build errors fix 2

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

> Source/WebCore/css/StylePropertySet.h:119
> +    typedef InspectorMemoryInstrumentation::ObjectMemoryAggregator ObjectMemoryAggregator;

FYI InspectorMemoryInstrumentation only exists #if ENABLE(INSPECTOR). This might fail to compile w/o inspector.

> Source/WebCore/inspector/InspectorMemoryInstrumentation.h:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

2012

[Ilya Tikhonovsky]

Comment on attachment 148579 [details]
build errors fix 2

clear r? because I'm not going to land the patch

[Ilya Tikhonovsky]

Created attachment 148747 [details]
Patch

[Ilya Tikhonovsky]

Created attachment 148749 [details]
Patch

[Ilya Tikhonovsky]

(In reply to comment #6)
> (From update of attachment 148561 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148561&action=review
> 
> > Source/WebCore/dom/Node.h:30
> > +#include "InspectorMemoryInstrumentation.h"
> 
> You can avoid this include in the headers of instrumented classes by moving ObjectMemoryAggregator out of InspectorMemoryInstrumentation.

forward declaration is not enough because TreeShared is a header only template class with instrumentation.



in this version I removed typedef from the instrumented classes.
ObjectMemoryAggregator was renamed to InspectorMemoryAggregator.

#if ENABLED(INSPECTOR) was removed because instrumentation code doesn't affect performance and can be reused by a third party component even without other Inspector's code. Also this change will solve the problem with Qt minimal.

[Build Bot]

Comment on attachment 148749 [details]
Patch

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

[Yury Semikhatsky]

Comment on attachment 148749 [details]
Patch

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

> Source/WebCore/inspector/InspectorMemoryInstrumentation.h:94
> +        void reportType(ObjectType type) { m_type = m_type == Other ? type : m_type; }

setType

> Source/WebCore/inspector/InspectorMemoryInstrumentation.h:138
> +typedef InspectorMemoryInstrumentation::InspectorMemoryAggregator InspectorMemoryAggregator;

What is the reason for not moving InspectorMemoryAggregator to the top level?

[Yury Semikhatsky]

Comment on attachment 148749 [details]
Patch

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

> Source/WebCore/inspector/InspectorMemoryAgent.cpp:433
> +        if (node->document()->frame() && m_page != node->document()->frame()->page())

Can node->document() be 0?

> Source/WebCore/inspector/InspectorMemoryAgent.cpp:437
> +        while (rootNode->parentNode())

Please merge this code with its copy above. Also it seems that it's enough here to just store a hash set of visited nodes.

[Yury Semikhatsky]

Comment on attachment 148749 [details]
Patch

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

> Source/WebCore/inspector/InspectorMemoryInstrumentation.h:70
> +            return sizeof(P);

The returned size doesn't seem to be used, the report* methods should be void.

[Alexei Filippov]

Comment on attachment 148749 [details]
Patch

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

> Source/WebCore/inspector/InspectorMemoryInstrumentation.h:48
> +        InspectorMemoryAggregator aggreagtor(this);

nit: typo

> Source/WebCore/inspector/InspectorMemoryInstrumentation.h:77
> +            return sizeof(OwnPtr<P>);

Won't sizeof of OwnPtr get in account in the caller method "return sizeof(*this)" ?

>> Source/WebCore/inspector/InspectorMemoryInstrumentation.h:94
>> +        void reportType(ObjectType type) { m_type = m_type == Other ? type : m_type; }
> 
> setType

Won't this work:
ASSERT(m_type == Other);
m_type = type;

[Ilya Tikhonovsky]

Created attachment 148763 [details]
Patch

[Ilya Tikhonovsky]

(In reply to comments)
> (From update of attachment 148749 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148749&action=review
> 
> > Source/WebCore/inspector/InspectorMemoryInstrumentation.h:94
> > +        void reportType(ObjectType type) { m_type = m_type == Other ? type : m_type; }
> 
> setType

done: was renamed to setObjectType



> 
> > Source/WebCore/inspector/InspectorMemoryInstrumentation.h:138
> > +typedef InspectorMemoryInstrumentation::InspectorMemoryAggregator InspectorMemoryAggregator;
> 
> What is the reason for not moving InspectorMemoryAggregator to the top level?

done



> > Source/WebCore/inspector/InspectorMemoryAgent.cpp:433
> > +        if (node->document()->frame() && m_page != node->document()->frame()->page())
> 
> Can node->document() be 0?

no idea at the moment



> 
> > Source/WebCore/inspector/InspectorMemoryAgent.cpp:437
> > +        while (rootNode->parentNode())
> 
> Please merge this code with its copy above.

the current implementation has almost nothing in common with the copy above.

>  Also it seems that it's enough here to just store a hash set of visited nodes.

this is done automatically by the MemoryInstrumentationImpl::visited method.




> > Source/WebCore/inspector/InspectorMemoryInstrumentation.h:48
> > +        InspectorMemoryAggregator aggreagtor(this);
> 
> nit: typo
> 
> > Source/WebCore/inspector/InspectorMemoryInstrumentation.h:77
> > +            return sizeof(OwnPtr<P>);
> 
> Won't sizeof of OwnPtr get in account in the caller method "return sizeof(*this)" ?

done



> 
> >> Source/WebCore/inspector/InspectorMemoryInstrumentation.h:94
> >> +        void reportType(ObjectType type) { m_type = m_type == Other ? type : m_type; }
> > 
> > setType
> 
> Won't this work:
> ASSERT(m_type == Other);
> m_type = type;

I'd like to save only the first value assigned by the topmost class.



(In reply to comment #22)
> (From update of attachment 148749 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148749&action=review
> 
> > Source/WebCore/inspector/InspectorMemoryInstrumentation.h:70
> > +            return sizeof(P);
> 
> The returned size doesn't seem to be used, the report* methods should be void.

done


Also InspectorMemoryInstrumentation was renamed to MemoryInstrumentation
InspectorMemoryAggregator was renamed to MemoryAggregator
The whole file also was renamed and moved to WebCore/dom folder.

[Build Bot]

Comment on attachment 148763 [details]
Patch

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

[Build Bot]

Comment on attachment 148763 [details]
Patch

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

[Ilya Tikhonovsky]

Created attachment 148792 [details]
build fix for win and mac bots

[Ilya Tikhonovsky]

Created attachment 148793 [details]
build fix for win and mac bots

[Build Bot]

Comment on attachment 148793 [details]
build fix for win and mac bots

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

[Yury Semikhatsky]

(In reply to comment #25)
> (In reply to comments)
> > (From update of attachment 148749 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=148749&action=review
> > 
> > > Source/WebCore/inspector/InspectorMemoryAgent.cpp:433
> > > +        if (node->document()->frame() && m_page != node->document()->frame()->page())
> > 
> > Can node->document() be 0?
> 
> no idea at the moment
> 
Then we probably need to check it here?

[Ilya Tikhonovsky]

Created attachment 148803 [details]
build fix for win

[Build Bot]

Comment on attachment 148803 [details]
build fix for win

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

[Pavel Feldman]

Comment on attachment 148803 [details]
build fix for win

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

> Source/WebCore/dom/MemoryInstrumentation.h:41
> +class MemoryInstrumentation {

What is MemoryInstrumentation? What is its relationship with the MemoryAggregator.

[Ilya Tikhonovsky]

(In reply to comment #34)
> (From update of attachment 148803 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148803&action=review
> 
> > Source/WebCore/dom/MemoryInstrumentation.h:41
> > +class MemoryInstrumentation {
> 
> What is MemoryInstrumentation? What is its relationship with the MemoryAggregator.

It is a class that aggregates information about an object instance.
It is very simple at this moment but it allows us to have single instrumentation function in the each instrumented class. Otherwise we have to have separate functions for reporting size + type of an object and pointers from the object to other objects.

[Ilya Tikhonovsky]

Created attachment 148847 [details]
heavy template code was removed because MSVS2005 can't compile it

[Alexei Filippov]

Comment on attachment 148847 [details]
heavy template code was removed because MSVS2005 can't compile it

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

> Source/WebCore/bindings/js/ScriptWrappable.h:61
> +        return sizeof(*this);

How about adding reportSelfSize to the aggregator and removing return value? It seems to be more uniform to me.

> Source/WebCore/dom/MemoryInstrumentation.h:46
> +        Other = 0,

Why do you need to specify values here?

> Source/WebCore/dom/MemoryInstrumentation.h:65
> +    virtual void countObjectSize(ObjectType, size_t) = 0;

Is there a real need to split the class into two (MemInstr and MemInstrImpl)? A single class would allow us to avoid having virtual calls.

> Source/WebCore/dom/MemoryInstrumentation.h:83
> +    void reportPointer(const P* pointer, MemoryInstrumentation::ObjectType objectType)

Do you really need to pass objectType here? For all the cases I see in this patch it is equal to m_objectType.

> Source/WebCore/dom/MemoryInstrumentation.h:116
> +void MemoryInstrumentation::reportInstrumentedObject(const T& object)

May the object be pointed from somewhere else as well? So does it need to check visited?

[Ilya Tikhonovsky]

Created attachment 149004 [details]
Patch

[Ilya Tikhonovsky]

Created attachment 149010 [details]
Patch

[Ilya Tikhonovsky]

(In reply to comment #37)
> (From update of attachment 148847 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148847&action=review
> 
> > Source/WebCore/bindings/js/ScriptWrappable.h:61
> > +        return sizeof(*this);
> 
> How about adding reportSelfSize to the aggregator and removing return value? It seems to be more uniform to me.

done: reportType was replaced with reportObjectInfo(type, size)



> 
> > Source/WebCore/dom/MemoryInstrumentation.h:46
> > +        Other = 0,
> 
> Why do you need to specify values here?

done


> 
> > Source/WebCore/dom/MemoryInstrumentation.h:65
> > +    virtual void countObjectSize(ObjectType, size_t) = 0;
> 
> Is there a real need to split the class into two (MemInstr and MemInstrImpl)? A single class would allow us to avoid having virtual calls.

I see no performance problems here. Grabbing all Native memory info data takes 1ms for now.
From the other side it allows us to have  self contained header and to hide implementation details from the DOM/CSS etc classes.

> 
> > Source/WebCore/dom/MemoryInstrumentation.h:83
> > +    void reportPointer(const P* pointer, MemoryInstrumentation::ObjectType objectType)
> 
> Do you really need to pass objectType here? For all the cases I see in this patch it is equal to m_objectType.

It wouldn't be always true.

> 
> > Source/WebCore/dom/MemoryInstrumentation.h:116
> > +void MemoryInstrumentation::reportInstrumentedObject(const T& object)
> 
> May the object be pointed from somewhere else as well? So does it need to check visited?

done.

[Alexei Filippov]

Comment on attachment 149004 [details]
Patch

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

> Source/WebCore/inspector/InspectorMemoryAgent.cpp:390
> +        for (int i = 0; i < LastTypeEntry; ++i)

nit: 0 -> Other

> Source/WebCore/inspector/InspectorMemoryAgent.cpp:414
> +        ASSERT(objectType >= 0 && objectType < LastTypeEntry);

nit: 0 -> Other

[Yury Semikhatsky]

Comment on attachment 149010 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        This patch adds MemoryInstrumentation class that counts all visited

Please update the description.

> Source/WebCore/dom/MemoryInstrumentation.h:98
> +    void reportObjectInfo(MemoryInstrumentation::ObjectType objectType, size_t objectSize)

You can make this method a template: 
template<typename T>
void reportObjectInfo(MemoryInstrumentation::ObjectType objectType, T* object)
{
...
    m_objectSize = sizeof(*object);

this way you wouldn't need to call sizeof() everywhere

> Source/WebCore/dom/MemoryInstrumentation.h:120
> +    MemoryObjectInfo aggregator(this);

aggregator -> info

> Source/WebCore/inspector/InspectorMemoryAgent.cpp:429
> +    DOMTreesIterator(Page* page) : m_page(page) { }

missing explicit

[Ilya Tikhonovsky]

Created attachment 149018 [details]
Patch

[Ilya Tikhonovsky]

(In reply to comment #42)
> (From update of attachment 149010 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149010&action=review
> 
> > Source/WebCore/ChangeLog:6
> > +        This patch adds MemoryInstrumentation class that counts all visited
> 
> Please update the description.

done

> 
> > Source/WebCore/dom/MemoryInstrumentation.h:98
> > +    void reportObjectInfo(MemoryInstrumentation::ObjectType objectType, size_t objectSize)
> 
> You can make this method a template: 
> template<typename T>
> void reportObjectInfo(MemoryInstrumentation::ObjectType objectType, T* object)
> {
> ...
>     m_objectSize = sizeof(*object);
> 
> this way you wouldn't need to call sizeof() everywhere

done


> 
> > Source/WebCore/dom/MemoryInstrumentation.h:120
> > +    MemoryObjectInfo aggregator(this);
> 
> aggregator -> info

done


> 
> > Source/WebCore/inspector/InspectorMemoryAgent.cpp:429
> > +    DOMTreesIterator(Page* page) : m_page(page) { }
> 
> missing explicit

done

[Ilya Tikhonovsky]

Committed r121022: <http://trac.webkit.org/changeset/121022>