Bug 100659 - Memory instrumentation: report actual object address for CachedResourceClients
Summary: Memory instrumentation: report actual object address for CachedResourceClients
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Yury Semikhatsky
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-29 05:34 PDT by Yury Semikhatsky
Modified: 2012-10-30 02:16 PDT (History)
29 users (show)

See Also:

Attachments
Patch (56.02 KB, patch)
2012-10-29 05:43 PDT, Yury Semikhatsky 		no flags Details | Formatted Diff | Diff
Patch (57.10 KB, patch)
2012-10-29 06:27 PDT, Yury Semikhatsky 		webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch (65.70 KB, patch)
2012-10-29 08:28 PDT, Yury Semikhatsky 		no flags Details | Formatted Diff | Diff
Patch (74.13 KB, patch)
2012-10-29 21:45 PDT, Yury Semikhatsky 		no flags Details | Formatted Diff | Diff
Patch (2.62 KB, patch)
2012-10-30 01:57 PDT, Yury Semikhatsky 		apavlov: review+
apavlov: commit-queue-
Details | Formatted Diff | Diff
Show Obsolete (4) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Yury Semikhatsky 2012-10-29 05:34:57 PDT 
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.
Comment 1 Yury Semikhatsky 2012-10-29 05:43:45 PDT 
Created attachment 171212 [details]
Patch
Comment 2 Early Warning System Bot 2012-10-29 05:53:46 PDT 
Comment on attachment 171212 [details]
Patch

Attachment 171212 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14640068
Comment 3 Early Warning System Bot 2012-10-29 05:56:28 PDT 
Comment on attachment 171212 [details]
Patch

Attachment 171212 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14629113
Comment 4 Build Bot 2012-10-29 06:07:14 PDT 
Comment on attachment 171212 [details]
Patch

Attachment 171212 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14618724
Comment 5 Build Bot 2012-10-29 06:09:39 PDT 
Comment on attachment 171212 [details]
Patch

Attachment 171212 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14626152
Comment 6 Yury Semikhatsky 2012-10-29 06:27:04 PDT 
Created attachment 171225 [details]
Patch
Comment 7 Early Warning System Bot 2012-10-29 06:37:11 PDT 
Comment on attachment 171225 [details]
Patch

Attachment 171225 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14630106
Comment 8 Early Warning System Bot 2012-10-29 06:37:46 PDT 
Comment on attachment 171225 [details]
Patch

Attachment 171225 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14631118
Comment 9 Build Bot 2012-10-29 07:01:48 PDT 
Comment on attachment 171225 [details]
Patch

Attachment 171225 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14625161
Comment 10 Ilya Tikhonovsky 2012-10-29 07:06:33 PDT 
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
Comment 11 Yury Semikhatsky 2012-10-29 07:28:46 PDT 
(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.
Comment 12 EFL EWS Bot 2012-10-29 07:36:39 PDT 
Comment on attachment 171225 [details]
Patch

Attachment 171225 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14640103
Comment 13 Build Bot 2012-10-29 07:54:08 PDT 
Comment on attachment 171225 [details]
Patch

Attachment 171225 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14631137
Comment 14 kov's GTK+ EWS bot 2012-10-29 08:09:13 PDT 
Comment on attachment 171225 [details]
Patch

Attachment 171225 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14630139
Comment 15 Yury Semikhatsky 2012-10-29 08:28:40 PDT 
Created attachment 171252 [details]
Patch
Comment 16 Ilya Tikhonovsky 2012-10-29 08:39:39 PDT 
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.
Comment 17 Build Bot 2012-10-29 08:53:19 PDT 
Comment on attachment 171252 [details]
Patch

Attachment 171252 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14630147
Comment 18 EFL EWS Bot 2012-10-29 09:30:30 PDT 
Comment on attachment 171252 [details]
Patch

Attachment 171252 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14640128
Comment 19 kov's GTK+ EWS bot 2012-10-29 09:59:03 PDT 
Comment on attachment 171252 [details]
Patch

Attachment 171252 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14631176
Comment 20 Yury Semikhatsky 2012-10-29 21:45:04 PDT 
Created attachment 171369 [details]
Patch
Comment 21 Yury Semikhatsky 2012-10-29 22:50:21 PDT 
(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.
Comment 22 Build Bot 2012-10-29 23:18:15 PDT 
Comment on attachment 171369 [details]
Patch

Attachment 171369 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14629394
Comment 23 Yury Semikhatsky 2012-10-30 01:57:04 PDT 
Created attachment 171401 [details]
Patch
Comment 24 Yury Semikhatsky 2012-10-30 02:01:50 PDT 
(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".
Comment 25 Yury Semikhatsky 2012-10-30 02:03:35 PDT 
(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
Comment 26 Ilya Tikhonovsky 2012-10-30 02:04:31 PDT 
Comment on attachment 171401 [details]
Patch

lgtm
Comment 27 Alexander Pavlov (apavlov) 2012-10-30 02:09:01 PDT 
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...
Comment 28 Yury Semikhatsky 2012-10-30 02:11:29 PDT 
(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.
Comment 29 Yury Semikhatsky 2012-10-30 02:16:01 PDT 
Committed r132884: <http://trac.webkit.org/changeset/132884>