Currently heap snapshots are retrieved aside from CPU profiles (via getProfilerLogLines). We need to align heap snapshots with CPU profiles.
Created attachment 65286 [details] Patch
Comment on attachment 65286 [details] Patch The patch is huge, could you break it down into smaller pieces? Also would be nice to cover this functionality with at least couple of layout tests. WebCore/bindings/v8/ScriptHeapSnapshot.cpp:70 + entry->setString("cons", toWebCoreString(node->GetName())); What does "cons" stand for? Is it "constructor"? If so, please use full name for the field. WebCore/inspector/InspectorProfilerAgent.cpp:70 + , m_nextUserInitiatedHeapSnapshotNumber(1) Shouldn't it be static to be unique among all instances of the InspectorProfilerAgent in the process? WebKit/chromium/src/js/HeapProfilerPanel.js:119 + this._loadProfile(this.profile, profileCallback); We usually call profileCallback.bind(this) instead of using var self = this; in Web Inspector code. WebKit/chromium/src/js/HeapProfilerPanel.js:297 + var self = this; ditto WebKit/chromium/src/js/HeapProfilerPanel.js:333 + var self = this; dtto
Comment on attachment 65286 [details] Patch LayoutTests/platform/chromium-win/http/tests/loading/307-after-303-after-post-expected.txt:127 + inspector/ScriptView.js - didFailLoadingWithError: <NSError domain NSURLErrorDomain, code -6, failing URL "file:///C:/b/slave/webkit-rel/build/src/webkit/Release/resources/inspector/ScriptView.js"> I'd suggest to fix these poor expectations. Test is anyways disabled... WebCore/inspector/front-end/ProfilesPanel.js:390 + if (this._profiles[i].typeId == typeId) === WebCore/inspector/front-end/ProfilesPanel.js:399 + if (this._profiles[i].typeId == profile.typeId ditto WebCore/inspector/front-end/ProfilesPanel.js:400 + && this._profiles[i].uid == profile.uid) { ditto WebKit/chromium/src/js/HeapProfilerPanel.js:35 + WebInspector.HeapSnapshotView = function(parent, profile) Why is this not in WebCore now? r- for this. WebKit/chromium/src/js/HeapProfilerPanel.js:425 + lastComparator = self.snapshotDataGridList.lastComparator; bad indent. WebKit/chromium/src/js/HeapProfilerPanel.js:460 + if (title.indexOf(UserInitiatedProfileName) === 0) if (!title ...) WebKit/chromium/src/js/DevTools.js:37 + devtools.ToolsAgent = function() Please remove ToolsAgent as a whole since it is no longer used. Once you remove it, please remove override of WebInspector.loaded that was used for the agent. Just put Preferences overrides in the anonymous inline function there. WebKit/chromium/src/js/DevTools.js:82 + var oldShow = WebInspector.ProfilesPanel.prototype.show; This code should also be removed as a whole - no need to intercept profiles panel showing anymore.
(In reply to comment #3) > (From update of attachment 65286 [details]) > LayoutTests/platform/chromium-win/http/tests/loading/307-after-303-after-post-expected.txt:127 > + inspector/ScriptView.js - didFailLoadingWithError: <NSError domain NSURLErrorDomain, code -6, failing URL "file:///C:/b/slave/webkit-rel/build/src/webkit/Release/resources/inspector/ScriptView.js"> > I'd suggest to fix these poor expectations. Test is anyways disabled... > Fixed in r66011 > WebCore/inspector/front-end/ProfilesPanel.js:390 > + if (this._profiles[i].typeId == typeId) > === > > WebCore/inspector/front-end/ProfilesPanel.js:399 > + if (this._profiles[i].typeId == profile.typeId > ditto > > WebCore/inspector/front-end/ProfilesPanel.js:400 > + && this._profiles[i].uid == profile.uid) { > ditto > All fixed. > WebKit/chromium/src/js/HeapProfilerPanel.js:35 > + WebInspector.HeapSnapshotView = function(parent, profile) > Why is this not in WebCore now? r- for this. > Moved. > WebKit/chromium/src/js/HeapProfilerPanel.js:425 > + lastComparator = self.snapshotDataGridList.lastComparator; > bad indent. > Fixed. > WebKit/chromium/src/js/HeapProfilerPanel.js:460 > + if (title.indexOf(UserInitiatedProfileName) === 0) > if (!title ...) > Fixed. > WebKit/chromium/src/js/DevTools.js:37 > + devtools.ToolsAgent = function() > Please remove ToolsAgent as a whole since it is no longer used. Once you remove it, please remove override of WebInspector.loaded that was used for the agent. > Just put Preferences overrides in the anonymous inline function there. > Done. > WebKit/chromium/src/js/DevTools.js:82 > + var oldShow = WebInspector.ProfilesPanel.prototype.show; > This code should also be removed as a whole - no need to intercept profiles panel showing anymore. Removing this code is orthogonal to this change. Will do as a separate issue.
(In reply to comment #2) > (From update of attachment 65286 [details]) > The patch is huge, could you break it down into smaller pieces? Also would be nice to cover this functionality with at least couple of layout tests. > > WebCore/bindings/v8/ScriptHeapSnapshot.cpp:70 > + entry->setString("cons", toWebCoreString(node->GetName())); > What does "cons" stand for? Is it "constructor"? If so, please use full name for the field. > Fixed. > WebCore/inspector/InspectorProfilerAgent.cpp:70 > + , m_nextUserInitiatedHeapSnapshotNumber(1) > Shouldn't it be static to be unique among all instances of the InspectorProfilerAgent in the process? > No. These numbers are not UIDs (which are indeed static), but rather numbers displayed on UI. Having snapshots started with "Snapshot 3" will confuse users. > WebKit/chromium/src/js/HeapProfilerPanel.js:119 > + this._loadProfile(this.profile, profileCallback); > We usually call profileCallback.bind(this) instead of using var self = this; in Web Inspector code. > > WebKit/chromium/src/js/HeapProfilerPanel.js:297 > + var self = this; > ditto > > WebKit/chromium/src/js/HeapProfilerPanel.js:333 > + var self = this; > dtto Fixed.
Created attachment 65431 [details] Comments addressed 'cr-linux' bot will be red because the last roll of V8 was reverted, and I'm relying on the updated API. As soon as the next roll will happen, I'll lift up DEPS in chromium port, and commit.
Comment on attachment 65431 [details] Comments addressed I just had some style comments, but I didn't investigate with great detail since this has already worked for some time in Chrome. WebCore/inspector/front-end/HeapSnapshotView.js:81 > + "sizeDelta": { title: WebInspector.UIString("\xb1 Size"), width: "72px", sortable: true } }; What is \xb1? Could there be a comment nearby that it is "±"? Also, a few of these Object Literals use quotes around the keys. I guess that is fine but its not required and we haven't used them elsewhere in the project. WebCore/inspector/front-end/HeapSnapshotView.js:123 > + WebInspector.HeapSnapshotView.prototype = { > + > + get statusBarItems() No newline WebCore/inspector/front-end/HeapSnapshotView.js:264 > + var maxDepth = 2; Could make this "const" since it doesn't change. WebCore/inspector/front-end/HeapSnapshotView.js:389 > + for (var profileEntry in profile.entries) { > + profile.entries[profileEntry].retainers = {}; > + } Braces not needed for single line WebCore/inspector/front-end/HeapSnapshotView.js:401 > + if ((item.constructorName == 'Object' || item.constructorName == 'Array')) { WebCore/inspector/front-end/HeapSnapshotView.js:415 > + if (retainer.constructorName == 'Object' || retainer.constructorName == 'Array') === WebCore/inspector/front-end/HeapSnapshotView.js:596 > + if (type === "CODE_TYPE" || type === "SHARED_FUNCTION_INFO_TYPE" || type === "SCRIPT_TYPE") return "code"; WebCore/inspector/front-end/HeapSnapshotView.js:597 > + if (type === "STRING_TYPE" || type === "HEAP_NUMBER_TYPE" || type.match(/^JS_/) || type.match(/_ARRAY_TYPE$/)) return "data"; Please put the return statement on the next line. WebCore/inspector/front-end/HeapSnapshotView.js:684 > + else > + // Math minus sign, same width as plus. > + return "\u2212"; Here braces are needed, or put the comment on the same line as the return statement. WebCore/inspector/front-end/HeapSnapshotView.js:689 > + showDeltaAsPercent: function(value) { WebCore/inspector/front-end/HeapSnapshotView.js:699 > + getTotalCount: function() { WebCore/inspector/front-end/HeapSnapshotView.js:708 > + getTotalSize: function() { Here are a series of functions where the opening brace should go on the next line.
Created attachment 65440 [details] Style fixes Thanks, Joe! I was believing that 'check-webkit-style' lints .js files, but apparently it's not. I walked over the file and fixed other style issues.
Comment on attachment 65440 [details] Style fixes More nits before you land. New file is too big to review the logic, but given that it is a WebKit -> WebCore move, I think it is fine. I think you'll need to merge with incoming change from Ilya (#44617). WebCore/inspector/front-end/HeapSnapshotView.js:31 + /** nit: no JSDoc in front-end yet. WebCore/inspector/front-end/HeapSnapshotView.js:124 + }; nit: no ; WebCore/inspector/front-end/HeapSnapshotView.js:278 + jumpToFirstSearchResult: WebInspector.CPUProfileView.prototype.jumpToFirstSearchResult, I personally hate this. Please inherit both from single abstract view instead. Can be done in subsequent patch. (Add FIXME here) WebCore/inspector/front-end/inspector.css:3915 + .heap-snapshot-sidebar-tree-item .icon { Please introduce a separate file "heap-profiler.css" instead. Can be done in immediate follow-up. WebKit/chromium/src/js/DevTools.js:41 + // Here and below are overrides to existing WebInspector methods only. This comment no longer applies. Nuke it! WebKit/chromium/src/js/DevTools.js:65 + var oldShow = WebInspector.ProfilesPanel.prototype.show; Put a FIXME telling it should be upstreamed.
Committed http://trac.webkit.org/changeset/66117: 2010-08-26 Mikhail Naganov <mnaganov@chromium.org> Reviewed by Pavel Feldman. Web Inspector: Store heap snapshots in InspectorProfilerAgent. Change the way heap snapshots are transported to Inspector to be aligned with CPU profiles. As a result, the Heap snapshots view of Profiles panel was upstreamed into WebCore. https://bugs.webkit.org/show_bug.cgi?id=44531