[tracing] Heap dump UI overhaul

[tracing] Heap dump UI overhaul

This patch modifies the "Heap details" pane by replacing the existing
nested table view with a more suitable explorer-like view. See the
tracking bug for more details.

BUG=catapult:#2750

Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/384efea3161c2eee6da51f2cf545f8a51284127e

[petrcermak, 2016-09-16 09:17:47]

Patchset #1 (id:1) has been deleted

[petrcermak, 2016-09-16 09:19:45]

petrcermak@chromium.org changed reviewers:
+ primiano@chromium.org

[petrcermak, 2016-09-16 09:19:46]

PTAL. Before adding tests, I want to check that you're happy with the overall
design ;-)

Thanks,
Petr

[Dmitry Skiba, 2016-09-16 19:58:17]

dskiba@google.com changed reviewers:
+ dskiba@chromium.org, dskiba@google.com

[Dmitry Skiba, 2016-09-16 19:58:17]

Issues that I found:

1. Bug: scrolling position of 'Heap Details' pane is reset when 'Current path'
is changed.
Scroll the whole section that contains 'Overview' and 'Heap Details' panes down
(to see more heap details on a MacBook for example). Double-clicking anything in
'break down' right subpane resets scrolls all the way to the top.

1.1. Bug: sorting in the "break down further" pane is also reset when anything
is double clicked.

2. Bug: width of 'Stack frame' column in 'Break selected node further' pane is
not sufficient to show thread names without breaking on the second line.
Has something to do with spaces (thread names are of form [Thread: bla]),
because switching to "Object types" correctly resizes the column, and no type
takes two lines.

3. Broken workflow: no keyboard navigation. In the old UI I could sort by size
and then just use the right key to expand top elements. That allowed to quickly
go all the way down to interesting places (skipping all those DoWork, RunTask,
etc.). Now right key doesn't do anything in the right subpane. It should break
down the selected element, just like the double clicking does. Wiring left key
to "go up" and undo the last right key seems logical too.

5. Idea: can we make 'Overview' and 'Component details' panes that sit on top of
'Heap details' collapsible? You don't really need them if you are digging the
heap.

6. Bug: clicking items in 'Current path' always fills right 'break down' subpane
with stack frames, even though 'Object type' is selected there. Selecting 'Stack
frame' and then 'Object type' correctly shows object types.

7. Bug: don't hide 'Stack frames' if there are no frames at this level. Clicking
the last frame in a trace (usually InsertAllocation) removes 'Stack frames'
option from 'break down' subpane and switches to 'Object type'. I would prefer
it to just show empty pane. Switching to object type is confusing.

