Web Inspector: Request JSON-serialized heap profile.

[Mikhail Naganov]

This is the first step in migrating to new heap profiles.

This change still requests old (aggregated) profiles, but using JSON serialization mechanism.
This makes heap profiling API simple, and makes saving heap snapshots possible.

[Mikhail Naganov]

Created attachment 74661 [details]
patch

[Build Bot]

Attachment 74661 [details] did not build on win:
Build output: http://queues.webkit.org/results/6316008

[Early Warning System Bot]

Attachment 74661 [details] did not build on qt:
Build output: http://queues.webkit.org/results/6411018

[Eric Seidel (no email)]

Attachment 74661 [details] did not build on mac:
Build output: http://queues.webkit.org/results/6342014

[Mikhail Naganov]

Created attachment 74670 [details]
fix WK compilation

[David Levin]

Removed [chromium] from title since this appears to affect more than Chromium files.

[Mikhail Naganov]

David, although formally you're right, in fact this code does work only in Chromium. For JSC, there is only a stub in bindings.

[Pavel Feldman]

Comment on attachment 74670 [details]
fix WK compilation

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

> WebCore/ChangeLog:5
> +        [Chromium] Request JSON-serialized heap profile.

Please use Web Inspector prefix. It would be nice to see more details on the change in the ChangeLog.

> WebCore/bindings/js/ScriptHeapSnapshot.h:44
> +        virtual void WriteChunk(const String& chunk) = 0;

OutputStream's API is fairly conservative: Write, Flush and Close.

