Summary: | Web Inspector: Request JSON-serialized heap profile. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mikhail Naganov <mnaganov> | ||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | apavlov, buildbot, bweinstein, eric, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit-ews, yurys | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Mikhail Naganov
2010-11-23 07:34:38 PST
Created attachment 74661 [details]
patch
Attachment 74661 [details] did not build on win: Build output: http://queues.webkit.org/results/6316008 Attachment 74661 [details] did not build on qt: Build output: http://queues.webkit.org/results/6411018 Attachment 74661 [details] did not build on mac: Build output: http://queues.webkit.org/results/6342014 Created attachment 74670 [details]
fix WK compilation
Removed [chromium] from title since this appears to affect more than Chromium files. David, although formally you're right, in fact this code does work only in Chromium. For JSC, there is only a stub in bindings. 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 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. Created attachment 74786 [details]
Pavel's comments addressed
Created attachment 74788 [details]
Previous patch rebased
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. 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 Manually committed as https://trac.webkit.org/changeset/72843: 2010-11-29 Mikhail Naganov <mnaganov@chromium.org> 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): |