Bug 89754 – Web Inspector: add external resources size to the native memory diagram

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

[Yury Semikhatsky]

Some javascript strings and arrays store their data in C++ heap. We should plot them on the diagram.

[Yury Semikhatsky]

Created attachment 148998 [details]
Patch

[Ilya Tikhonovsky]

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.

[Yury Semikhatsky]

Created attachment 149011 [details]
Patch

[Yury Semikhatsky]

(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.

[Ilya Tikhonovsky]

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.

[Yury Semikhatsky]

(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).

[Ilya Tikhonovsky]

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

[Yury Semikhatsky]

Created attachment 149017 [details]
Patch

[Yury Semikhatsky]

(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.

[Early Warning System Bot]

Comment on attachment 149017 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 149017 [details]
Patch

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

[Alexei Filippov]

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

[Gyuyoung Kim]

Comment on attachment 149017 [details]
Patch

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

[Yury Semikhatsky]

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.

[Yury Semikhatsky]

Created attachment 149252 [details]
Patch

[Yury Semikhatsky]

Committed r121144: <http://trac.webkit.org/changeset/121144>