Timeline panel should display JS heap size graph.
Created attachment 54084 [details] screenshot of used heap graph in Overview pane
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.
Having a 4th row will allow it to have a proper label.
(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.
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.
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.
The design I mentioned was mocked up long ago when the Timeline panel was being designed. It was just never implemented. See the attachment.
Created attachment 54162 [details] Original mock up
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.
Why are there two numbers for heap anyway? I never understood this.
(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.
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.
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.
(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!
I'll try later today.
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.
I like this approch. Except anchor the checkboxes along the divider line, like we anchor the view buttons in the Pofiles panel.
Created attachment 54804 [details] patch
Created attachment 54805 [details] Screenshot for the patch
What is being shown in the console? Why are there magnifying glasses next to each log?
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?
(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.
Does the patch currently do 1? If not we shouldn't land it until option 1 is done.
(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).
(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.
Created attachment 55874 [details] patch
Created attachment 55875 [details] Safari screenshot for the patch
You attached the patch again, not the screenshot.
Created attachment 55877 [details] Safari screenshot for the patch Sorry about that, here is the screenshot
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.
Created attachment 56368 [details] patch
Created attachment 56369 [details] Safari screenshot for the patch
(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.
I find screenshot confusing. Graph is drawn outside the axis.
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?
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.
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.
(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.
Created attachment 56687 [details] Safari,max heap size in a div in the top left corner Moved heap size label into a div.
Created attachment 56887 [details] patch
Committed r60141