Bug 44531 – Web Inspector: Store heap snapshots in InspectorProfilerAgent

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

[Mikhail Naganov]

Currently heap snapshots are retrieved aside from CPU profiles (via getProfilerLogLines). We need to align heap snapshots with CPU profiles.

[Mikhail Naganov]

Created attachment 65286 [details]
Patch

[Yury Semikhatsky]

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

[Pavel Feldman]

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.

[Mikhail Naganov]

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

[Mikhail Naganov]

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

[Mikhail Naganov]

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.

[Joseph Pecoraro]

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.

[Mikhail Naganov]

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.

[Pavel Feldman]

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.

[Mikhail Naganov]

Committed http://trac.webkit.org/changeset/66117:


2010-08-26  Mikhail Naganov  <[e-mail]>

        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