Introduce column resizer in request list
Categories
(DevTools :: Netmonitor, defect, P1)
Tracking
(firefox63 wontfix, firefox64 wontfix, firefox65 wontfix, firefox66 wontfix, firefox67 verified)
People
(Reporter: rickychien, Assigned: lba_2)
References
(Depends on 2 open bugs, Blocks 2 open bugs)
Details
(Keywords: dev-doc-complete, Whiteboard: [netmonitor])
Attachments
(2 files, 30 obsolete files)
Chromium devtools is using column resizer to control column width. It's nice to have feature and bring flexibility, so user can adjust their request table as they want. As comment https://bugzilla.mozilla.org/show_bug.cgi?id=1349173#c36 mentioned, column resizer is good solution to distribute available column width, it's also can fix the bug (https://bugzilla.mozilla.org/show_bug.cgi?id=1349173#c26) when hiding column in request list.
Updated•7 years ago
|
Comment 3•6 years ago
|
||
Did not face face issue before (54.0.1, 54.0 and previous), but faced it today after moving to 55 release.
Comment 4•6 years ago
|
||
(In reply to Alexandr Paliy from comment #3) > Did not face face issue before (54.0.1, 54.0 and previous), but faced it > today after moving to 55 release. The DevTools never allowed to resize the columns in the Net Monitor. This report asks to add this feature. Sebastian
Comment 5•6 years ago
|
||
This is a very highly requested feature. I've heard 2 requests in the past week. One today on IRC #devtools. I think it would be worth spending time making the bug approachable at least so that people can come in and try to fix it. Ricky: do you think you could point people in the right directions here? Which files would need to be modified for this? Which components? Whether a pref should be added to store the sizes? What are the edge cases to keep in mind? Where is the complexity? Etc. I think this would help a lot someone (with some DevTools hacking experience) to get started. And then maybe, setting yourself (or Honza?) as mentor on the bug would make it more visible too.
Comment 6•6 years ago
|
||
Couple of notes: * The solution should be based on React/Redux (no XUL like the Storage panel) * We might want to get inspiration from https://github.com/react-dnd/react-dnd * The result component should be shared among panels (e.g. used in the Storage panel) * Good analysis prior implementation needed. Honza
Comment 7•6 years ago
|
||
Hi Honza, I also found this library to suit our needs https://facebook.github.io/fixed-data-table/example-resize.html Your thoughts on whether we can use fixed-data-table to solve this issue?
Comment 8•6 years ago
|
||
(In reply to Abhinav Koppula from comment #7) > Hi Honza, > I also found this library to suit our needs > https://facebook.github.io/fixed-data-table/example-resize.html > Your thoughts on whether we can use fixed-data-table to solve this issue. Yep, looks promising. I don't like how the header labels are selected when dragging the column marker, but that's probably just a little detail. One thing to note. We want to change the UI/UX so, entries in the table are expandable, and so the user can see all the HTTP details (headers, response, etc.) underneath of the request (just like in the Console panel) instead of in the side bar. So, height of a row doesn't have to be always the same. Honza
Reporter | ||
Comment 9•6 years ago
|
||
Sorry for the late reply. I'm busy in shipping 57 and Mozilla dev conference. I believe Honza has mentioned useful notes on comment 6 and fixed-data-table seems like the right component we want. Feel free to ask Honza and me! BTW, Each column width can be stored in our pref like `visibleColumn` [1]. [1] http://searchfox.org/mozilla-central/source/devtools/client/netmonitor/index.js#24-27
Reporter | ||
Updated•6 years ago
|
Reporter | ||
Comment 10•6 years ago
|
||
Here is my observation of fixed-data-table-2 [1] and react-virtualized [2] The mechanism behind fixed-data-table-2 is implemented by `transform: translate` to position each table row individually. Take a look at its example [1] to see how it performs while scrolling in a large amount of data (> 1,000,000 rows in this example). The transform mechanism is pretty fitted into Firefox since it doesn't cause any conflict with APZ. I'm sure fixed-data-table-2 is using recycle pool to maintain the list of data just like react-virtualized, but it won't render the blank result when scrolling too fast. It performs pretty fast and smooth! However, we need to pay more attention on dealing with responsive table [3] since the dimension of Netmonitor flexible. The resizing performance of example [3] is noticeable janky and it's unacceptable. It's using react-dimension to adjust the table dimension [4]. Thus, we'd probably find out a better approach to make table resizing more faster and responsive. On the other hand, react-virtualized doesn't use `transform: translate` way to position table row, it's choosing `position: absolute` to layout the table. Sounds similar but it will conflict with APZ and incurs blank screen if user scrolls the table too fast. If we can find out a solution to deal with resizing issue of fixed-data-table-2, I believe fixed-data-table-2 is a nice library for Netmonitor. I'll keep investigating it to make sure fixed-data-table-2 can fulfill our needs. [1] http://schrodinger.github.io/fixed-data-table-2/example-object-data.html [2] https://bvaughn.github.io/react-virtualized/#/components/List [3] http://schrodinger.github.io/fixed-data-table-2/example-responsive.html [4] https://github.com/schrodinger/fixed-data-table-2/blob/master/examples/ResponsiveExample.js#L11
Reporter | ||
Comment 11•6 years ago
|
||
After modifying the number of FakeObjectDataListStore in example [1] from 1,000,000 to 1,000, I can see the speed of table resizing is way faster than before. P.S. In practice, cnn.com is less than 500 requests & bbc.com is less than 300 requests, so I believe 1,000 rows is a reasonable number. If there is no other obvious issues found in fixed-data-table-2, I'm going to refactor entire request-list with using fixed-data-table-2. [1] https://github.com/schrodinger/fixed-data-table-2/blob/master/examples/ResponsiveExample.js#L18
Comment 12•6 years ago
|
||
Ricky, please consider that the vast majority of our users have slower computers than we have. Also, remember that the net monitor can persist the requests: if a user keeps this setting enabled and reload the page while working (usual practice, right? :) ), it can achieve a lot more lines than what a website normally is. So while 1,000,000 is likely a lot, I think you should look at numbers like 100,000 (reloading a single webpage more than 300 times in one day while working on it doesn't seem unusual).
Reporter | ||
Comment 13•6 years ago
|
||
Thank you Julien for this good insight! I've done more investigation about adjusting configuration of react-dimension and reproduce the case of rendering 1,000,000 items in ResponsiveExample. After setting `debounce: 50` into react-dimension config [1], I've seen that table horizontal resizing is way better. (I'd prefer to test horizontal or vertical resizing individually since AFAIK there is no way to do horizontal and vertical resizing for toolbox at the same time.) [1] https://github.com/schrodinger/fixed-data-table-2/blob/master/examples/ResponsiveExample.js#L61
Reporter | ||
Comment 14•6 years ago
|
||
Attachment is an example showing table resizing performance of example-object-data.html in fixed-data-table site. As you can see in attachment, there are some red bars indicating in FPS timeline and telling us a requestAnimationFrame call from react-dimension took 56.70ms to complete a task. We got other similar red bars while resizing table. Comparing with current request list implementation, pure flexbox layout is way faster than passing width/height props into react component. By calculating size through layout engine itself which is able to avoid redundant scripting calculation. Unfortunately, according to fixed-data-table document [1], width / height props seem to be required. [1] https://github.com/schrodinger/fixed-data-table-2/tree/master/docs#create-your-table
Reporter | ||
Comment 15•6 years ago
|
||
After taking a glance at the implementation of fixed-data-table-2 [1], there are more than one places requiring `width` props to determine the width of row or width of column, which looks like the width / height props are necessary and impossible to workaround by flexbox. However, we can enable `debounce` (comment 13) to mitigate the performance issue of horizontal resizing and efficiently reduce the occurrence frequency of red bars. If above result is still unacceptable, we might go back to see how to implement column resizer without using library. [1] https://github.com/schrodinger/fixed-data-table-2/tree/master/src
Reporter | ||
Comment 16•6 years ago
|
||
After netmonitor meeting today, we've reached a consensus that not to integrate third party library into our code since both react-virtualized and fixed-data-table-2 have perf downsides or constraints. Instead of adopting new library, we'll start working on column resizer feature by ourselves.
Reporter | ||
Updated•6 years ago
|
Reporter | ||
Comment 17•6 years ago
|
||
This feature will depend on fixed-data-table experiment but unfortunately we hit perf bottleneck based on WIP patch https://bugzilla.mozilla.org/show_bug.cgi?id=1414728#c6. Therefore, team have decided to fully focus on perf issue first before jumping into column resizer. We will come back to this soon once the perf issue gets fixed.
Updated•6 years ago
|
Comment 19•6 years ago
|
||
Should bug 1308451 be closed as duplicate or should it be used to implement the widget code and this one to use the widget in the Netmonitor? Sebastian
Comment 20•6 years ago
|
||
(In reply to Sebastian Zartner [:sebo] from comment #19) > should it be used to implement the widget code and this one to use > the widget in the Netmonitor? Yep, let's keep this one open. Honza
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 22•5 years ago
|
||
This patch is work in progress: - Draggable component is introduced in RequestListHeader.js - basic styling for splitter (draggable) in RequestList.css
Comment 23•5 years ago
|
||
Comment on attachment 9034318 [details] [diff] [review] resizable-columns-v1.patch Review of attachment 9034318 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for working on this Lenka! Honza ::: devtools/client/netmonitor/src/components/RequestListHeader.js @@ +168,5 @@ > > return div({ className }, label); > } > > +// Dragging Events nit: wrong indentation @@ +248,5 @@ > div({ > className: "devtools-toolbar requests-list-headers", > onContextMenu: this.onContextMenu, > }, > HEADERS.filter((header) => columns[header.name]).map((header) => { The filtering is done already above so, we could utilize the result here. @@ +269,5 @@ > + // Calculate splitter size > + const splitterSize = 5; > + const splitterStyle = { > + flex: "0 0 " + splitterSize + "px", > + }; This looks like something that doesn't have to be in the loop (constants are usually defined at the top of the file).
Comment 25•5 years ago
|
||
A quick note about performance. I'm expecting this work to have a significant impact on netmonitor performance. Both possibly positive and negative. So I would suggest pushing work in progress patches early to DAMP in order to have a sense of its impact on perf.
Comment 26•5 years ago
|
||
As I know it is not easy to do so, I pushed the current patch to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=261e05c2f0b71d362ccb6839d4331048298e819c
https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=261e05c2f0b71d362ccb6839d4331048298e819c
Comment 27•5 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #26)
As I know it is not easy to do so, I pushed the current patch to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=261e05c2f0b71d362ccb6839d4331048298e819c
https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=261e05c2f0b71d362ccb6839d4331048298e819c
Thanks Alex for the help here!
Note that the patch is currently very preliminary, so the results doesn't have to be useful. But, it's great to watch the impact since the very beginning.
Honza
Comment 28•5 years ago
|
||
Here is the actual results page:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=261e05c2f0b71d362ccb6839d4331048298e819c&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&framework=12&selectedTimeRange=172800
The patch may have an impact on simple.requestsFinish which isn't a big deal.
Updated•5 years ago
|
Assignee | ||
Comment 29•5 years ago
|
||
This patch is still just a WIP:
@Alex please have a look. In order for the table to resize - it is currently using display: table-row
in class .request-list-item
(RequestList.css) that you have commented out in regards to the bug 1431132. Please share your thoughts on this.
Thank you,
Lenka
Comment 30•5 years ago
|
||
A follow up for comment #29
Here are some Talos results showing how much the performance regress when we use display: table-row;
CSS prop for .request-list-item class in the Network panel list.
complicated.netmonitor.reload.DAMP 11%
complicated.netmonitor.requestsFinished.DAMP 13%
Note that this props was removed in bug 1431132 to avoid unnecessary repainting. But, it's extremely useful for column resizing - as we can set width on header only and make all <td>s be adjusted automatically.
There is another Talos push, using table-layout: fixed;
, but the same tests are regressing.
@Alex, is this table-layout: fixed;
change the one you had in mind when talking about possible perf optimization?
Honza
Comment 31•5 years ago
|
||
TBH. I never really understood why removing display: table-row; prevented reflows when adding a new row in the table.
We must be asking help from someone that better understand CSS than me.
Daniel, Would you have cycle to help us understand the netmonitor layout bottlenecks? Or know someone who would?
This is about this rule:
https://searchfox.org/mozilla-central/rev/bee8cf15c901b9f4b0c074c9977da4bbebc506e3/devtools/client/netmonitor/src/assets/styles/RequestList.css#572-582
.request-list-item {
position: relative;
/*
display: table-row;
Bug 1431132: Disabling this rule is surprising as we no longer have "rows".
But this helps preventing reflowing the whole requests list anytime we append
a new request/row.
*/
height: 24px;
line-height: 24px;
}
Here is an overview of netmonitor DOM (only a subset of it and I may have forgot important rules...):
<div class="requests-list-table" style="display: table; position: relative; width/height: 100%;">
<div class="requests-list-contents"
style="display: table-row-group; width: XXXpx; height: XXXpx; /* size set by JS */
position: absolute; overflow-x: hidden; overflow-y: auto;">
<div class="devtools-toolbar requests-list-headers-wrapper" style="position: sticky; width: 100%; display: flex;">
<div class="request-list-item" style="display: block; position: relative; height/line-height: 24px;">
<div class="request-list-item" style="display: block; position: relative; height/line-height: 24px;">
<div class="request-list-item" style="display: block; position: relative; height/line-height: 24px;">
...
// <= this is when we add a new div.request-list-item that we get full table reflow when it is display: table-row;
One surprising thing is that div.requests-list-contents is having display: table-row-group; set, but display: block is shown in the computed view?!
The complexity of netmonitor UI is mostly around its sticky column titles (div.requests-list-headers-wrapper). Modifying anything easily break it and/or crush the performance. I'm pretty confident there is way too many hacks that piled up over time and that the final rules we are using make no sense anymore.
So may be we should go from a blank page and see what would be the best rules to use to implement a table with a sticky header and go from there? Break the UI, but see if the performance is better.
Comment 32•5 years ago
•
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #31)
One surprising thing is that div.requests-list-contents is having display: table-row-group; set, but display: block is shown in the computed view?!
I can reproduce this issue too. The Rule panel seems to work well.
Honza
Assignee | ||
Comment 33•5 years ago
|
||
@Alex, thanks for your comments.
I was doing some googling re:table performance and these are some interesting suggestions I found. Maybe some of you with better understanding of how React works can tell, how is the table row being created in our case.
... // <= this is when we add a new div.request-list-item that we get full table reflow when it is display: table-row;
Stack overflow this issue
- it suggests replacing .insertRow() with .createElement('tr')
- also some useful CSS improvements
- also suggests maybe using .createDocumentFragment
- and more, please take a look
Another interesting article
Another discussion
This article is more complex, but might be useful.
Someone who has more deeper understanding can get some suggestions from here for our situation with netmonitor?
Comment 34•5 years ago
•
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #31)
<div class="requests-list-contents"
style="display: table-row-group; width: XXXpx; height: XXXpx; /* size set by JS */
position: absolute; overflow-x: hidden; overflow-y: auto;">
[...]
One surprising thing is that div.requests-list-contents is having display: table-row-group; set, but display: block is shown in the computed view?!
That's because it has 'position:absolute', which converts its display
value to the closest block-level thing. (e.g. inline-flex--> flex, inline-->block, etc) For table-row-group, there is no block-level equivalent, so it just gets directly converted to display:block.
So its display:table-row-group
styling is ignored/unused, as you noticed, as long as it has position:absolute
basically. Table parts (like display:table-row
elements) inside of it will trigger the generation of anonymous table wrapper frames as-needed, so that they can make sense of their world.
RE the slow table-row insertion:
// <= this is when we add a new div.request-list-item that we get full table reflow when it is display: table-row;
You're probably triggering something like this case:
https://searchfox.org/mozilla-central/rev/dac799c9f4e9f5f05c1071cba94f2522aa31f7eb/layout/base/nsCSSFrameConstructor.cpp#11296
...which causes anonymous table wrapper frames to be regenerated.
If you can ensure that you're inserting this new table-row into an element that is definitely a table-row-group (in its computed style), you may see better performance because we won't trigger the "rebuild our anonymous table wrappers for all the adjacent improperly-wrapped table parts that maybe should be grouped together" codepath.
Assignee | ||
Comment 35•5 years ago
|
||
This is a patch that's currently not working - I am attaching it for debugging and review with Honza.
Here I am implementing a function that should autoResize the columns, so they take up 100% of the available space. Also should autoResize the columns when the browser window is resized. But it has bugs currently.
Assignee | ||
Comment 36•5 years ago
|
||
Hello Daniel,
That's because it has 'position:absolute', which converts its
display
value to the closest block-level thing. (e.g. inline-flex--> flex, inline-->block, etc) For table-row-group, there is no block-level equivalent, so it just gets directly converted to display:block.
If you can ensure that you're inserting this new table-row into an element that is definitely a table-row-group (in its computed style), you may see better performance because we won't trigger the "rebuild our anonymous table wrappers for all the adjacent improperly-wrapped table parts that maybe should be grouped together" codepath.
I am not sure yet, why the position is set to absolute nor how can I get rid of it and not lose what it was implemented for.
However, I have a question in regards to our table structure in netmonitor.
Current hierarchy of elements:
Hierarchy CSS display prop Table element
<div.requests-list-table> -> table <table>
<div.requests-list-contents> -> table-row-group++ <tbody>
<div.requests-list-headers-wrapper> -> table-header-group** <thead>
<div.requests-list-headers> -> table-row <tr>
<div.request-list-column> -> table-cell <td>
</div.requests-list-column> </td>
</div.requests-list-headers> </tr>
</div.requests-list-headers-wrapper> </thead>
<div.request-list-item> -> table-row <tr>
<div.requests-list-column> -> table-cell <td>
</div.requests-list-column> </td>
</div.request-list-item> </tr>
</div.requests-list-contents> </tbody>
</div.requests-list-table> </table>
++ currently (computed -> block) because position: absolute
** position: sticky
.requests-list-column has no position specified (default is static)
All others have position: relative
Could that be problem (in regards to table behavior) that the <thead> is within <tbody> ? I do not know the reason behind this structure.
Thanks for any insights.
Lenka
Assignee | ||
Updated•5 years ago
|
Comment 37•5 years ago
•
|
||
(In reply to Lenka Pelechova from comment #36)
Could that be problem (in regards to table behavior) that the <thead> is within <tbody> ?
That is a problem, yes. I don't know if it's an important part of the main problem that you're investigating here, but it's definitely kinda-broken.
Specifically: let's ignore the extra position:absolute
wrinkle for a minute.... The table nesting that you diagrammed above means that we actually end up with two nested tables here, because the inner table-header-group
finds itself without the correct type of table-part parent box that it expects to have. So, it reacts to that by generating an anonymous display:table
box to wrap itself in. And in the frame tree, that anonymous box ends up being the child of the table-row-group
, and the parent of the table-header-group
.
Now, if we take the position:absolute
into consideration, we've still got a version of that same misparenting issue (but now our table-header-box
is just parented by a block
rather than the wrong sort of table-part) -- so the table-header-group
still ends up generating an anonymous display:table
wrapper box as its parent, either way.
So your intuition is right, I think, that it's probably saner for the thead
to not be a child of the tbody
(to avoid generating unexpected extra nested tables). Having said that, there may be organizational reasons that the current structure depends on the grouping that it's got, but I imagine those could be addressed by other tweaks.
Assignee | ||
Comment 38•5 years ago
|
||
for review with Honza
Assignee | ||
Comment 39•5 years ago
|
||
WIP - update for review
Update:
- when resizing 1 column, the added/substracted width should be compensated by the last column before waterfall (if waterfall visible). Otherwise it is the last column visible.
Assignee | ||
Comment 40•5 years ago
|
||
Update: Please apply this patch on top of the previous 9039508: WIP-resize-columns-01-28-19v2.patch
Change in this patch: the proffered widths are stored in % rather than in px.
This is work in progress, for code review for Honza.
thank you,
Lenka
Comment 41•5 years ago
|
||
Comment on attachment 9039508 [details] [diff] [review] WIP-resize-columns-01-28-19v2.patch Review of attachment 9039508 [details] [diff] [review]: ----------------------------------------------------------------- Review done at a meeting. Honza
Comment 42•5 years ago
|
||
Comment on attachment 9039729 [details] [diff] [review] WIP-resize-columns-01-29-19.patch Review of attachment 9039729 [details] [diff] [review]: ----------------------------------------------------------------- Review done at a meeting. Honza
Assignee | ||
Comment 43•5 years ago
|
||
Work in progress - this patch is attached for debugging with Honza at a review meeting.
Status update: Switching from using px for column widths to %. However, currently dealing with problem, that the headers (columns) don't stretch across all width of the table.
Comment 44•5 years ago
|
||
Hey, here is a crazy idea: if the width of all columns is to be known, maybe it's not necessary to keep this structure with a real table element and instead use flex. This could reduce the performance problems by removing this very long table with a lot of lines.
Comment 45•5 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #44)
Hey, here is a crazy idea: if the width of all columns is to be known, maybe it's not necessary to keep this structure with a real table element and instead use flex. This could reduce the performance problems by removing this very long table with a lot of lines.
Do you have more details about how the panel content DOM structure with flex would look like? Note that one of the advantages of table based layout is that when changing col-width -> only size of the header (one element) needs to be updated and all rows are adopted automatically.
Honza
Comment 46•5 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #44)
Hey, here is a crazy idea: if the width of all columns is to be known, maybe it's not necessary to keep this structure with a real table element and instead use flex. This could reduce the performance problems by removing this very long table with a lot of lines.
I'd rather say this shouts for grid, not flex, because it's a two-dimensional layout. All that needs to be done then is to adjust the grid-template-columns
property accordingly when resizing or adding/removing columns. I've created a simple example for this.
Sebastian
Comment 47•5 years ago
|
||
Honza, Sebastian, this was exactly my crazy idea: not using a grid-based layout (table or not table). Indeed we'd need to repeat the widths on each lines. But the advantage is that the layout engine doesn't need to compute the whole table and only needs to look at individual lines.
It's totally possible that grid layout with a grid-template-columns
value containing fixed pixels values (or even fr
values which is similar from a layout point of view) can give the same performance. All I'm saying is we should try this and measure.
Comment 48•5 years ago
|
||
The grid layout looks promising. Does grid support "sticky header"? (i.e. the header still visible even if the user scrolls down)
Honza
Comment 49•5 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] (need-info? me) from comment #48)
Does grid support "sticky header"?
Yeah, position:sticky works in grid and can achieve this sort of thing. There's a demo at e.g. https://codepen.io/stephanmax/pen/odyxWE -- each purple block there is a position:sticky grid item.
Comment 50•5 years ago
|
||
Here's :sebo's example, modified to have sticky headers:
https://jsfiddle.net/c4tp3x7q/
Comment 51•5 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #50)
Here's :sebo's example, modified to have sticky headers:
https://jsfiddle.net/c4tp3x7q/
Actually, you don't even need the extra scroll container, but in that case you need to set the height of the rows via grid-auto-rows
to min-content
or a fixed value to avoid the rows stretching over the height of the grid container when there's no overflow. See https://jsfiddle.net/SebastianZ/zoe9pa78/.
(In reply to Julien Wajsberg [:julienw] from comment #47)
Honza, Sebastian, this was exactly my crazy idea: not using a grid-based layout (table or not table). Indeed we'd need to repeat the widths on each lines. But the advantage is that the layout engine doesn't need to compute the whole table and only needs to look at individual lines.
I'm not sure how this could be faster regarding layout. Do you mean, if there are many rows the layout engine doesn't have to compute the ones that are outside the viewport?
But I agree that different possible solutions should be tested regarding performance, especially because there can be a huge amount of rows.
Sebastian
Assignee | ||
Comment 52•5 years ago
|
||
Work in progress - patch for review at a meeting with Honza.
Status update:
- Inserting thead at the right position immediately fixed the 100% header size
- position:absolute removed from .requests-list-contents
- Using
width
notminWidth
- min-width set for all columns in prefs
- implementing changes from our working prototype in react app into devtools:
Now the table has corrected structure :
<table>
<thead>
<tr>.....</tr>
</thead>
<tbody>
<tr>.....</tr>
<tr>.....</tr>
</tbody>
</table>
We are correctly assigning width to all columns in %
But disabling position:absolute for .requests-list-contents (display:table-row-group) causes no ability to scroll the rows and the row height gets ruined when resizing either the browser window or the height of the devtools window.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 53•5 years ago
|
||
corrected patch for honza
Assignee | ||
Comment 54•5 years ago
|
||
update: This patch has corrected problems with scrolling & row height & status bar that were introduced when we removed position:absolute from .requests-list-contents. This is a working prototype (still WIP :-)
Comment 55•5 years ago
|
||
I refactored the panel DOM structure on top of the suggested Grid layout (see the attached patch) I tried to do only minimal set of changes to see how performant it is.
But, I am seeing perf regressions on the following tests:
complicated.netmonitor.reload.DAMP 12.03%
complicated.netmonitor.requestsFinished.DAMP 17.87%
(these tests usually indicate that rendering/CSS layout is slower)
I'd love to get feedback on this experiment.
E.g. does that patch look ok to you?
Honza
Assignee | ||
Comment 56•5 years ago
|
||
(In reply to Lenka Pelechova from comment #54)
Created attachment 9041447 [details] [diff] [review]
WIP-resize-columns-02-05-19-v2.patchupdate: This patch has corrected problems with scrolling & row height & status bar that were introduced when we removed position:absolute from .requests-list-contents. This is a working prototype (still WIP :-)
Here are Talos results (if I've done it correctly - I am still new at this) for this patch (using html table layout):
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=85490ae15c45ba63e973694386456083d89c00fc&newProject=try&newRevision=a33c5cff8c539393a7c4b9515e543c98b6a86491&originalSignature=1759151&newSignature=1759151&framework=12
Comment 57•5 years ago
|
||
I'm not sure how this could be faster regarding layout. Do you mean, if there are many rows the layout engine doesn't have to compute the ones that are outside the viewport?
Thanks for your question, this made me thing more about why I was suggesting this. I think that the main thing is that the Netmonitor isn't a fixed view, it's changing a lot, especially when loading a page: new rows are added very often. Of course, it's slower and slower as the table is growing. So I think that's where my suggestion came from: by adding independant rows, the layout engine doesn't have to recompute the whole table at each new row, instead it can just compute the new added row.
Again, this is just a wild guess and a crazy idea, and maybe using a grid layout will provide just the same thing, and I agree this is the best way if it works like we want. When testing the various scenarios, enabling paint flashing on chrome could help looking at whether Gecko repaints the whole table or just the changed parts.
Comment 58•5 years ago
|
||
(In reply to Julien Wajsberg [:julienw] from comment #57)
I'm not sure how this could be faster regarding layout. Do you mean, if there are many rows the layout engine doesn't have to compute the ones that are outside the viewport?
Thanks for your question, this made me thing more about why I was suggesting this. I think that the main thing is that the Netmonitor isn't a fixed view, it's changing a lot, especially when loading a page: new rows are added very often. Of course, it's slower and slower as the table is growing. So I think that's where my suggestion came from: by adding independant rows, the layout engine doesn't have to recompute the whole table at each new row, instead it can just compute the new added row.
Ok, I've created three new examples, one with table, one with grid and one with flex layout, same HTML structure, same JavaScript code, only different CSS. They both have a button to add 1000 new rows at once and a second one that adds them continuously:
Table: https://jsfiddle.net/SebastianZ/567spj10/
Grid: https://jsfiddle.net/SebastianZ/eab4j32f/
Flexbox: https://jsfiddle.net/SebastianZ/oaz2grnq/
On my machine there is no big difference between flex and grid (~120ms for the last layout), but the normal table layout is about twice as fast. Having said that, the HTML structure in these examples is not optimized for flex and grid, so they might get a little bit more performant after optimizations.
Sebastian
Assignee | ||
Comment 59•5 years ago
|
||
Update:
-
this patch fixes some (not all) of the issues from our last review
-
some clean up of CSS
Here are talos results for this patch (the regression numbers went down some). -
intended for next review w/Honza
Comment 60•5 years ago
|
||
(In reply to Sebastian Zartner [:sebo] from comment #58)
Table: https://jsfiddle.net/SebastianZ/567spj10/
Grid: https://jsfiddle.net/SebastianZ/eab4j32f/
Flexbox: https://jsfiddle.net/SebastianZ/oaz2grnq/On my machine there is no big difference between flex and grid (~120ms for the last layout), but the normal table layout is about twice as fast. Having said that, the HTML structure in these examples is not optimized for flex and grid, so they might get a little bit more performant after optimizations.
Great analysis, thanks Sebastian!
Honza
Comment 61•5 years ago
|
||
Comment on attachment 9042022 [details] [diff] [review] WIP-resize-columns-02-07-19.patch Review of attachment 9042022 [details] [diff] [review]: ----------------------------------------------------------------- Review done at a meeting. Honza
Comment 62•5 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] (always need-info? me) from comment #55)
I'd love to get feedback on this experiment.
E.g. does that patch look ok to you?
I've not looked at the patch itself, only at the end result in term of rendering.
With nglayout.debug.paint_flashing_chrome turned on, you can see that there is a repaint on the far right of the netmonitor, everytime a new request is added.
Here is a handy test page to check that:
data:text/html,<script>window.onclick=()=>{x=new XMLHttpRequest();x.open("GET","http://mozilla.org",false);x.send();</script>
Clicking on the page will add a new request, it is important to have only the scrollbars to be repainted once the list overflow and requests get added out of the viewport.
Otherwise the end result looks broken, the column are mixed up and the timeline isn't shown.
I believe that it would really help trying to understand and prototype anything if we first get rid of all the cruft:
- useless DOM Elements thanks to react now supporting returning list of elements
For example this wrapper:
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/RequestListContent.js#278
div({ className: "requests-list-wrapper" },
Or this other wrapper:
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/components/RequestListHeader.js#170
div({ className: "devtools-toolbar requests-list-headers-wrapper" },
Or may be all these intermediate containers:
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/assets/styles/netmonitor.css#34-37
#mount,
.launchpad-root,
.network-monitor,
.monitor-panel { - all non-mandatory CSS rules
I imagine we should read carefully these two files:
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/assets/styles/netmonitor.css
https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/assets/styles/RequestList.css
And try removing anything that may not be mandatory.
Especially the rules: overflow, (min-/max-)width/height, display, position.
Also, I remember that I was having an hard time preventing any unecessary reflow because of the toolbar having a variable height. Depending on the width of the window, it can be one or two lines.
It is based on this rule (if I remember correctly)
.devtools-toolbar-container {
height: auto;
}
So if you are having extra repaint, it is always worth removing the header to see if that's a side effect of this complex layout or not.
We do focus a lot of the request list layout, but that may not be the real source of our issues.
We should also look at the very complex setup we have around it! From the <table> up to <body>, we have 8 elements with a lot of various CSS rules, especially flex layouts and some usages of overflow.
Comment 63•5 years ago
|
||
On my machine there is no big difference between flex and grid (~120ms for the last layout), but the normal table layout is about twice as fast. Having said that, the HTML structure in these examples is not optimized for flex and grid, so they might get a little bit more performant after optimizations.
Love the test cases as well! The constrains that right now cause overhead are not scrolling but (as Alex pointed out in his test) the overpainting when adding/updating rows and the layout overhead by table cells with overflow (can't find the bug atm). If we have a layout that is more expensive initially but has less overhead when updating, it might be a viable trade off.
Assignee | ||
Comment 64•5 years ago
|
||
Update 02/11/19:
- fix the resizer in scenario for very narrow browser window
- fixed missing text ellipsis in header labels
- calling resizeWaterfall() when the width of waterfall column changes
- when resizing waterfall - all columns are affected (although still not quite right)
patch for review in the meeting with Honza
Assignee | ||
Comment 65•5 years ago
|
||
Update 02-12-19:
- The header is not be displayed if there are not data (requests)
- when Add/remove columns, widths are recalculated
- in componentDidUpdate(), check if widths need to be updated. Recalculate columns widths, store in prefs
- resizeWaterfall in onStopMove()
- Introduce this.getVisibleColumns()
Work in progress - patch for code review in meeting
Comment 66•5 years ago
|
||
Store columns data into UI reducer + auto save to preferences.
Honza
Comment 67•5 years ago
|
||
Updated version
Updated•5 years ago
|
Comment 68•5 years ago
|
||
@Lenka, please look at this last patch. If all is working fine we should merge both together.
Honza
Updated•5 years ago
|
Assignee | ||
Comment 69•5 years ago
|
||
02-15-19 Update:
- added redux functionality patch
- some fixed tests
Note: one new test browser_net_headers-resize.js is still not finished, and one test browser_net_autoscroll.js is still failing (Work in Progress)
Comment 70•5 years ago
|
||
Comment on attachment 9044103 [details] [diff] [review] WIP-resize-columns-02-15-19v2.patch Review of attachment 9044103 [details] [diff] [review]: ----------------------------------------------------------------- @Lenka: browser_net_headers-resize.js test-file seems to be missing in the patch. Honza
Assignee | ||
Comment 71•5 years ago
|
||
added missing test file browser_net_headers-resize.js
Comment 72•5 years ago
|
||
@Lenka: please merge this into your patch, it should fix the auto-scroll issue and the related test.
Honza
Assignee | ||
Comment 74•5 years ago
|
||
02-18-19
Thank you @Honza for your help.
Here is a patch with integrated fix for auto-scroll feature, plus the new test for browser_net_headers-resize.js
Lenka
Comment 75•5 years ago
|
||
Comment on attachment 9044572 [details] [diff] [review] WIP-resize-columns-02-18-19.patch Patch discussed at a meeting.
Comment 76•5 years ago
|
||
I've opened bug 1529018 in order to focus on the current implementation of the netmonitor and why it is so hard to prevent the repaints when new requests are added. Once we will figure this out, it will be easier to ensure we keep preventing these repaints in the column resize refactoring.
Assignee | ||
Comment 77•5 years ago
|
||
02-20-19 update:
- code refactor
- still working on the test where we simulate mouse resize
- work in progress
Comment 78•5 years ago
|
||
Comment on attachment 9045167 [details] [diff] [review] WIP-resize-columns-02-20-19.patch Review done at a meeting. Honza
Assignee | ||
Comment 79•5 years ago
|
||
02-21-19 update
- patch for review
- code refactoring
- still working on test browser_net_headers-resize.js
Assignee | ||
Comment 80•5 years ago
|
||
02-24-19 Update:
- I have corrected one small bug that caused the new test browser_net_headers-resize.js to fail but the test is still failing. I suspect the problem could be in method moveWaterfall(). I will look at it some more. But here is the current patch.
Assignee | ||
Comment 81•5 years ago
|
||
Honza,
please see if you can spot something... there is a slight change in width that I wouldn't expect and atm I can't figure out why
-
install the latest patch
-
console.log in method moveWaterfall():
new computed width:
const newWaterfall = Math.max(waterfallRefRect.width - diff, minWaterfall);
console.log("moveWaterfall - new width:" + newWaterfall);
and after setting style, console log again:
waterfallRef.style.width =${this.px2percent(newWaterfall, parentWidth)}%
;
console.log("set style for waterfall:" + waterfallRef.style.width);
console.log("new real waterfall width:" + waterfallRef.getBoundingClientRect().width); -
build and test like this:
- open Netmonitor & read a request
- reset columns
- resize waterfall column and check the console
- you will see that the real waterfall width is slightly higher than new width which was computed and set in style (translated to %)
- what I would expect is to see the same real width as the newWaterfall width that was set in .style.width
Similar slight adjustment in width is also happening in onMove with any column.
It is not significant, because the resizing works for the user as expected and the "automatic" resize is just tiny but as it adds up it causes the test to fail. That's how I found out about it.
thanks,
Lenka
Assignee | ||
Comment 82•5 years ago
|
||
needs to be applied on top of these 2 patches
1.first apply this Bug 1529018 - Simplify netmonitor DOM/CSS around table elements
2. then this patch Bug 1529018 - Use table DOM elements instead of div.
3. and after that attachement in this patch
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 83•5 years ago
|
||
03-04-19 update
- This is a resize-columns patch rebased on top of changes from [Bug 1529018] (Figure out netmonitor layout issues)
- just noticed that it doesn't include a fix for missing border on headers from (will add that tomorrow to the patch)
Assignee | ||
Comment 84•5 years ago
|
||
Update [03-06-19]
- updated code + fixed a bug that cause intermittent failure of test devtools/client/netmonitor/test/browser_net_background_update.js
- also confirmed on Talos that removal of onResize() caused indeed a perf regression, so I have put it back in.
Comment 85•5 years ago
|
||
(In reply to Lenka Pelechova from comment #84)
- also confirmed on Talos that removal of onResize() caused indeed a perf regression, so I have put it back in.
I opened bug 1532914 in order to focus on just and only that.
This isn't something you should care if the size set by javascript doesn't interfere with your patch.
Assignee | ||
Comment 86•5 years ago
|
||
Update [03-07-19]
- fixed couple more bugs discovered by testing some edge cases
- @Honza, please review the test browser_net_headers-resize.js
Thanks. - looking into more testing now
Assignee | ||
Comment 87•5 years ago
|
||
Comment 88•5 years ago
|
||
Comment on attachment 9049450 [details] [diff] [review] resize-columns6.patch Review of attachment 9049450 [details] [diff] [review]: ----------------------------------------------------------------- Please see my inline comments Honza ::: devtools/client/netmonitor/src/actions/ui.js @@ +140,5 @@ > > /** > + * Set width of multiple columns > + * > + * TODO: doc for @param Please update the doc ::: devtools/client/netmonitor/src/assets/styles/RequestList.css @@ +180,5 @@ > + * Make sure headers are not processing any mouse > + * events. This is good for performance during dragging. > + */ > + > +.requests-list-headers.dragging { Please remove the empty line between the comment and CSS rule ::: devtools/client/netmonitor/src/components/RequestListContent.js @@ +130,1 @@ > onResize() { You can mention bug 1532914 in the comment Also fix indentation of the comment (the last *) ::: devtools/client/netmonitor/src/components/RequestListHeader.js @@ +195,5 @@ > + */ > + onStartMove() { > + // Set cursor to dragging > + const container = > + document.querySelector(".request-list-container"); This can be on one line: const container = document.querySelector(".request-list-container"); @@ +236,5 @@ > + > + // Calculate new width (according to the mouse x-position) and set to style. > + // Do not allow to set it below minWidth. > + > + const newWidth = Math.max(x - headerRefRect.left, minWidth); You can remove the empty line between the comment and the code @@ +287,5 @@ > + const visibleColumns = this.getVisibleColumns(); > + const lastColumn = visibleColumns[visibleColumns.length - 1].name; > + return (lastColumn === "waterfall") ? > + visibleColumns[visibleColumns.length - 2].name : > + lastColumn; You can simplify this code to: getCompensateHeader() { const visibleColumns = this.getVisibleColumns(); const delta = (lastColumn === "waterfall") ? 2 : 1; return visibleColumns[visibleColumns.length - delta].name; } @@ +350,5 @@ > + } > + > + /** > + * Method takes the total change in width for waterfall column > + * and distributes it among all other columns. Retuns an array Typo: Retuns -> Returns @@ +360,5 @@ > + const colWidth = headerRef.getBoundingClientRect().width; > + return colWidth; > + }); > + > + // Devide changeInWidth among all columns but waterfall (that's why -1). Typo: Devide -> Divide @@ +411,5 @@ > + totalPercent += +widthFromStyle; // + converts it to a number > + }); > + > + // Do not update if total percent is from 99-101% or when it is 0 > + // - it means all columns are currently hidden (e.g. other panel is opened). nit: `(e.g. other panel is opened).` -> `(e.g. other panel is currently selected).` ::: devtools/client/netmonitor/src/create-store.js @@ +76,5 @@ > return state; > } > > /** > + * Get column data (width, min-width) nit: `Get column data` -> `Get columns data` ::: devtools/client/netmonitor/test/browser_net_headers-resize.js @@ +7,5 @@ > + * Tests resizing of columns in NetMonitor. > + */ > +add_task(async function() { > + // Reset visibleColumns so we only get the default ones > + // and not all thatare set in head.js nit: `thatare` -> `that are` @@ +65,5 @@ > + checkSumOfVisibleColumns(columnsData, visibleColumns); > + checkAllColumnsChanged(columnsData, oldColumnsData, visibleColumns); > + > + // 3. Check that all rows have the right column sizes. > + info("Checking alignement of columns and headers..."); typo: algnement -> alignment
Assignee | ||
Comment 89•5 years ago
|
||
Update [03-08-19]
- the functionality of resizing columns is hidden behind the pref
devtools.netmonitor.features.resizeColumns
where true means enabled. - the push to Try is green
- Talos results are slightly optimistic because the functionality of resizing is disabled (pref is turned off)
- for running all the tests however -> we turn the pref on
- comments from last code review are fixed
Assignee | ||
Comment 90•5 years ago
|
||
Adding feature to netmonitor for resizing of columns. In this patch the functionality is hidden behind the pref devtools.netmonitor.features.resizeColumns. This feature is currently turned off - false.
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Comment 91•5 years ago
|
||
Pushed by jodvarko@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/13115d00f301 Introduce column resizer in request list; r=Honza
Comment 92•5 years ago
|
||
bugherder |
Updated•5 years ago
|
Updated•5 years ago
|
Comment 94•5 years ago
|
||
Verified - fixed on latest Firefox Beta 67.0b3 (64-bit) on Windows 7/10, Mac OS 10.13, Ubuntu 16.04.
The columns of the request list can now be resized and reset. Tested by switching on the pref mentioned in Comment 89.
Comment 95•4 years ago
|
||
Added:
You can also change the width of the columns to help make the information you are looking for easier to view. The Reset Columns command on the context menu also resets the width of the columns to the default values.
Made a video showing the feature, uploaded to YouTube, and linked to it from the page.
Added the feature to the release notes as well.
Description
•