Bug 37879 – Web Inspector: support heap graph in Timeline panel

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

[Yury Semikhatsky]

Timeline panel should display JS heap size graph.

[Ilya Tikhonovsky]

Created attachment 54084 [details]
screenshot of used heap graph in Overview pane

[Timothy Hatcher]

I think this is toomuch overloading and the user has no indication/label telling them what this graph is in the background.

I would rather see a 4th row that has a  shorter graph.

[Timothy Hatcher]

Having a 4th row will allow it to have a proper label.

[Ilya Tikhonovsky]

(In reply to comment #2)
> I think this is toomuch overloading and the user has no indication/label
> telling them what this graph is in the background.
> 
> I would rather see a 4th row that has a  shorter graph.

there is very small space for the graph. Right now it is just 60px. 
In any case it is not informative at all.
I think it would be much useful to have the same behavior as in google finance.

[Yury Semikhatsky]

Created attachment 54142 [details]
Screenshot

We were also thinking about adding a tab that would replace three event timeline overviews with one heap graph. This way user would be able to focus on the heap graph and then narrow timeline window to find out what caused heap spikes.

[Timothy Hatcher]

This version is confusing to me. Records do no smothly transition into the the next one, they start and stop. But your graph implies they ramp up and down over time. It should really be steps up and down with no smooth transition. (Make sense?) But steps would look even uglier…

I's prefer seperate rows like it was originally, with gradients that darken/lighten in the bar to show intensity for that period of time.

[Timothy Hatcher]

The design I mentioned was mocked up long ago when the Timeline panel was being designed. It was just never implemented.

See the attachment.

[Timothy Hatcher]

Created attachment 54162 [details]
Original mock up

[Timothy Hatcher]

Ok, so I was really confused. Your mock up uses the same colors for the heap grap as Scripting and Renderting. And you kept the labels for the record types, making me think the graph was matching those labels.

Ignore my idea then, but we should implemet it sometime, with the time summaries.

[Timothy Hatcher]

Why are there two numbers for heap anyway? I never understood this.

[Pavel Feldman]

(In reply to comment #9)
> Ok, so I was really confused. Your mock up uses the same colors for the heap
> grap as Scripting and Renderting. And you kept the labels for the record types,
> making me think the graph was matching those labels.
> 
> Ignore my idea then, but we should implemet it sometime, with the time
> summaries.

This is just a wip patch and screenshot. It is about _optionally_ enabled heap graphs. The idea is that we can switch between overview bars and heap graph there. Colors are random I guess. Should be blue and gray, semitransparent.

(In reply to comment #10)
> Why are there two numbers for heap anyway? I never understood this.

Lower is objects allocated in the heap, higher is memory allocated for the heap. When we get out of memory we do GC and in not successful, we request more memory for the VM.

[Yury Semikhatsky]

Created attachment 54292 [details]
proposed Timeline panel layout with Timelines and Memory tabs

We discussed this a bit and come up with the layout captured on the screenshot. User can switch between memory and cpu view in the timeline overview area. The area itself is stretched to the width of its container and all filters are moved to the toolbar above it.

[Timothy Hatcher]

You have disconnected the summary bar colors and the checkboxes, so you have no clues what the colors mean.

I am not sure why memory has to go in this area of the panel. What if there was a special row (or rows) in the record area that zoom and move with the other records.

You could have a small summary line in the top area too.

[Pavel Feldman]

(In reply to comment #13)
> You have disconnected the summary bar colors and the checkboxes, so you have no
> clues what the colors mean.
> 
> I am not sure why memory has to go in this area of the panel. What if there was
> a special row (or rows) in the record area that zoom and move with the other
> records.
> 
> You could have a small summary line in the top area too.

Mock! Mock!

[Timothy Hatcher]

I'll try later today.

[Yury Semikhatsky]

Created attachment 54568 [details]
Record filters moved to the status bar,  CPU/Memory switch like in Resources panel

Another option: move record filters to the status bar, switch between CPU/Memory view much like Size/Time in Resources panel.

[Timothy Hatcher]

I like this approch.

Except anchor the checkboxes along the divider line, like we anchor the view buttons in the Pofiles panel.

[Yury Semikhatsky]

Created attachment 54804 [details]
patch

[Yury Semikhatsky]

Created attachment 54805 [details]
Screenshot for the patch

[Timothy Hatcher]

What is being shown in the console? Why are there magnifying glasses next to each log?

[Timothy Hatcher]

Comment on attachment 54804 [details]
patch

How will this look in Safari/WebKit where the heap graph isn't supported.

Is it too much to ask for the look to not change if heap isn't supported?

[Pavel Feldman]

(In reply to comment #21)
> (From update of attachment 54804 [details])
> How will this look in Safari/WebKit where the heap graph isn't supported.
> 
> Is it too much to ask for the look to not change if heap isn't supported?

It is not too much, but it would be a through away work. Knowing that JSC folks most likely have this information in hand makes this approach not that appealing.

So other options are:

1) We remove Memory item as a whole when there is no support for heap. The only drawback is the area under the large timeline item. (which is to be occupied soonish)
2) we remove both items to the left, making entire top area an overview. Window resizers become less usable, but new "drag to select" functionality mitigates it.

If you don't like them, we'll support the dual-mode.

[Timothy Hatcher]

Does the patch currently do 1? If not we shouldn't land it until option 1 is done.

[Pavel Feldman]

(In reply to comment #23)
> Does the patch currently do 1? If not we shouldn't land it until option 1 is
> done.

We discussed with Yury that it is necessary, but I am not sure whether it is in the patch already. Do you think it is Ok to land given that it is done? (Yury is on vacations, I might need to finish this for him).

[Yury Semikhatsky]

(In reply to comment #24)
> (In reply to comment #23)
> > Does the patch currently do 1? If not we shouldn't land it until option 1 is
> > done.
> 
> We discussed with Yury that it is necessary, but I am not sure whether it is in
> the patch already. 
It's not in the patch yet. I can implement any of discussed options when I'm back from vacation(I have limited internet connection here). Personally I don't like the option 1) as it would add a useless control(Timelines button) to the overview area.

[Yury Semikhatsky]

Created attachment 55874 [details]
patch

[Yury Semikhatsky]

Created attachment 55875 [details]
Safari screenshot for the patch

[Timothy Hatcher]

You attached the patch again, not the screenshot.

[Yury Semikhatsky]

Created attachment 55877 [details]
Safari screenshot for the patch 

Sorry about that, here is the screenshot

[Pavel Feldman]

Comment on attachment 55874 [details]
patch

No real problems, but numerous nits. Clearing the r flag and looking forward to your fixes.

WebCore/inspector/front-end/Drawer.js:43
 +      this._countItems = document.getElementById("count-items");
itemsCounter?


WebCore/inspector/front-end/Drawer.js:118
 +              var newRight = 25; // margin-right value when the counter is on main status bar
Please declare it as a constant and refer to the style it depends on. Can we actually get rid of it?


WebCore/inspector/front-end/Drawer.js:119
 +              var rightPadding = (oldRight - newRight) - 2;
Same here. Is this a border? Which style does it belong to? Can we get rid?


WebCore/inspector/front-end/Drawer.js:258
 +              this._currentPanelCounters = null;
delete this._currentPanelCounters



WebCore/inspector/front-end/Settings.js:33
 +      heapInfoAvailable: false,
Looks like we have a great hint on how to make this available in jsc. Might be easier to support it there than to fork behavior.

WebCore/inspector/front-end/TimelineOverviewPane.js:94
 +          this._overviewGrid.element.replaceChild(this._overviewGrid.itemsGraphsElement, this._heapGraphElement);
We usually manage cases like this using "hidden" style and leaving DOM structure as is.

WebCore/inspector/front-end/TimelinePanel.js:36
 +      var overviewPanelElement = document.createElement("div");
I think these should no longer have overview prefix in their names.

WebCore/inspector/front-end/TimelineOverviewPane.js:181
 +      updateHeapGraph: function(records)
We might want to extract this into a separate class that will be further used for rendering selected window's heap details.

[Yury Semikhatsky]

Created attachment 56368 [details]
patch

[Yury Semikhatsky]

Created attachment 56369 [details]
Safari screenshot for the patch

[Yury Semikhatsky]

(In reply to comment #30)
> (From update of attachment 55874 [details])
> No real problems, but numerous nits. Clearing the r flag and looking forward to your fixes.
> 
> WebCore/inspector/front-end/Drawer.js:43
>  +      this._countItems = document.getElementById("count-items");
> itemsCounter?
> 
Renamed.

> 
> WebCore/inspector/front-end/Drawer.js:118
>  +              var newRight = 25; // margin-right value when the counter is on main status bar
> Please declare it as a constant and refer to the style it depends on. Can we actually get rid of it?
>
Introduced a constant. I don't see how we can easily get access to the margin value without creating invisible elements and taking their positions.
 
 
> WebCore/inspector/front-end/Drawer.js:119
>  +              var rightPadding = (oldRight - newRight) - 2;
> Same here. Is this a border? Which style does it belong to? Can we get rid?
>
It was left-padding value, I changed style declaration and removed the constant from code.

 

> WebCore/inspector/front-end/Drawer.js:258
>  +              this._currentPanelCounters = null;
> delete this._currentPanelCounters
> 
Done.

 
 
> WebCore/inspector/front-end/Settings.js:33
>  +      heapInfoAvailable: false,
> Looks like we have a great hint on how to make this available in jsc. Might be easier to support it there than to fork behavior.
>
Done. Now that Ilya implemented basic heap info in JSC I can reuse it.

 
> WebCore/inspector/front-end/TimelineOverviewPane.js:94
>  +          this._overviewGrid.element.replaceChild(this._overviewGrid.itemsGraphsElement, this._heapGraphElement);
> We usually manage cases like this using "hidden" style and leaving DOM structure as is.
> 
Done.

> WebCore/inspector/front-end/TimelinePanel.js:36
>  +      var overviewPanelElement = document.createElement("div");
> I think these should no longer have overview prefix in their names.
> 
Done. Renamed to topPane... as was discussed offline.

> WebCore/inspector/front-end/TimelineOverviewPane.js:181
>  +      updateHeapGraph: function(records)
> We might want to extract this into a separate class that will be further used for rendering selected window's heap details.
Extracted class.

[Pavel Feldman]

I find screenshot confusing. Graph is drawn outside the axis.

[Yury Semikhatsky]

Created attachment 56613 [details]
Screenshot with only max heap size in the top left corner

Do you like better when only max heap size is shown in the top left corner without the scale?

[Yury Semikhatsky]

Created attachment 56617 [details]
Safari screenshot with only max heap size in the top left corner

Heap graph has additional 5px offset from time axis above it.

[Pavel Feldman]

For some reason Safari does not have the bounding stroke, only the blueish fill. Is there anything wrong with your canvas code? Also I don't really like the way graph goes to the right - it feels truncated. + Canvas-based rendering of the size looks inconsistent with the rest of the text.

[Yury Semikhatsky]

(In reply to comment #37)
> For some reason Safari does not have the bounding stroke, only the blueish fill. Is there anything wrong with your canvas code? 
I was experimenting with another graph presentation - the one without the grey border, it has more smooth edge compared to the previous screenshots.

> Also I don't really like the way graph goes to the right - it feels truncated. 
We could add a padding between the graph right border and the panel right bound. We'd need to change the time axis in that case. What do you think about it?

> Canvas-based rendering of the size looks inconsistent with the rest of the text.
I'll move it into a div.

[Yury Semikhatsky]

Created attachment 56687 [details]
Safari,max heap size in a div in the top left corner

Moved heap size label into a div.

[Yury Semikhatsky]

Created attachment 56887 [details]
patch

[Yury Semikhatsky]

Committed r60141