1431395 - Setting fixed size for request columns improve netmonitor reflow performance

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement)

Description

[Alexandre Poirot [:ochameau]]

In bug 1421926, Fred identified that one column may have a big impact on netmonitor reflow performance. At the end, if we set a fixed width in pixels, the performance improves.

We should try to set a fixed width for all column that have an expected typical width, like most of them: status, source, method, type, transferred and size.
They all are quite small and most of them can be simply set to the size of their header title.

Comment 1

[Alexandre Poirot [:ochameau]]

Doing that improves complicated.requestsFinished by 5%:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=f09fff6464b8177de8a7eb345ffa9f1f9406e644&newProject=try&newRevision=4bfe58b98d04a8feee848aef2a04c19a7d374d27&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&showOnlyConfident=1&framework=1

Comment 2

[Alexandre Poirot [:ochameau]]

This may help doing the column resizer feature as we will end up setting size in pixels for all columns.

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1431395 - Set fixed size on all columns.

Review commit: https://reviewboard.mozilla.org/r/213958/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/213958/

Comment 4

[Alexandre Poirot [:ochameau]]

Comment on attachment 8943586 [details]
Bug 1431395 - Set fixed size on all columns.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213958/diff/1-2/

Comment 5

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

Comment on attachment 8943586 [details]
Bug 1431395 - Set fixed size on all columns.

https://reviewboard.mozilla.org/r/213958/#review219662

Thanks for the patch Alex!


Could we make the (last) waterfall column to get the rest of the space? If I make the browser window wide enough the column doesn't take all available space and the UI looks weird (screenshot coming...). Or should that be solved in bug 1430723 ?

Otherwiser fixed size for cols like Status, Method, etc. feels natural (the required widthe in these case is quite constant).

Honza

::: devtools/client/netmonitor/src/assets/styles/RequestList.css:222
(Diff revision 1)
>  /* Status column */
>  
>  .requests-list-status {
> -  width: 8%;
> +  width: 80px;
> +  min-width: 80px;
> +  max-width: 80px;

Why do we have to specify all three props (width, min-width and max-width)?

Comment 6

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

Attachment: fixed-columns.png

See the space at the end of the column list marked with red ellipse.

Honza

Comment 7

[Alexandre Poirot [:ochameau]]

(In reply to Jan Honza Odvarko [:Honza] from comment #5)
> Comment on attachment 8943586 [details]
> Bug 1431395 - Set fixed size on all columns.
> 
> https://reviewboard.mozilla.org/r/213958/#review219662
> 
> Thanks for the patch Alex!
> 
> 
> Could we make the (last) waterfall column to get the rest of the space? If I
> make the browser window wide enough the column doesn't take all available
> space and the UI looks weird (screenshot coming...). Or should that be
> solved in bug 1430723 ?

Oh I see. I imagine I should increment the percentage being used for waterfall.
Today it is 20%, but I removed many % with this patch and so waterfall doesn't fill in the void being created.
This patch shouldn't regress waterfall behavior, but I'll keep setting its size to pixels for bug 1430723.
Unless it is easier to first land bug 1430723 if that helps this one...

> ::: devtools/client/netmonitor/src/assets/styles/RequestList.css:222
> (Diff revision 1)
> >  /* Status column */
> >  
> >  .requests-list-status {
> > -  width: 8%;
> > +  width: 80px;
> > +  min-width: 80px;
> > +  max-width: 80px;
> 
> Why do we have to specify all three props (width, min-width and max-width)?

Only min-width has a visual impact, but I wasn't sure if the other would help the layout engine.
I just pushed a version with only min-width rules:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=083e3c61127f1c0e0bde21439ac32dea0876499b

Comment 8

[Alexandre Poirot [:ochameau]]

(In reply to Jan Honza Odvarko [:Honza] from comment #6)
> Created attachment 8943601 [details]
> fixed-columns.png
> 
> See the space at the end of the column list marked with red ellipse.

Wait... Isn't it already happening on m-c?
I can see the exact same behavior on my nightly. Is the waterfall even smaller with current patch?

Comment 9

[Alexandre Poirot [:ochameau]]

It looks like setting only min-width is enough to get the perf improvement.
Also, I increased waterfall from 20% to 30% and it allows it to fill the gap.
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=33bcccd7603c952cf454a642e1b3830438c91fdd&newProject=try&newRevision=37a0b7cd57c241c1155fb0c5cb9749578d1f11ec&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&showOnlyConfident=1&framework=1
Still 5% improvement.

Comment 10

[Alexandre Poirot [:ochameau]]

Comment on attachment 8943586 [details]
Bug 1431395 - Set fixed size on all columns.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213958/diff/2-3/

Comment 11

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

Comment on attachment 8943586 [details]
Bug 1431395 - Set fixed size on all columns.

https://reviewboard.mozilla.org/r/213958/#review219994

I am still seeing a gap if I maximize the browser window on my monitor (1920x1200). Don't forget to reset columns to use the default set. Also, I am wondering if fixed value for the waterfall is the right way to go (eg the gap is also there if I span the browser window over two monitors). My feeling is that we should either find more flexible solution in CSS or introduce JS resize handler that updates width of the waterfall column, so it takes rest of the space (using px). Different column should be used to fill up the space in case the waterfall is hidden.

Of course, we should verify that JS resize handler doesn't negatively impact the perf (but it should only happen whe the browser window is resized, which doesn't usually happen eg during page reload).

Honza

Comment 12

[Alexandre Poirot [:ochameau]]

Comment on attachment 8943586 [details]
Bug 1431395 - Set fixed size on all columns.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213958/diff/3-4/

Comment 13

[Alexandre Poirot [:ochameau]]

(In reply to Jan Honza Odvarko [:Honza] from comment #11)
> Comment on attachment 8943586 [details]
> Bug 1431395 - Set fixed size on all columns.
> 
> https://reviewboard.mozilla.org/r/213958/#review219994
> 
> I am still seeing a gap if I maximize the browser window on my monitor
> (1920x1200). Don't forget to reset columns to use the default set. Also, I
> am wondering if fixed value for the waterfall is the right way to go (eg the
> gap is also there if I span the browser window over two monitors). My
> feeling is that we should either find more flexible solution in CSS or
> introduce JS resize handler that updates width of the waterfall column, so
> it takes rest of the space (using px). Different column should be used to
> fill up the space in case the waterfall is hidden.

This time I tested with very large resolutions, and instead of increasing the size of the waterfall,
I increased file and domain as well. So it should look slightly better when you remove the waterfall.
But with and without the patch, we do not fill the gap that appear when removing the waterfall.
That would be something to look into in bug 1358414.

> Of course, we should verify that JS resize handler doesn't negatively impact
> the perf (but it should only happen whe the browser window is resized, which
> doesn't usually happen eg during page reload).

I would like to keep JS computation of columns for a followup.
With such CSS modification we improve performance without changing much the current behavior of netmonitor layout.

Comment 14

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

Comment on attachment 8943586 [details]
Bug 1431395 - Set fixed size on all columns.

https://reviewboard.mozilla.org/r/213958/#review220268

Looks better now, thanks!

R+

Honza

Comment 15

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=93564a0d9f1a556699cd802631dd3bce2cf9f0b4&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&showOnlyConfident=1&framework=1&selectedTimeRange=172800
Still 5% on very latest patch.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e9d182c93bc1
Set fixed size on all columns. r=Honza

Comment 17

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/e9d182c93bc1