1201742 - Use a Tree Table component in performance's call tree, allocations views

Status: RESOLVED INVALID

(DevTools :: Performance Tools (Profiler/Timeline), defect, P2)

Description

[Brian Grinstead [:bgrins]]

It'd be good to be able to resize the columns.

Sidenote: I understand that maybe this tree widget would need some changes to be able to add resize, reorder, sorting functionality.  If this won't work, I hope we can pick an existing widget to be *the* widget to use for any tabley/tree-like things in the tools.  I believe Gabriel has started using this one for Promise debugging, for example.  Netmonitor has it's own thing, and the  storage inspector and console.table use the TreeWidget and TableWidget, respectively.

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

TreeWidget and TableWidget need to be replaced, IMO. Neither is flexible enough for a generalized widget and have many warts.

Ccing James, mentioned doing a tree style widget for debugger's sources.

Currently Perftools' call tree and waterfall, and memory tools are using AbstractTreeItem (and JIT Coach stuff too, probably, as it's using TreeWidget right now and there are too many issues with it.)

The downfalls of AbstractTreeItem is that it renders everything up front -- if we only rendered what's in the view, this would be a huge performance gain. On pages that generate a lot of markers, this can easily crash the tools (several bugs open for this). On the upside, it's very flexible.

If we have a generalized tree (ideally that does not use XUL) and can also handle sortable, reorderable, and resizable, that could render both something like a traditional table (storage, network), tree (waterfall) and table-tree combos (call tree, memory), and only rendered what was in view, bonus points if it could only render a React-style diff, I'd be in heaven.

Comment 2

[Brian Grinstead [:bgrins]]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #1)
> If we have a generalized tree (ideally that does not use XUL) and can also
> handle sortable, reorderable, and resizable, that could render both
> something like a traditional table (storage, network), tree (waterfall) and
> table-tree combos (call tree, memory), and only rendered what was in view,
> bonus points if it could only render a React-style diff, I'd be in heaven.

If getting from the AbstractTreeItem to something like that is going to be a lot of work (and it sounds like it might), I suggest we evaluate some 3rd party libraries to see if there is some code we could use or adapt from there.  One that I've found that has a 'subgrid' concept (which I think is similar to what the call stack view does aka 'table-tree combo') is here: http://griddlegriddle.github.io/Griddle/subgrids.html

Comment 3

[Brian Grinstead [:bgrins]]

Another requirement is that contents of the table can be arbitrary DOM nodes as in the network monitor

Comment 4

[Victor Porof [:vporof][:vp]]

If we're going to have another widget, I want the AbstractTreeItem's flexibility with displaying things, with the bonus of only displaying what's in the view, dom diffing and having resizable colums when used as a table.

The AbstractTreeItem can be changed to render only what's in the view. Shouldn't be hard, I have something that resembles a patch. DOM diffing could happen if we use React for rendering. However, making its columns resizable when used as a table would require each implementation to write its own resizing logic. That's nasty.

Comment 5

[James Long (:jlongster)]

@vporof not too familiar with AbstractTreeItem, but if we end up using React the tree/table view would just accept component types to render the individual items with, which sounds similar to how it currently works. This would actually be a good use case to show React's composability. Not sure what we'll end up just wanted to say that.

Maybe in Q4 we can experiment with a One True widget but right now sounds like trying that optimization in AbstractTreeItem is a good plan.

Comment 6

[Victor Porof [:vporof][:vp]]

Replacing the rendering to be based on React is exactly what I have in mind to get dom diffing and only rendering visible items.

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vporof][:vp] from comment #6)
> Replacing the rendering to be based on React is exactly what I have in mind
> to get dom diffing and only rendering visible items.

I believe column resizing and sorting is critical for the UX here (also for any tool that shows data like this).  As I understand from Comment 4 that will be a lot of work to add to the AbstractTreeItem, so we should plan to ultimately do that work or replace the widget.

If replacing the rendering isn't a lot of work it seems like it would be a win for perf in the interim.

Comment 8

[Victor Porof [:vporof][:vp]]

Replacing AbstractTreeItem's rendering with React for perf gains is trivial, but must be done for all its subclasses/incarnations.

On the other hand, I don't have any immediate ideas for how we could support resizable columns and maintain AbstractTreeItem's flexibility with what it renders.

Worth noting is that there's no use rewriting an entirely new widget that doesn't give us all the above mentioned benefits.

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vporof][:vp] from comment #8)
> Replacing AbstractTreeItem's rendering with React for perf gains is trivial,
> but must be done for all its subclasses/incarnations.
> 
> On the other hand, I don't have any immediate ideas for how we could support
> resizable columns and maintain AbstractTreeItem's flexibility with what it
> renders.
> 
> Worth noting is that there's no use rewriting an entirely new widget that
> doesn't give us all the above mentioned benefits.

Agreed that we shouldn't write/rewrite/pull in a 3rd party widget unless if it can accommodate the needs outlined in this bug.  My preferences in order:

1) We could take one of our existing widgets that's already in tree with test coverage and enhance it to fit our feature / perf needs.
2) We could pull in a 3rd party library and make (and hopefully contribute back) any modifications needed for it work for us
3) Do anything else except for 4
4) We could write a brand new widget

Between 1 and 2 I think it's about figuring out what's going to be less work (both up front and ongoing)

Comment 11

[Brian Grinstead [:bgrins]]

I've started a document to summarize the requirements and existing widgets we have here: https://etherpad.mozilla.org/devtools-tree-table

Comment 12

[Jordan Santell [:jsantell] [@jsantell]]

fitzgen made a react-able, render-only-in-view tree for the initial prototype of memory tools here: https://bugzilla.mozilla.org/attachment.cgi?id=8487436&action=diff#a/browser/devtools/memory/Tree.js_sec2

Comment 13

[Brian Grinstead [:bgrins]]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #12)
> fitzgen made a react-able, render-only-in-view tree for the initial
> prototype of memory tools here:
> https://bugzilla.mozilla.org/attachment.cgi?id=8487436&action=diff#a/browser/
> devtools/memory/Tree.js_sec2

Does that support table-like layout for columns?  Relatedly, Honza is in the process of landing a table-like tree for the JSON viewer (Bug 1132203), but I don't think it includes render-only-in-view functionality yet.  Maybe these two could be somehow merged?

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Brian Grinstead [:bgrins] from comment #13)
> (In reply to Jordan Santell [:jsantell] [@jsantell] from comment #12)
> > fitzgen made a react-able, render-only-in-view tree for the initial
> > prototype of memory tools here:
> > https://bugzilla.mozilla.org/attachment.cgi?id=8487436&action=diff#a/browser/
> > devtools/memory/Tree.js_sec2
> 
> Does that support table-like layout for columns?  Relatedly, Honza is in the
> process of landing a table-like tree for the JSON viewer (Bug 1132203), but
> I don't think it includes render-only-in-view functionality yet.  Maybe
> these two could be somehow merged?

Yes, see AllocationsTree.js for a concrete use in that same diff.

Comment 15

[Tom Tromey :tromey]

See bug 1199126 for a case where we want to sort a column based on the model,
not based on the displayed text.

Comment 16

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

https://github.com/facebook/fixed-data-table is Facebook's implementation of a table widget and is reasonably extensible.

Just sayin'

Comment 17

[Brian Grinstead [:bgrins]]

There's an implementation in flight in Bug 1247065.  I'm assuming (hoping) this new one or the mem tool's tree will cover the use case here

Comment 18

[Victor Porof [:vporof][:vp]]

Blocked by the widgets work, P2 for now.

Comment 19

[Jan Honza Odvarko [:Honza] (always need-info? me)]

This report is related to the old DevTools Profiler.
The Performance panel now points to the new Firefox profiler available at profiler.firefox.com
Closing as Invalid bug