8. Broken workflow: imagine that function f(), responsible for 4 MiB, calls a()
and b(), responsible for 1 and 3 MiB. In the old UI I could go all the way down
to a() path, and then go all the way down to b() path, and end up with two paths
simultaneously opened. Sometimes that's useful. Expansion state was also
preserved when expanding/collapsing, so I could collapse b(), dig into a(), and
then click on b() to expand it to its pre-collapsed state (i.e. with all
subnodes expanded).
In the new UI I see f -> a -> ... path in the left subpane. I don't see b(), but
I see a drop in size between f() and a(), so something must be there. I click on
f(), find that there is b() (that's not easy because a() is not highlighted
there), double-click on b(), and lose everything about a() that I had.
One solution I can think of is to have right subpane to memorize the previously
expanded paths. I.e. if I dug into a(), then clicked on f in the left subpane,
double clicked on b(), played with it, then returned to f, I would see two
highlighed entries: one for b() (because it's currently selected) and one for
a() (because it was selected before). Clicking (or right-keying) on a() would
restore everything I had before I clicked to b(), including state of all
subframes of a().

9. Bug: as was mentioned in #8, selecting a frame in the left subpane should
highlight currently broken down frame in the right subpane.

10. Also, the new UI feels a little slower when enough depth is expanded.

[Dmitry Skiba, 2016-09-19 21:20:00]

Couple of more points:

11. Heavy modes are way faster, wow!

12. 'Count' column is not displayed at all. BTW, previously we also had 'Average
size' - that was useless IMHO because almost always allocations are of different
sizes, and it ended up with some strange value like 20.3 bytes.

[petrcermak, 2016-09-22 18:02:53]

Thanks a lot for the detailed feedback!

> 1. Bug: scrolling position of 'Heap Details' pane is reset when 'Current path'
> is changed.
> Scroll the whole section that contains 'Overview' and 'Heap Details' panes
down
> (to see more heap details on a MacBook for example). Double-clicking anything
in
> 'break down' right subpane resets scrolls all the way to the top.

I suspect you are referring to the following scenario:

  * The right sub-pane contains N rows.
  * You scroll all the way down.
  * You double click on a row.
  * The row has M << N children, so the right sub-pane is not as tall anymore
    and, therefore, the whole analysis view scrolls up (so the bottom of the
    right sub-pane is now aligned with the bottom of the window).

If this is the case, then the above is WAI (although I agree that it's not
ideal). This is how scrolling works in Chrome. As far as I'm aware, it doesn't
support any kind of "temporary overscroll" like some file explorers do. There
are two ways around this:

  A) [easy but sub-optimal] Give the heap details pane a fixed (minimum) height.
     However, this would lead to unnecessary scroll space.
  B) [optimal but non-trivial] Implement the "temporary overscroll" behavior
     above as a Polymer element.

> 1.1. Bug: sorting in the "break down further" pane is also reset when anything
> is double clicked.

This should be relatively straightforward to do. Would you like to keep the
same sort order also when switching between "Stack frames" and "Object types"?

> 2. Bug: width of 'Stack frame' column in 'Break selected node further' pane is
> not sufficient to show thread names without breaking on the second line.
> Has something to do with spaces (thread names are of form [Thread: bla]),
> because switching to "Object types" correctly resizes the column, and no type
> takes two lines.

Right now, the width of the first column is fixed at 200 pixels, so the
'Stack frame' behavior is actually WAI. It's a little tricky to figure out what
the right solution is here. Clearly, the first column cannot keep expanding
forever---it has to break lines at some point so that the other columns would
be visible. As far as I know, HTML, unfortunately, doesn't provide a simple way
to create "adaptive" tables. Hence, we are left with the following options:

  1. Leave as is.
  2. Change the width of the first column to be in % (instead of pixels). This
     will actually require a little bit of work, but shouldn't be too difficult.
  3. Create a heuristic for setting the width of the first column. We could
     take the longest displayed stack frame/object type and use it to guess the
     optimal column width. This is a bit of a hack that might not work well due
     to different widths of characters, so I would rather not go this way.
  4. Add support for resizable columns to Trace Viewer. In my opinion, this
     would be the ideal solution. However, it's a full-time project for several
     weeks.

Correct me if I'm wrong, but this seems like a relatively minor bug to me. The
UI doesn't look as nice, but its usage isn't hindered its usage in any way.
Therefore, I'm going to focus on other issues and leave this to someone else.

> 3. Broken workflow: no keyboard navigation. In the old UI I could sort by size
> and then just use the right key to expand top elements. That allowed to
quickly
> go all the way down to interesting places (skipping all those DoWork, RunTask,
> etc.). Now right key doesn't do anything in the right subpane. It should break
> down the selected element, just like the double clicking does. Wiring left key
> to "go up" and undo the last right key seems logical too.

Good point. You can enter a node by pressing "Enter". I'll try to fix the
focusing logic tomorrow or next week.


> 5. Idea: can we make 'Overview' and 'Component details' panes that sit on top
of
> 'Heap details' collapsible? You don't really need them if you are digging the
> heap.

Definitely. Let's do it in a separate patch. I might not have enough time for
this, so someone else might have to do this (should be straightforward).

