Bug 28429 – Adding Heap profiler page to Web Inspector

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

[Mikhail Naganov]

Created attachment 35041 [details]
UI snapshot

Hi Kevin and Timothy,

Recently I've added an initial version of Heap profiler page to Chromium. UI snapshots are attached. Information needed for displaying heap statistics is gathered by traversing the heap. Basically, it consists of such rows: "<item name>, <instances count>, <instances size>".

If you're interested, let's discuss upstreaming this page to WebKit and wiring with JSC. I need your feedback on UI, and hints on how is it possible to gather in JSC the data needed.

[Timothy Hatcher]

Looks neat! I think the UI makes sense. I think we should put the summary graph at the bottom of view instead of the top, more like iTunes. It is only at the top of the Resources panel to line up with the 2 graph items in the sidebar.

Also I don't think we need a seperate panel for this in the Web Inspector. The Profiles panel should be the home for this, and just have a button to take a JS profile or a JS heap profile. In the future we would like to add more profiling capabilities to the Profiles panel, and I think this fits.

What do you think?

[Timothy Hatcher]

What falls under Other?

[Mikhail Naganov]

I like the idea of placing heap profiles on the "Profiles" tab. We can make two collapsible sections on the side pane for these two kinds of profiles.

Under "Other" category I put VM's internal stuff: things that are needed for embedding into browser and debugger data.

I will put updated UI snapshots tomorrow.

[Timothy Hatcher]

Another idea regarding comparing profiles. I was working on a prototype that never landed that would let you compare JS profiles. And I was going to do that just by multiple selection in the sidebar. Select one profile then the command-click (or equivlant) the next profile and it show a comparison view of the the two.

The popup button you added can get confusing and large if you make many heap profiles. My sugesstion would let you use the normal profile selection UI.

[Mikhail Naganov]

Popup button is surely non-scalable, but it's discoverable and clearly shows the current state.

Drag'n'dropping profiles is less discoverable. I think, we will need some hint for users. It also lacks scalability. Imagine if I want to compare snapshot 1 with snapshot 30, which is deep down below and need to be scrolled to. Perhaps, a context menu would be a better choice?

[Mikhail Naganov]

Created attachment 35121 [details]
Refined UI

Timothy,

Here is a refined version of UI. CPU and Heap profiles are merged together an can be filtered a la Resources panel. I know you talked with Pavel about using context menu for selecting profiles to compare, I think it's a better solution than dragging profiles on each other.

P.S. I can't draw good icons for buttons, relying on you.

[Timothy Hatcher]

I didn't mention dragging to compare, I was saying have multiple selection in the sidebar be the way to select the two profiles to compare.

I think the new UI looks better. Can you move the shadow to the top of the gray area, that way it matches  iTunes?

Also, I don't think the "Compared to Heap Profile 1" text is appropiate in the scope bar corner, since that bar is a global thing and not specific to the current profile view.

Why is the Boolean row double height? Also "(no_constructor)" should use a speace instead of an underscore.

[Mikhail Naganov]

Created attachment 35141 [details]
Refined UI, 3rd ed.

OK Timothy, I've fixed the mock after your comments. Sorry, I misunderstood your words about selecting profiles to compare.

As for displaying current base snapshot, here are two ideas. The first (upper) is to display it in the status bar of the view. The second one (lower, proposed by Pavel) is to have an additional bar appearing above the table.

BTW, why this topic is now secure? Is it due to some policy?

[Timothy Hatcher]

Not sure why I got marked, I made it public again.

I like the status bar version, takes up less space (good for Docked mode.)

[Pavel Feldman]

Created attachment 35142 [details]
Chooser at top, no filters

I think one should be able to choose profile in place on the "Compare to". My original proposal was having no top bar, so it was not taking that much space. The idea was that it is easy to notice that you are in a 'diff' mode and it is obvious what you are diffing against.

[Timothy Hatcher]

What does this look like in normal mode when not comparing?

[Mikhail Naganov]

Created attachment 38566 [details]
Proposed patch to start adding Heap profiler UI behind a flag

Timothy, to be sure that we're on the same page, here is a patch that adds some needed UI elements to Profiler page behind a flag. When the functionality needed will be exposed by JSC, all branches will be removed.

I decided that instead of using a filter as on Resources tab, it is more convenient to use groups on the sidebar (as on the Storage tab.) This approach uses screen estate more effectively, as having a filter pane for just two types of profiles is a waste of space.

See the screenshots of UI in the next attachment.

Some notes:
- although side pane groups are collapsible, this isn't reflected in UI (on Storage tab too), I think this needs to be fixed separately;
- instead of "Heap profiles" I'm using the term "Heap snapshot", as it is taken instantly, as opposed to CPU profile;
- we need a glyph image for "Take heap snapshot" button, I'm not a designed, so if you could draw it, this would be great.

[Timothy Hatcher]

I think groups makes sense for the reasons you mentioned. Though they are not intentionally collapsable, that is just a bug.

[Mikhail Naganov]

Created attachment 38568 [details]
Here is how UI looks like with the patch

[Timothy Hatcher]

Comment on attachment 38566 [details]
Proposed patch to start adding Heap profiler UI behind a flag


> +    if (Preferences.heapProfilerPresents) {

This pref should be named heapProfilerPresent.

> +        this.snapshotsListTreeElement = new WebInspector.SidebarSectionTreeElement(WebInspector.UIString("HEAP SNAPSHOTS"), {}, true);

I think you can just use null in place of the {} if you don't have a represented object.

> +        if (this.snapshotsListTreeElement) this.snapshotsListTreeElement.removeChildren();

This should be on two lines to match our style guidelines.

> +            if (this.snapshotButton) this.snapshotButton.visible = true;

Ditto.

> +            if (this.snapshotButton) this.snapshotButton.visible = false;

Ditto.

> +.heap-snapshot-status-bar-item .glyph {
> +    -webkit-mask-image: url(Images/focusButtonGlyph.png);
> +}
> +

Can you add a FIXME comment about need a new image here.

[Mikhail Naganov]

Created attachment 38602 [details]
Patch, comments addressed

Timothy, I've addressed your comments.

I also realized that I can hide Heap profiler-related UI items instead of not creating them, this had eliminated some of branches.

[Pavel Feldman]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/inspector/front-end/ProfilesPanel.js
	M	WebCore/inspector/front-end/inspector.css
	M	WebCore/inspector/front-end/inspector.js
Committed r47821