Bug 89754 - Web Inspector: add external resources size to the native memory diagram
Summary: Web Inspector: add external resources size to the native memory diagram
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: 87262
  Show dependency treegraph
 
Reported: 2012-06-22 04:38 PDT by Yury Semikhatsky
Modified: 2012-06-25 02:27 PDT (History)
19 users (show)

See Also:


Attachments
Patch (10.03 KB, patch)
2012-06-22 04:51 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (27.93 KB, patch)
2012-06-22 06:06 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (28.84 KB, patch)
2012-06-22 06:53 PDT, Yury Semikhatsky
no flags Details | Formatted Diff | Diff
Patch (30.11 KB, patch)
2012-06-24 23:54 PDT, Yury Semikhatsky
vsevik: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yury Semikhatsky 2012-06-22 04:38:08 PDT
Some javascript strings and arrays store their data in C++ heap. We should plot them on the diagram.
Comment 1 Yury Semikhatsky 2012-06-22 04:51:22 PDT
Created attachment 148998 [details]
Patch
Comment 2 Ilya Tikhonovsky 2012-06-22 05:02:12 PDT
Comment on attachment 148998 [details]
Patch

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

> Source/WebCore/inspector/DOMWrapperVisitor.h:46
> +    virtual void visitNode(Node*) { }
> +    virtual void visitJSExternalString(WTF::StringImpl*) { }
> +    virtual void visitJSExternalArray(WTF::ArrayBufferView*) { }
>  protected:

I'd expect to see three abstract classes DOMNodeVisitor, JSExternalStringVisitor etc and the single abstract visit method in each with corresponding param.
Comment 3 Yury Semikhatsky 2012-06-22 06:06:54 PDT
Created attachment 149011 [details]
Patch
Comment 4 Yury Semikhatsky 2012-06-22 06:07:40 PDT
(In reply to comment #2)
> (From update of attachment 148998 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148998&action=review
> 
> > Source/WebCore/inspector/DOMWrapperVisitor.h:46
> > +    virtual void visitNode(Node*) { }
> > +    virtual void visitJSExternalString(WTF::StringImpl*) { }
> > +    virtual void visitJSExternalArray(WTF::ArrayBufferView*) { }
> >  protected:
> 
> I'd expect to see three abstract classes DOMNodeVisitor, JSExternalStringVisitor etc and the single abstract visit method in each with corresponding param.

Done. Renamed the file to BindingVisitors.h and split the class into three classes.
Comment 5 Ilya Tikhonovsky 2012-06-22 06:14:12 PDT
Comment on attachment 149011 [details]
Patch

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

> Source/WebCore/inspector/InspectorMemoryAgent.cpp:334
> +        if (m_arrayBuffers.add(buffer).isNewEntry)
> +            m_externalArraySize += buffer->byteLength();
> +    }
> +    virtual void visitJSExternalString(StringImpl* string)
> +    {
> +        int size = stringSize(string);
> +        m_jsExternalStringSize += size;

is it possible to meet with a single string twice? If yes then we have to have a hash set for strings too.
Comment 6 Yury Semikhatsky 2012-06-22 06:21:08 PDT
(In reply to comment #5)
> (From update of attachment 149011 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149011&action=review
> 
> is it possible to meet with a single string twice? If yes then we have to have a hash set for strings too.

It shouldn't be possible as we cache created external strings by StringImpl* key (see StringCache::v8ExternalStringSlow).
Comment 7 Ilya Tikhonovsky 2012-06-22 06:43:27 PDT
Comment on attachment 149011 [details]
Patch

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

nits. Otherwise looks good to me.

> Source/WebCore/bindings/js/ScriptProfiler.h:77
> +    static void visitJSDOMWrappers(NodeWrapperVisitor*) { }

nit: visitNodeWrappers

> Source/WebCore/bindings/js/ScriptProfiler.h:79
> +    static void visitExternalJSStrings(ExternalStringVisitor*) { }
> +    static void visitExternalJSArrays(ExternalArrayVisitor*) { }

visitExternalStrings ?
visitExternalArrays ?

> Source/WebCore/bindings/v8/ScriptProfiler.h:81
> +    static void visitJSDOMWrappers(NodeWrapperVisitor*);

ditto
Comment 8 Yury Semikhatsky 2012-06-22 06:53:50 PDT
Created attachment 149017 [details]
Patch
Comment 9 Yury Semikhatsky 2012-06-22 06:54:12 PDT
(In reply to comment #7)
> (From update of attachment 149011 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149011&action=review
> 
> nits. Otherwise looks good to me.
> 
> > Source/WebCore/bindings/js/ScriptProfiler.h:77
> > +    static void visitJSDOMWrappers(NodeWrapperVisitor*) { }
> 
> nit: visitNodeWrappers
> 
Done.

> > Source/WebCore/bindings/js/ScriptProfiler.h:79
> > +    static void visitExternalJSStrings(ExternalStringVisitor*) { }
> > +    static void visitExternalJSArrays(ExternalArrayVisitor*) { }
> 
> visitExternalStrings ?
> visitExternalArrays ?
> 
Done.

> > Source/WebCore/bindings/v8/ScriptProfiler.h:81
> > +    static void visitJSDOMWrappers(NodeWrapperVisitor*);
> 
> ditto
Done.
Comment 10 Early Warning System Bot 2012-06-22 07:27:55 PDT
Comment on attachment 149017 [details]
Patch

Attachment 149017 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13038386
Comment 11 Early Warning System Bot 2012-06-22 07:34:43 PDT
Comment on attachment 149017 [details]
Patch

Attachment 149017 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13030360
Comment 12 Alexei Filippov 2012-06-22 08:00:31 PDT
Comment on attachment 149017 [details]
Patch

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

lgtm

> Source/WebCore/inspector/InspectorMemoryAgent.cpp:338
> +    int m_externalArraySize;

nit: size_t
Comment 13 Gyuyoung Kim 2012-06-22 08:50:57 PDT
Comment on attachment 149017 [details]
Patch

Attachment 149017 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13085032
Comment 14 Yury Semikhatsky 2012-06-24 23:34:26 PDT
Comment on attachment 149017 [details]
Patch

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

>> Source/WebCore/inspector/InspectorMemoryAgent.cpp:338
>> +    int m_externalArraySize;
> 
> nit: size_t

Done.
Comment 15 Yury Semikhatsky 2012-06-24 23:54:53 PDT
Created attachment 149252 [details]
Patch
Comment 16 Yury Semikhatsky 2012-06-25 02:27:08 PDT
Committed r121144: <http://trac.webkit.org/changeset/121144>