> 6. Bug: clicking items in 'Current path' always fills right 'break down'
subpane
> with stack frames, even though 'Object type' is selected there. Selecting
'Stack
> frame' and then 'Object type' correctly shows object types.

Done.

> 7. Bug: don't hide 'Stack frames' if there are no frames at this level.
Clicking
> the last frame in a trace (usually InsertAllocation) removes 'Stack frames'
> option from 'break down' subpane and switches to 'Object type'. I would prefer
> it to just show empty pane. Switching to object type is confusing.

Done.

> 8. Broken workflow: imagine that function f(), responsible for 4 MiB, calls
a()
> and b(), responsible for 1 and 3 MiB. In the old UI I could go all the way
down
> to a() path, and then go all the way down to b() path, and end up with two
paths
> simultaneously opened. Sometimes that's useful. Expansion state was also
> preserved when expanding/collapsing, so I could collapse b(), dig into a(),
and
> then click on b() to expand it to its pre-collapsed state (i.e. with all
> subnodes expanded).
> In the new UI I see f -> a -> ... path in the left subpane. I don't see b(),
but
> I see a drop in size between f() and a(), so something must be there. I click
on
> f(), find that there is b() (that's not easy because a() is not highlighted
> there), double-click on b(), and lose everything about a() that I had.
> One solution I can think of is to have right subpane to memorize the
previously
> expanded paths. I.e. if I dug into a(), then clicked on f in the left subpane,
> double clicked on b(), played with it, then returned to f, I would see two
> highlighed entries: one for b() (because it's currently selected) and one for
> a() (because it was selected before). Clicking (or right-keying) on a() would
> restore everything I had before I clicked to b(), including state of all
> subframes of a().

The proposed solution seems very non-intuitive and it doesn't scale (e.g. what
if you want to compare 3 different paths?). Two solutions I can think of:

  A) Multiple views. Add support for displaying multiple "Heap details" views
     simultaneously. Each of them could then display a separate path.
  B) Branched paths. Show multiple paths simultaneously in the left pane (as a
     tree). We would need to figure out how to add/change/remove a branch from
     the tree (might be too difficult to use for non-experts).

> 9. Bug: as was mentioned in #8, selecting a frame in the left subpane should
> highlight currently broken down frame in the right subpane.

As I explained above, I think that this would make the UI quite confusing.

> 10. Also, the new UI feels a little slower when enough depth is expanded.

This is unexpected. Could you please send me a trace where this happens?

> 11. Heavy modes are way faster, wow!

This was expected :-)

> 12. 'Count' column is not displayed at all. BTW, previously we also had
'Average
> size' - that was useless IMHO because almost always allocations are of
different
> sizes, and it ended up with some strange value like 20.3 bytes.

Fixed.

'Average size per allocation' column was proposed by ssid@
(https://github.com/catapult-project/catapult/issues/2258#issue-149096076). I'm
very happy to remove it if it's not useful.

[Primiano Tucci (use gerrit), 2016-09-26 10:57:40]

Allright then. Let's get this and improve all the issues Dmitry has pointed out
in the next patches.
For me the major win is that this one is way faster. Thanks a lot for working on
this.

LGTM

[petrcermak, 2016-09-27 15:14:52]

The CQ bit was checked by petrcermak@chromium.org

[petrcermak, 2016-09-27 15:14:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/2347813002/#ps60001
(title: "Improve focus retention and add tests")

[commit-bot: I haz the power, 2016-09-27 15:14:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 15:38:23]

Description was changed from

==========
[tracing] Heap dump UI overhaul

This patch modifies the "Heap details" pane by replacing the existing
nested table view with a more suitable explorer-like view. See the
tracking bug for more details.

BUG=catapult:#2750
==========

to

==========
[tracing] Heap dump UI overhaul

This patch modifies the "Heap details" pane by replacing the existing
nested table view with a more suitable explorer-like view. See the
tracking bug for more details.

BUG=catapult:#2750

Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2016-09-27 15:38:24]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...