Closed Bug 1431132 Opened 6 years ago Closed 6 years ago

Prevent reflowing the whole request list when adding/updating one request

Categories

(DevTools :: Netmonitor, enhancement, P2)

Product:
DevTools
Component:
Netmonitor
Type:
enhancement

Tracking

(firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

Attachments

(3 files, 2 obsolete files)

screen record
1.32 MB, image/gif
Details
Bug 1431132 - Prevent repainting the whole request list by sizing the request table manually.
59 bytes, text/x-review-board-request
Honza
: review+
Details
screen record with fixes
1.33 MB, image/gif
Details 
Today, whenever we append a new request or update an already displayed one with new lazily fetched data, the whole request list is repainted.
You can see that via nglayout.debug.paint_flashing_chrome pref.
Attached image screen record — 
With the pref turned on, you can that the whole request list repaint when one is added. Also, if you scroll up and add a request, the request list is also fully repainted.
Oh, fixing this could be nice perf win!
Honza
Setting a fixed size on the waterfall helps preventing reflow, but that is not enough.
I'll attach some more patches tomorrow.
Depends on: 1430723
With this patch *and* bug 1430723's one, we get another 7% improvement in complicated.requestsFinished:
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=4bfe58b98d04&newProject=try&newRevision=aa81c3e3eb26b2ab9ee1ed5ec64ef1177495608a&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&showOnlyConfident=1&framework=1

See this screen record, where you can see that each already displayed record is never repainted. It is even more obvious when you scroll up, there is only the scrollbar and the waterfall that repaint.

I think the modification made to RequestListContent.js is trivial and is most likely going to be necessary for bug 1358414 and resizable columns.
The really disturbing modification in this patch is the removal of `display: table-row;` on `.request-list-item`. It doesn't affect the layout from what I can see, but it is a key to stop having these unecessary repaint...
I think we could live with that if we start working on bug 1358414 soon.
In the meantime, it will bring a very substantial performance improvement.
Assignee: nobody → poirot.alex
Comment on attachment 8943591 [details]
Bug 1431132 - Prevent repainting the whole request list by sizing the request table manually.

https://reviewboard.mozilla.org/r/213962/#review220024

This looks promising.

R+, but please see my inline comment.

Thanks Alex!
Honza

::: devtools/client/netmonitor/src/assets/styles/RequestList.css:608
(Diff revision 2)
>  
>  /* Request list item */
>  
>  .request-list-item {
>    position: relative;
> -  display: table-row;
> +/*  display: table-row;*/

Please append a comment explaining why the prop is commented out.

::: devtools/client/netmonitor/src/components/RequestListContent.js:129
(Diff revision 2)
> +    window.removeEventListener("resize", this.onResize);
> +  }
> +
> +  onResize() {
> +    let parent = this.refs.contentEl.parentNode;
> +    this.refs.contentEl.style.height = parent.offsetHeight + "px";

Since we have the resize handler now, we could calculate width of the Waterfall column(in px) here (see my comment in bug 1430723)
Attachment #8943591 - Flags: review?(odvarko) → review+
(In reply to Jan Honza Odvarko [:Honza] from comment #7)
> ::: devtools/client/netmonitor/src/components/RequestListContent.js:129
> (Diff revision 2)
> > +    window.removeEventListener("resize", this.onResize);
> > +  }
> > +
> > +  onResize() {
> > +    let parent = this.refs.contentEl.parentNode;
> > +    this.refs.contentEl.style.height = parent.offsetHeight + "px";
> 
> Since we have the resize handler now, we could calculate width of the
> Waterfall column(in px) here (see my comment in bug 1430723)

This is more complex for width. In this patch I only control height from JS.
Height on elements only changes on resize, whereas width changes also when you open detail panels (header, cookies, ...).
So you would also have to listen for more than resize event. Then would you use redux action + state like waterfallWidth? Also we would have to listen for splitbox event as we have to resize column width everyime we resize the network detail sidebar.
It looks like with latest tip, the reflow fix only need the fixed width patch in order to work. So making it depend on bug 1431395.
Depends on: 1431395
No longer depends on: 1430723
Comment on attachment 8943591 [details]
Bug 1431132 - Prevent repainting the whole request list by sizing the request table manually.

https://reviewboard.mozilla.org/r/213962/#review220262

::: devtools/client/netmonitor/src/assets/styles/RequestList.css:68
(Diff revision 3)
>  }
>  
>  .requests-list-contents {
>    display: table-row-group;
>    position: absolute;
> -  top: 0;
> +  width: 100%;

I had to introduce this `width: 100%` to unbreak detail sidebars (header, cookies, ...). Otherwise sidebar was overlapping the request list.
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/58e45c38f6c8
Prevent repainting the whole request list by sizing the request table manually. r=Honza
https://hg.mozilla.org/mozilla-central/rev/58e45c38f6c8
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Depends on: 1432834
Depends on: 1434885
Comment on attachment 8953195 [details]
Bug 1431132 - Netmonitor list of requests height set to 100% to prevent overrides

https://reviewboard.mozilla.org/r/222480/#review228410


Code analysis found 3 defects in this patch:
 - 3 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/netmonitor/src/components/RequestListContent.js:129
(Diff revision 1)
>      this.tooltip.stopTogglingOnHover();
>      window.removeEventListener("resize", this.onResize);
>    }
>  
>    onResize() {
> -    let parent = this.refs.contentEl.parentNode;
> +    //let parent = this.refs.contentEl.parentNode;

Error: Expected space or tab after '//' in comment. [eslint: spaced-comment]

::: devtools/client/netmonitor/src/components/RequestListContent.js:130
(Diff revision 1)
>      window.removeEventListener("resize", this.onResize);
>    }
>  
>    onResize() {
> -    let parent = this.refs.contentEl.parentNode;
> -    this.refs.contentEl.style.height = parent.offsetHeight + "px";
> +    //let parent = this.refs.contentEl.parentNode;
> +    //this.refs.contentEl.style.height = parent.offsetHeight - 20 + "px";

Error: Expected space or tab after '//' in comment. [eslint: spaced-comment]

::: devtools/client/netmonitor/src/components/RequestListContent.js:133
(Diff revision 1)
>    onResize() {
> -    let parent = this.refs.contentEl.parentNode;
> -    this.refs.contentEl.style.height = parent.offsetHeight + "px";
> +    //let parent = this.refs.contentEl.parentNode;
> +    //this.refs.contentEl.style.height = parent.offsetHeight - 20 + "px";
> +  }
> +
> +  componentWillReceiveProps() {

Error: Componentwillreceiveprops should be placed before componentwillupdate [eslint: react/sort-comp]
Attachment #8953195 - Attachment is obsolete: true
Attachment #8953217 - Flags: review?(odvarko)
Comment on attachment 8953217 [details]
Bug 1431132 - Netmonitor list of requests height set to 100% to prevent overflows

https://reviewboard.mozilla.org/r/222506/#review229832

Hi,
thanks for working on this!

The pach looks reasonable, but this bug is already closed & resolved.

Can you please file a new bug and:
1) mention that it's a follow up for this bug
2) attach your patch to the new bug and I'll review it

Thanks!
Honza
Attachment #8953217 - Flags: review?(odvarko)
Hi!
Sorry, I confused IDs of the bugs. While I indented to relate to 1434855, I mistakenly referenced this one. I'll fix this :)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: