Closed
Bug 1201742
Opened 9 years ago
Closed 2 years ago
Use a Tree Table component in performance's call tree, allocations views
Categories
(DevTools :: Performance Tools (Profiler/Timeline), defect, P2)
DevTools
Performance Tools (Profiler/Timeline)
Tracking
(Not tracked)
RESOLVED
INVALID
People
(Reporter: bgrins, Unassigned)
References
(Blocks 1 open bug)
Details
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•9 years ago
|
||
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.
Reporter | ||
Comment 2•9 years ago
|
||
(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
Reporter | ||
Comment 3•9 years ago
|
||
Another requirement is that contents of the table can be arbitrary DOM nodes as in the network monitor
Comment 4•9 years ago
|
||
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•9 years ago
|
||
@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•9 years ago
|
||
Replacing the rendering to be based on React is exactly what I have in mind to get dom diffing and only rendering visible items.
Reporter | ||
Comment 7•9 years ago
|
||
(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•9 years ago
|
||
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.
Reporter | ||
Comment 9•9 years ago
|
||
(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)
Reporter | ||
Comment 11•9 years ago
|
||
I've started a document to summarize the requirements and existing widgets we have here: https://etherpad.mozilla.org/devtools-tree-table
Comment 12•9 years ago
|
||
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
Reporter | ||
Comment 13•9 years ago
|
||
(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•9 years ago
|
||
(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.
Updated•9 years ago
|
Depends on: 1214323
Summary: Make the performance call tree columns resizable → Use a Tree Table component in performance's call tree, allocations views
Updated•9 years ago
|
Blocks: dt-widget-unity
Comment 15•9 years ago
|
||
See bug 1199126 for a case where we want to sort a column based on the model,
not based on the displayed text.
Reporter | ||
Updated•9 years ago
|
https://github.com/facebook/fixed-data-table is Facebook's implementation of a table widget and is reasonably extensible.
Just sayin'
Reporter | ||
Comment 17•9 years ago
|
||
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
Depends on: 1247065
Updated•9 years ago
|
Priority: -- → P1
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•2 years ago
|
Severity: normal → S3
Comment 19•2 years ago
|
||
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
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → INVALID
You need to log in
before you can comment on or make changes to this bug.
Description
•