Bug 100659 – Memory instrumentation: report actual object address for CachedResourceClients

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

[Yury Semikhatsky]

There are several implementations of CachedResourceClients which have multiple parent classes and thus CachedResourceClient* is different from the actual pointer. This can be fixed by adding CachedResourceClient::reportMemoryUsage which would return actual object address.

[Yury Semikhatsky]

Created attachment 171212 [details]
Patch

[Early Warning System Bot]

Comment on attachment 171212 [details]
Patch

Attachment 171212 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14640068

[Early Warning System Bot]

Comment on attachment 171212 [details]
Patch

Attachment 171212 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14629113

[Build Bot]

Comment on attachment 171212 [details]
Patch

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

[Build Bot]

Comment on attachment 171212 [details]
Patch

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

[Yury Semikhatsky]

Created attachment 171225 [details]
Patch

[Early Warning System Bot]

Comment on attachment 171225 [details]
Patch

Attachment 171225 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14630106

[Early Warning System Bot]

Comment on attachment 171225 [details]
Patch

Attachment 171225 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14631118

[Build Bot]

Comment on attachment 171225 [details]
Patch

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

[Ilya Tikhonovsky]

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

> Source/WebCore/css/CachedSVGDocumentReference.h:45
> +        MemoryClassInfo info(memoryObjectInfo, this, WebCoreMemoryTypes::DOM);

What do you think about WebCoreMemoryTypes::SVG = "Page.SVG"

> Source/WebCore/dom/PendingScript.cpp:71
> +    MemoryClassInfo info(memoryObjectInfo, this, WebCoreMemoryTypes::DOM);

What do you think about WebCoreMemoryTypes::Script = "Page.Script"

> Source/WebCore/html/HTMLDocument.h:82
> +    virtual void reportMemoryUsage(MemoryObjectInfo*) const;

OVERRIDE

> Source/WebCore/html/HTMLLinkElement.cpp:401
> +    MemoryClassInfo info(memoryObjectInfo, this, WebCoreMemoryTypes::CSS);

DOM?

> Source/WebCore/html/HTMLLinkElement.h:91
> +    virtual void reportMemoryUsage(MemoryObjectInfo*) const;

OVERRIDE

> Source/WebCore/html/HTMLScriptElement.h:69
> +    virtual void reportMemoryUsage(MemoryObjectInfo*) const;

OVERRIDE

> Source/WebCore/html/parser/HTMLDocumentParser.h:82
> +    virtual void reportMemoryUsage(MemoryObjectInfo*) const;

ditto

> Source/WebCore/rendering/RenderObject.cpp:2843
> +    MemoryClassInfo info(memoryObjectInfo, this, WebCoreMemoryTypes::RenderTreeUsed, 0);

as we discussed offline that it'd be nice to have customAllocated flag on MemoryObjectInfo which indicates that this object was allocated in RenderArena and shouldn't be counted.

> Source/WebCore/svg/SVGFEImageElement.cpp:207
> +    MemoryClassInfo info(memoryObjectInfo, this, WebCoreMemoryTypes::DOM);

SVG?

> Source/WebCore/svg/SVGFontFaceUriElement.cpp:95
> +    MemoryClassInfo info(memoryObjectInfo, this, WebCoreMemoryTypes::DOM);

SVG?

> Source/WebCore/svg/SVGScriptElement.cpp:217
> +    MemoryClassInfo info(memoryObjectInfo, this, WebCoreMemoryTypes::DOM);

SVG

[Yury Semikhatsky]

