Closed
Bug 1431132
Opened 7 years ago
Closed 7 years ago
Prevent reflowing the whole request list when adding/updating one request
Categories
(DevTools :: Netmonitor, enhancement, P2)
DevTools
Netmonitor
Tracking
(firefox60 fixed)
RESOLVED
FIXED
Firefox 60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: ochameau, Assigned: ochameau)
References
Details
Attachments
(3 files, 2 obsolete files)
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.
Assignee | ||
Comment 1•7 years ago
|
||
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.
Updated•7 years ago
|
Priority: -- → P2
Comment 2•7 years ago
|
||
Oh, fixing this could be nice perf win!
Honza
Assignee | ||
Comment 3•7 years ago
|
||
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
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
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 | ||
Updated•7 years ago
|
Assignee: nobody → poirot.alex
Comment hidden (mozreview-request) |
Comment 7•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 8•7 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 10•7 years ago
|
||
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.
Assignee | ||
Comment 11•7 years ago
|
||
mozreview-review |
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.
Assignee | ||
Comment 12•7 years ago
|
||
https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=d97c43134c24c5ecb02e46b064cea70d35d10bbd&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&showOnlyConfident=1&framework=1&selectedTimeRange=172800
The last version makes 11.8% faster on complicated.requestsFinished!
Comment 13•7 years ago
|
||
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
Comment 14•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Comment hidden (mozreview-request) |
Comment 16•7 years ago
|
||
mozreview-review |
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]
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Attachment #8953195 -
Attachment is obsolete: true
Updated•7 years ago
|
Attachment #8953217 -
Flags: review?(odvarko)
Comment 18•7 years ago
|
||
mozreview-review |
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)
Comment 19•7 years ago
|
||
Hi!
Sorry, I confused IDs of the bugs. While I indented to relate to 1434855, I mistakenly referenced this one. I'll fix this :)
Updated•7 years ago
|
Attachment #8953217 -
Attachment is obsolete: true
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•