> WebCore/bindings/v8/ScriptHeapSnapshot.cpp:56
> +class OutputStreamAdaptor : public v8::OutputStream {

Adapter?

> WebCore/inspector/front-end/HeapSnapshotView.js:38
> +    get done() {

{ on the next line.

> WebCore/inspector/front-end/HeapSnapshotView.js:42
> +    get isElement() {

ditto x 7

> WebCore/inspector/front-end/HeapSnapshotView.js:145
> +        this._nodeTypeOffset = meta.fields.indexOf('type');

This does not sound optimal:

for (var i = 0; i < fields.length; ++i)
  map [fields[i]] = i;

this._nodeNameOffset = map['name'];
this._nodeIdOffset = map['id'];

> WebCore/inspector/front-end/HeapSnapshotView.js:147
> +        this._nodeIdOffset = meta.fields.indexOf('id');

Please use double quotes.

> WebCore/inspector/front-end/HeapSnapshotView.js:435
> +        if (!this._loadedCallbacks[uid]) return;

return on the next line please.

> WebCore/inspector/front-end/HeapSnapshotView.js:443
> +        this._loadedCallbacks[uid].callback(profile);

this._loadedCallbacks[uid](profile); (bind other arguments should you need them).

> WebCore/inspector/front-end/HeapSnapshotView.js:538
> +            if (node.isHidden) {

No { } for single lines.

> WebCore/inspector/front-end/HeapSnapshotView.js:539
> +                result.lowlevels[node.name] = { count: node.instancesCount, size: node.selfSize, type: node.name };

{count

[Mikhail Naganov]

Comment on attachment 74670 [details]
fix WK compilation

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

>> WebCore/ChangeLog:5
>> +        [Chromium] Request JSON-serialized heap profile.
> 
> Please use Web Inspector prefix. It would be nice to see more details on the change in the ChangeLog.

Details provided.

>> WebCore/bindings/js/ScriptHeapSnapshot.h:44
>> +        virtual void WriteChunk(const String& chunk) = 0;
> 
> OutputStream's API is fairly conservative: Write, Flush and Close.

I don't need Flush. Changed to Write and Close.

>> WebCore/bindings/v8/ScriptHeapSnapshot.cpp:56
>> +class OutputStreamAdaptor : public v8::OutputStream {
> 
> Adapter?

Both "Adaptor" and "Adapter" are valid. But as I see, "Adapter" is used more frequently in WK sources. Changing to "Adapter".

>> WebCore/inspector/front-end/HeapSnapshotView.js:38
>> +    get done() {
> 
> { on the next line.

Fixed

>> WebCore/inspector/front-end/HeapSnapshotView.js:42
>> +    get isElement() {
> 
> ditto x 7

Fixed

>> WebCore/inspector/front-end/HeapSnapshotView.js:145
>> +        this._nodeTypeOffset = meta.fields.indexOf('type');
> 
> This does not sound optimal:
> 
> for (var i = 0; i < fields.length; ++i)
>   map [fields[i]] = i;
> 
> this._nodeNameOffset = map['name'];
> this._nodeIdOffset = map['id'];

This array has < 10 elements, and this code runs only once, so using linear search is fine.

>> WebCore/inspector/front-end/HeapSnapshotView.js:147
>> +        this._nodeIdOffset = meta.fields.indexOf('id');
> 
> Please use double quotes.

Done.

>> WebCore/inspector/front-end/HeapSnapshotView.js:435
>> +        if (!this._loadedCallbacks[uid]) return;
> 
> return on the next line please.

Done.

>> WebCore/inspector/front-end/HeapSnapshotView.js:443
>> +        this._loadedCallbacks[uid].callback(profile);
> 
> this._loadedCallbacks[uid](profile); (bind other arguments should you need them).

Fixed.

>> WebCore/inspector/front-end/HeapSnapshotView.js:538
>> +            if (node.isHidden) {
> 
> No { } for single lines.

Fixed.

>> WebCore/inspector/front-end/HeapSnapshotView.js:539
>> +                result.lowlevels[node.name] = { count: node.instancesCount, size: node.selfSize, type: node.name };
> 
> {count

Done.

[Mikhail Naganov]

Created attachment 74786 [details]
Pavel's comments addressed

[Mikhail Naganov]

Created attachment 74788 [details]
Previous patch rebased

[Pavel Feldman]

Comment on attachment 74788 [details]
Previous patch rebased

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

> WebCore/bindings/js/ScriptHeapSnapshot.h:43
> +        virtual ~OutputStream() {}

{ }

> WebCore/bindings/js/ScriptHeapSnapshot.h:53
> +    void writeJSON(OutputStream*) {}

{ }

> WebCore/inspector/front-end/HeapSnapshotView.js:73
> +    get _nameOrIndex()

We don't use getters for private fields.

[Mikhail Naganov]

Comment on attachment 74788 [details]
Previous patch rebased

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

>> WebCore/bindings/js/ScriptHeapSnapshot.h:43
>> +        virtual ~OutputStream() {}
> 
> { }

Fixed

>> WebCore/bindings/js/ScriptHeapSnapshot.h:53
>> +    void writeJSON(OutputStream*) {}
> 
> { }

Fixed

>> WebCore/inspector/front-end/HeapSnapshotView.js:73
>> +    get _nameOrIndex()
> 
> We don't use getters for private fields.

Fixed

[Mikhail Naganov]

Manually committed as https://trac.webkit.org/changeset/72843:


2010-11-29  Mikhail Naganov  <[e-mail]>

        Reviewed by Pavel Feldman.

        WebInspector: Request JSON-serialized heap snapshot from JS engine.
        This simplifies heap snapshots interaction and API. Instead of
        having objects representing snapshot entities, the whole snapshot
        is transferred to WebInspector and parsed there.

        https://bugs.webkit.org/show_bug.cgi?id=49974

        * bindings/js/ScriptHeapSnapshot.h:
        (WebCore::ScriptHeapSnapshot::OutputStream::~OutputStream):
        (WebCore::ScriptHeapSnapshot::~ScriptHeapSnapshot):
        (WebCore::ScriptHeapSnapshot::writeJSON):
        (WebCore::ScriptHeapSnapshot::ScriptHeapSnapshot):
        * bindings/v8/ScriptHeapSnapshot.cpp:
        (WebCore::ScriptHeapSnapshot::writeJSON):
        * bindings/v8/ScriptHeapSnapshot.h:
        (WebCore::ScriptHeapSnapshot::OutputStream::~OutputStream):
        * inspector/Inspector.idl:
        * inspector/InspectorProfilerAgent.cpp:
        (WebCore::InspectorProfilerAgent::getProfile):
        * inspector/front-end/HeapSnapshotView.js:
        (WebInspector.HeapSnapshotEdgesIterator):
        (WebInspector.HeapSnapshotEdgesIterator.prototype.get done):
        (WebInspector.HeapSnapshotEdgesIterator.prototype.get isElement):
        (WebInspector.HeapSnapshotEdgesIterator.prototype.get isHidden):
        (WebInspector.HeapSnapshotEdgesIterator.prototype.get name):
        (WebInspector.HeapSnapshotEdgesIterator.prototype.next):
        (WebInspector.HeapSnapshotEdgesIterator.prototype.get node):
        (WebInspector.HeapSnapshotEdgesIterator.prototype.get nodeIndex):
        (WebInspector.HeapSnapshotEdgesIterator.prototype._getNameOrIndex):
        (WebInspector.HeapSnapshotEdgesIterator.prototype._getType):
        (WebInspector.HeapSnapshotNodeWrapper):
        (WebInspector.HeapSnapshotNodeWrapper.prototype.get edges):
        (WebInspector.HeapSnapshotNodeWrapper.prototype.get edgesCount):
        (WebInspector.HeapSnapshotNodeWrapper.prototype.get instancesCount):
        (WebInspector.HeapSnapshotNodeWrapper.prototype.get isHidden):
        (WebInspector.HeapSnapshotNodeWrapper.prototype.get name):
        (WebInspector.HeapSnapshotNodeWrapper.prototype.get selfSize):
        (WebInspector.HeapSnapshotNodeWrapper.prototype._getName):
        (WebInspector.HeapSnapshotNodeWrapper.prototype._getEdges):
        (WebInspector.HeapSnapshotNodeWrapper.prototype._getType):
        (WebInspector.HeapSnapshot):
        (WebInspector.HeapSnapshot.prototype._init):
        (WebInspector.HeapSnapshot.prototype.get rootEdges):
        (WebInspector.HeapSnapshotView.prototype.snapshotLoaded):
        (WebInspector.HeapSnapshotView.prototype._loadProfile.processLoadedSnapshot):
        (WebInspector.HeapSnapshotView.prototype._loadProfile):
        (WebInspector.HeapSnapshotView.prototype._convertSnapshot):
        (WebInspector.HeapSnapshotView.prototype._prepareProfile.mergeRetainers):
        (WebInspector.HeapSnapshotView.prototype._prepareProfile):
        (WebInspector.HeapSnapshotView.prototype._sortData):
        * inspector/front-end/ProfilesPanel.js:
        (WebInspector.ProfilesPanel):
        (WebInspector.ProfilesPanel.prototype.addHeapSnapshotChunk):
        (WebInspector.ProfilesPanel.prototype.finishHeapSnapshot):
        * inspector/front-end/inspector.js:
        (WebInspector.addHeapSnapshotChunk):
        (WebInspector.finishHeapSnapshot):