(In reply to comment #10)
> View in context: https://bugs.webkit.org/attachment.cgi?id=171225&action=review
> 
> > Source/WebCore/css/CachedSVGDocumentReference.h:45
> > +        MemoryClassInfo info(memoryObjectInfo, this, WebCoreMemoryTypes::DOM);
> 
> What do you think about WebCoreMemoryTypes::SVG = "Page.SVG"
> 
Added.

> > Source/WebCore/dom/PendingScript.cpp:71
> > +    MemoryClassInfo info(memoryObjectInfo, this, WebCoreMemoryTypes::DOM);
> 
> What do you think about WebCoreMemoryTypes::Script = "Page.Script"
> 
We don't report scripts separately yet.

> > Source/WebCore/html/HTMLDocument.h:82
> > +    virtual void reportMemoryUsage(MemoryObjectInfo*) const;
> 
> OVERRIDE
> 
Done.

> > Source/WebCore/html/HTMLLinkElement.cpp:401
> > +    MemoryClassInfo info(memoryObjectInfo, this, WebCoreMemoryTypes::CSS);
> 
> DOM?
> 
Done.

> > Source/WebCore/html/HTMLLinkElement.h:91
> > +    virtual void reportMemoryUsage(MemoryObjectInfo*) const;
> 
> OVERRIDE
> 
Done.

> > Source/WebCore/html/HTMLScriptElement.h:69
> > +    virtual void reportMemoryUsage(MemoryObjectInfo*) const;
> 
> OVERRIDE
> 
Done.

> > Source/WebCore/html/parser/HTMLDocumentParser.h:82
> > +    virtual void reportMemoryUsage(MemoryObjectInfo*) const;
> 
> ditto
> 
Done.

> > Source/WebCore/rendering/RenderObject.cpp:2843
> > +    MemoryClassInfo info(memoryObjectInfo, this, WebCoreMemoryTypes::RenderTreeUsed, 0);
> 
> as we discussed offline that it'd be nice to have customAllocated flag on MemoryObjectInfo which indicates that this object was allocated in RenderArena and shouldn't be counted.
> 
Done.

> > Source/WebCore/svg/SVGFEImageElement.cpp:207
> > +    MemoryClassInfo info(memoryObjectInfo, this, WebCoreMemoryTypes::DOM);
> 
> SVG?
> 
Done.

> > Source/WebCore/svg/SVGFontFaceUriElement.cpp:95
> > +    MemoryClassInfo info(memoryObjectInfo, this, WebCoreMemoryTypes::DOM);
> 
> SVG?
> 
Done.

> > Source/WebCore/svg/SVGScriptElement.cpp:217
> > +    MemoryClassInfo info(memoryObjectInfo, this, WebCoreMemoryTypes::DOM);
> 
> SVG
Done.

[EFL EWS Bot]

Comment on attachment 171225 [details]
Patch

Attachment 171225 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14640103

[Build Bot]

Comment on attachment 171225 [details]
Patch

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

[kov's GTK+ EWS bot]

Comment on attachment 171225 [details]
Patch

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

[Yury Semikhatsky]

Created attachment 171252 [details]
Patch

[Ilya Tikhonovsky]

Comment on attachment 171252 [details]
Patch

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

lgtm

> Source/WebCore/rendering/RenderObject.cpp:2843
> +    memoryObjectInfo->setCustomAllocator();

Actually RenderObject allocates in heap in debug mode.

[Build Bot]

Comment on attachment 171252 [details]
Patch

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

[EFL EWS Bot]

Comment on attachment 171252 [details]
Patch

Attachment 171252 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14640128

[kov's GTK+ EWS bot]

Comment on attachment 171252 [details]
Patch

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

[Yury Semikhatsky]

Created attachment 171369 [details]
Patch

[Yury Semikhatsky]

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

CachedResourceClient::reportMemoryUsage is pure virtual function while other methods in the clients have default implementation. It is hard to provide a meaningful default implementation for reportMemoryUsage so I tried to update all existing implementations of the client interface and left the method pure virtual for now. I could change it to report the object as having custom allocator by default which would compile but report no information about the client if the method is not overriden. Let me know if you want me to do the change.

[Build Bot]

Comment on attachment 171369 [details]
Patch

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

[Yury Semikhatsky]

Created attachment 171401 [details]
Patch

[Yury Semikhatsky]

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

After offline discussion I changed the approach to simply skip CachedResourceClients. Otherwise we would try to report uncontrolled set of clients and their implementation details through the abstract interface. If we want to report the client's memory usage we should do this through the client's owner instead. General approach to the memory graph traversal should sound like "report memory usage only for objects owned by the current one".

[Yury Semikhatsky]

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

This change reduces number of reported objects that are not allocated by the memory allocator from >350 to 4 on Chromium build and nytimes.com

[Ilya Tikhonovsky]

Comment on attachment 171401 [details]
Patch

lgtm

[Alexander Pavlov (apavlov)]

Comment on attachment 171401 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Skiped pointers to objects that are not allocated on the heap directly.

Skiped -> Skipped

> Source/WebCore/ChangeLog:11
> +        insrumentation with those allocated by the memory allocator. Latter set should

The latter set...

[Yury Semikhatsky]

(In reply to comment #27)
> (From update of attachment 171401 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171401&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Skiped pointers to objects that are not allocated on the heap directly.
> 
> Skiped -> Skipped
> 
Done.

> > Source/WebCore/ChangeLog:11
> > +        insrumentation with those allocated by the memory allocator. Latter set should
> 
> The latter set...
Done.

[Yury Semikhatsky]

Committed r132884: <http://trac.webkit.org/changeset/132884>