We discussed at length in bug 1416714 the cost of automatic scroll to bottom feature in netmonitor. Profiles show that isScrolledToBottom method is slow and called a lot. bug 1416714 comment 15 describes potential ways to optimize this.

The challenge with Julien's idea (bug 1416714 comment 15) is that after all it only moves the reflow/call to isScrolledToBottom from componentWillUpdate to scroll event processing. That's because scroll events also fire when we append a new request/line. So that doesn't reduce the number of reflows. Also, having the reflows done from componentWillUpdate or within scroll event listener doesn't seem to make any difference in term of perf. It would be improving things if we can somehow make the difference between a user scroll and a DOM change. But most of the attributes of scroll event object are unset... making it hard to filter the events. In this patch I set a flag on componentWillUpdate, but this is weak and subject to races. We could use `wheel` event instead. This one is fired only on user interaction and comes with a deltaY attribute. This attribute helps calling isScrolledToBottom only when it is strictly necessary and reduces even more the number of reflows during user scroll! But this event doesn't fire when the user grabs the scrollbar and so we still have to listen for scroll event...

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=c7a68fdb0183368932d909dd91e821cc52c1b9b2&newProject=try&newRevision=1eb737d4f2fa1134570b4af48894e67753dcb9c3&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&showOnlyImportant=1&framework=1 DAMP reports a 4% improvement on complicated.requestsFinished. That sounds great. Last try from bug 1416714 comment 7 reported a 3% win when disabling scroll-to-bottom completely. So it looks like, with such patch, this feature doesn't impact performance significantly anymore. Now, as I said, there is a possible races where a user scroll may be ignored. So that it may keep scrolling to top even if user started scrolling. Also, it may regress performances when scrolling. It would be great to have this patch tested by various people to see if these issues are real or not.

Comment on attachment 8941879 [details] Bug 1429824 - Prevent reflows related to scroll to bottom feature. Julien, Jan, Could you give this patch a try and tell me if you see anything suspicious? Big newspaper websites like cnn.com/lemonde.fr are good tests as they keep dispatching requests for a while!

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=c7a68fdb0183368932d909dd91e821cc52c1b9b2&newProject=try&newRevision=da4044279a633e2ed69a2e70fc3de63f6a96463f&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&showOnlyImportant=1&framework=1 Here is a try run that completely disable scroll to bottom. We get a 4.33% win, compared to 4.16% with the current patch, so it looks pretty good regarding netmonitor performance on reload. Now, again, scroll performance may have regressed and DAMP doesn't track that.

Comment on attachment 8941879 [details] Bug 1429824 - Prevent reflows related to scroll to bottom feature. https://reviewboard.mozilla.org/r/212096/#review218102 ::: devtools/client/netmonitor/src/components/RequestListContent.js:190 (Diff revision 1) > } > > /** > * Scroll listener for the requests menu view. > */ > - onScroll() { > + onScroll(event) { nit: event is unused, please remove it. ::: devtools/client/netmonitor/src/components/RequestListContent.js:199 (Diff revision 1) > + // To prevent slow reflows done in isScrolledToBottom. > + if (this.ignoreNextScroll) { > + this.ignoreNextScroll = false; > + return; > + } > + this.scrolledManually = !this.isScrolledToBottom(); Nice trick! Reducing isScrolledToBottom() can decrease reflow accordingly.

Comment on attachment 8941879 [details] Bug 1429824 - Prevent reflows related to scroll to bottom feature. https://reviewboard.mozilla.org/r/212096/#review218160 From my tests it seems to work fine. Here are some observations, I tested with various heavy sites like lemonde (like you suggested) or thedailybeast.com which are request-heavy: 1. Thanks to Async Pan Zoom (APZ) scrolling the netmonitor list stays smooth even if we have sync reflow -- but when the list is bigger we do see some white checkerboarding while the reflow happens. I believe this is fine as I don't think users are scrolling frenetically while looking at this list. 2. Scrolling the page itself with the scrollbar with the netmonitor opened seems to suffer from the reflows, even async ones. At first the scroll stays smooth but when a lot of requests are in the table we can see big lags. I believe this can be different when we scroll using the mousewheel or the trackpad, or on different OS (I'm on Linux) because when async pan zoom is used I think we don't see these lags at all; so YMMV. But I think this is worse without your patch. ::: devtools/client/netmonitor/src/components/RequestListContent.js:99 (Diff revision 1) > interactive: true > }); > + // Set a flag on "scroll" event, used in componentWillUpdate, in order to help > + // knowing if we should keep automatically scrolling to bottom without doing > + // too many reflows. > + this.scrolledManually = false; I don't know this code, so it will just be a not of advice: you'll need to reset this flag when we clear the view (either when the user does it or when we reload the page for example). I don't know if we get a new mounted component in that case ? ::: devtools/client/netmonitor/src/components/RequestListContent.js:109 (Diff revision 1) > componentWillUpdate(nextProps) { > // Check if the list is scrolled to bottom before the UI update. > // The scroll is ever needed only if new rows are added to the list. > const delta = nextProps.displayedRequests.size - this.props.displayedRequests.size; > - this.shouldScrollBottom = delta > 0 && this.isScrolledToBottom(); > + this.shouldScrollBottom = delta > 0 && !this.scrolledManually; > + this.ignoreNextScroll = delta > 0; nit: we don't use the delta value, so maybe just keep a boolean like this: ``` const hasNewRows = nextProps.displayedRequests.size > this.props.displayedRequests.size; ``` another nit is: why don't you do this in `componentDidUpdate` ::: devtools/client/netmonitor/src/components/RequestListContent.js:117 (Diff revision 1) > componentDidUpdate(prevProps) { > let node = this.refs.contentEl; > // Keep the list scrolled to bottom if a new row was added > - if (this.shouldScrollBottom && node.scrollTop !== MAX_SCROLL_HEIGHT) { > + if (this.shouldScrollBottom) { > // Using maximum scroll height rather than node.scrollHeight to avoid sync reflow. > node.scrollTop = MAX_SCROLL_HEIGHT; have you tried using `scrollIntoView` yet? ::: devtools/client/netmonitor/src/components/RequestListContent.js:199 (Diff revision 1) > + // To prevent slow reflows done in isScrolledToBottom. > + if (this.ignoreNextScroll) { > + this.ignoreNextScroll = false; > + return; > + } > + this.scrolledManually = !this.isScrolledToBottom(); One thing I've seen in some react code (in componentDidUpdate mostly but this can apply in other places) is to wrap things that might trigger sync reflow with requestAnimationFrame, so that a reflow should happen asynchronously. eg: ``` onScroll() { ... requestAnimationFrame(() => { this.scrolledManually = !this.isScrolledToBottom(); }); } ```

To answer this from a previous comment: > The challenge with Julien's idea (bug 1416714 comment 15) is that after all it only moves the reflow/call to isScrolledToBottom from componentWillUpdate to scroll event processing. Scrolling is made smooth thanks to APZ so reflows are much less visible. Moreover a user is unlikely to scroll much while a page he's analyzing is loading -- he can scroll but not frenetically. Lastly we must throttle scroll events (for example with requestAnimationFrame). (Note: I confusedly thought React does this if we use React's event mechanism, but I don't see anything in the doc about this. And anyway we don't use React for the scroll event (does somebody know why?)).

Comment on attachment 8941879 [details] Bug 1429824 - Prevent reflows related to scroll to bottom feature. (In reply to Alexandre Poirot [:ochameau] from comment #4) > Comment on attachment 8941879 [details] > Bug 1429824 - Prevent reflows related to scroll to bottom feature. > > Julien, Jan, Could you give this patch a try and tell me if you see anything > suspicious? Big newspaper websites like cnn.com/lemonde.fr are good tests as > they keep dispatching requests for a while! I was testing both with/without the patch on cnn.com and thedailybeast.com I am not seeing and wrong behavior and the scrolling (perceived) performance seems to be a bit better (not worse). Sometimes is hard to scroll to bottom (and activate the 'scroll to bottom' feature). After releasing the scroll-thumb the content doesn't stick at the bottom. But, I was experiencing this even before (without the patch). F+, I don't see any reason to block this. Thanks, Honza

(In reply to Julien Wajsberg [:julienw] from comment #7) > Comment on attachment 8941879 [details] > Bug 1429824 - Prevent reflows related to scroll to bottom feature. > > https://reviewboard.mozilla.org/r/212096/#review218160 > > From my tests it seems to work fine. > > Here are some observations, I tested with various heavy sites like lemonde > (like you suggested) or thedailybeast.com which are request-heavy: > > 1. Thanks to Async Pan Zoom (APZ) scrolling the netmonitor list stays smooth > even if we have sync reflow -- but when the list is bigger we do see some > white checkerboarding while the reflow happens. I believe this is fine as I > don't think users are scrolling frenetically while looking at this list. You keep thinking you are on FxOS with content processes and APZ, but we are not ;) netmonitor runs in chrome, in parent processes and I don't think we have APZ there. > ::: devtools/client/netmonitor/src/components/RequestListContent.js:99 > (Diff revision 1) > > interactive: true > > }); > > + // Set a flag on "scroll" event, used in componentWillUpdate, in order to help > > + // knowing if we should keep automatically scrolling to bottom without doing > > + // too many reflows. > > + this.scrolledManually = false; > > I don't know this code, so it will just be a not of advice: you'll need to > reset this flag when we clear the view (either when the user does it or when > we reload the page for example). I don't know if we get a new mounted > component in that case ? componentDidMount is called everytime the request list is emptied. > ::: devtools/client/netmonitor/src/components/RequestListContent.js:109 > (Diff revision 1) > > componentWillUpdate(nextProps) { > > // Check if the list is scrolled to bottom before the UI update. > > // The scroll is ever needed only if new rows are added to the list. > > const delta = nextProps.displayedRequests.size - this.props.displayedRequests.size; > > - this.shouldScrollBottom = delta > 0 && this.isScrolledToBottom(); > > + this.shouldScrollBottom = delta > 0 && !this.scrolledManually; > > + this.ignoreNextScroll = delta > 0; > > nit: we don't use the delta value, so maybe just keep a boolean like this: > ``` > const hasNewRows = nextProps.displayedRequests.size > > this.props.displayedRequests.size; > ``` > > another nit is: why don't you do this in `componentDidUpdate` We don't have `nextProps` in componentDidUpdate, so we can't compute this `delta` thing. > > ::: devtools/client/netmonitor/src/components/RequestListContent.js:117 > (Diff revision 1) > > componentDidUpdate(prevProps) { > > let node = this.refs.contentEl; > > // Keep the list scrolled to bottom if a new row was added > > - if (this.shouldScrollBottom && node.scrollTop !== MAX_SCROLL_HEIGHT) { > > + if (this.shouldScrollBottom) { > > // Using maximum scroll height rather than node.scrollHeight to avoid sync reflow. > > node.scrollTop = MAX_SCROLL_HEIGHT; > > have you tried using `scrollIntoView` yet? No, but this in independant of this bug/patch. Here, I would like to focus on the code that do reflows just to compute *if* we should scroll to bottom. We should look into that independantly. See if that make any difference. > ::: devtools/client/netmonitor/src/components/RequestListContent.js:199 > (Diff revision 1) > > + // To prevent slow reflows done in isScrolledToBottom. > > + if (this.ignoreNextScroll) { > > + this.ignoreNextScroll = false; > > + return; > > + } > > + this.scrolledManually = !this.isScrolledToBottom(); > > One thing I've seen in some react code (in componentDidUpdate mostly but > this can apply in other places) is to wrap things that might trigger sync > reflow with requestAnimationFrame, so that a reflow should happen > asynchronously. > > eg: > ``` > onScroll() { > ... > requestAnimationFrame(() => { > this.scrolledManually = !this.isScrolledToBottom(); > }); > } > ``` Unfortunately here, it is very important to do it synchronously :/ Otherwise it increase the existing race in this patch: * onScroll will fire for a user event scrolling up, we should stop immediatly scrolling to bottom * instead, of setting scrolledManually to false immediately, it is postpone to later. Instead, a new request slices in. scrolledManually is still false, we do scroll to bottom * the animation frame is over, we try updating scrolledManually, but as the new request forced scrolling to bottom, scrolledManually stays false. => We ignored used event. This scenario is the typical race this patch may introduce. Now, may be this race never happens thanks to reflow being synchronous, but I wouldn't be surprised if for some reason, or only on some platform, reflow event is slightly different and introduces races.

(In reply to Alexandre Poirot [:ochameau] from comment #10) > (In reply to Julien Wajsberg [:julienw] from comment #7) > > > > 1. Thanks to Async Pan Zoom (APZ) scrolling the netmonitor list stays smooth > > even if we have sync reflow -- but when the list is bigger we do see some > > white checkerboarding while the reflow happens. I believe this is fine as I > > don't think users are scrolling frenetically while looking at this list. > > You keep thinking you are on FxOS with content processes and APZ, but we are > not ;) > netmonitor runs in chrome, in parent processes and I don't think we have APZ > there. I'm not thinking about FxOS at all :) I see that the scroll stays smooth, maybe it isn't APZ but this is what I see, on Linux at least :) (and I think Linux is the worst platform for smoothness, as a lot more effort is done on Mac and Windows). > > > ::: devtools/client/netmonitor/src/components/RequestListContent.js:109 > > (Diff revision 1) > > > componentWillUpdate(nextProps) { > > > // Check if the list is scrolled to bottom before the UI update. > > > // The scroll is ever needed only if new rows are added to the list. > > > const delta = nextProps.displayedRequests.size - this.props.displayedRequests.size; > > > - this.shouldScrollBottom = delta > 0 && this.isScrolledToBottom(); > > > + this.shouldScrollBottom = delta > 0 && !this.scrolledManually; > > > + this.ignoreNextScroll = delta > 0; > > > > nit: we don't use the delta value, so maybe just keep a boolean like this: > > ``` > > const hasNewRows = nextProps.displayedRequests.size > > > this.props.displayedRequests.size; > > ``` > > > > another nit is: why don't you do this in `componentDidUpdate` > > We don't have `nextProps` in componentDidUpdate, so we can't compute this > `delta` thing. componentWillUpdate happens before the update: the parameter is "nextProps", this.props are the prevProps. componentDidUpdate happens after the update: the parameter is "prevProps", this.props are the nextProps. So you _can_ compute the delta :) and this would make the patch simpler. > > Unfortunately here, it is very important to do it synchronously :/ > Otherwise it increase the existing race in this patch: > * onScroll will fire for a user event scrolling up, we should stop > immediatly scrolling to bottom > * instead, of setting scrolledManually to false immediately, it is postpone > to later. Instead, a new request slices in. scrolledManually is still false, > we do scroll to bottom => maybe this request to scroll could be in a requestAnimationFrame too. But this isn't in the scope for this patch so let's look at this elsewhere. > * the animation frame is over, we try updating scrolledManually, but as the > new request forced scrolling to bottom, scrolledManually stays false. => We > ignored used event. > > This scenario is the typical race this patch may introduce. > Now, may be this race never happens thanks to reflow being synchronous, but > I wouldn't be surprised if for some reason, or only on some platform, reflow > event is slightly different and introduces races.

(In reply to Julien Wajsberg [:julienw] from comment #7) > ::: devtools/client/netmonitor/src/components/RequestListContent.js:117 > > node.scrollTop = MAX_SCROLL_HEIGHT; > > have you tried using `scrollIntoView` yet? DAMP report no win: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=935966d36f6cc7146c2d99de638c4b68f43d9fc2&newProject=try&newRevision=ac1a7cb674832bba4fa0ef007f1a8a2270ce32cd&originalSignature=edaec66500db21d37602c99daa61ac983f21a6ac&newSignature=edaec66500db21d37602c99daa61ac983f21a6ac&filter=netmonitor&framework=1 Nor does the profiler. It is interesting to see that I get the very expect same duration for both! scrollIntoView: https://perfht.ml/2BeKAyI 360ms scrollTop: https://perfht.ml/2mRNULL 360ms

Comment on attachment 8941879 [details] Bug 1429824 - Prevent reflows related to scroll to bottom feature. https://reviewboard.mozilla.org/r/212096/#review220530 Thanks for the patch Alex. R+ assuming try is green Honza

Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b764ca8d82da Prevent reflows related to scroll to bottom feature. r=Honza

Backed out for failing browser chrome devtools at devtools/client/netmonitor/test/browser_net_autoscroll.js on a CLOSED TREE Push for which the failing jobs ran: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=a678eec13c6e6bf1662cc4db5719bbb413c63f6 recent failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=158014531&repo=autoland&lineNumber=3988 Backout: https://hg.mozilla.org/integration/autoland/rev/e8d981d2ce37ecf6f34909ed3bd76dd536012ef3

This patch made browser_net_autoscroll.js be intermittently failing. Here is a new patch that simplifies a bit the various variables used around scroll-to-bottom. (renamed scrolledManually to shouldScrollToBottom) It fixed the disabling of scroll to bottom when selecting a request line (it was broken and kept scrolling down) and fixed the race that made this test fail by setting ignoreNextScroll only when it is strictly needed (i.e. when we set scrollTop to MAX_SCROLL_HEIGHT). I added some more comments! Here is a handy test document to verify all this: data:text/html,<script>window.setInterval(()=>{var%20x=new%20XMLHttpRequest();%20x.open("GET",%20"http://mozilla.org",%20false);%20x.send(null);},%20250)</script>

Comment on attachment 8941879 [details] Bug 1429824 - Prevent reflows related to scroll to bottom feature. Jan, Could you look at the latest version?

Comment on attachment 8941879 [details] Bug 1429824 - Prevent reflows related to scroll to bottom feature. https://reviewboard.mozilla.org/r/212096/#review221260 Looks good, but nees rebase on mc. I am seeing: Hunk #1 FAILED at 87 1 out of 3 hunks FAILED -- saving rejects to file devtools/client/netmonitor/src/components/RequestListContent.js.rej Honza

Looks like it still fails, but only when stylo is disabled: https://treeherder.mozilla.org/#/jobs?repo=try&revision=73753e3ed0082ac6369223b5199f9b1b24a15e19&selectedJob=158640464 https://treeherder.mozilla.org/logviewer.html#?job_id=158640464&repo=try&lineNumber=7527

Rebased and re run it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a5ccad30f6709e8733b06e5b389a454229f3124 It highlights that it is intermittent and isn't specific to stylo being disabled as it occured on linux64-debug.

Comment on attachment 8950565 [details] Bug 1429824 - Always use a valid transformation against the waterwall. Without this patch, you get a tons of these error in stdout: Console message: [JavaScript Warning: "Error in parsing value for ‘transform’ after substituting variables. Generated value was ‘ scaleX(Infinity)’. Falling back to ‘initial’." {file: "chrome://devtools/content/netmonitor/src/assets/styles/RequestList.css" line: 606 column: 12267 source: " scaleX(Infinity)"}] This doesn't help fixing the intermittent, but cleans up the test output!

Comment on attachment 8950566 [details] Bug 1429824 - Try to prevent races in browser_net_autoscroll.js. But this patch seems to fix the intermittent against browser_net_autoscroll.js! https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e7672796b003b348b3e6de71aa45e8972b38f13&selectedJob=161306276

Comment on attachment 8950565 [details] Bug 1429824 - Always use a valid transformation against the waterwall. https://reviewboard.mozilla.org/r/219840/#review225942 Looks reasonable, thanks. R+ Honza

Comment on attachment 8950566 [details] Bug 1429824 - Try to prevent races in browser_net_autoscroll.js. https://reviewboard.mozilla.org/r/219842/#review225944 Ok, let's see if this helps, R+ Honza

Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cf796a41ff7d Prevent reflows related to scroll to bottom feature. r=Honza https://hg.mozilla.org/integration/autoland/rev/e3d4ec2c18b5 Always use a valid transformation against the waterwall. r=Honza https://hg.mozilla.org/integration/autoland/rev/500836846f62 Try to prevent races in browser_net_autoscroll.js. r=Honza

https://hg.mozilla.org/mozilla-central/rev/cf796a41ff7d https://hg.mozilla.org/mozilla-central/rev/e3d4ec2c18b5 https://hg.mozilla.org/mozilla-central/rev/500836846f62

Backout by ebalazs@mozilla.com: https://hg.mozilla.org/mozilla-central/rev/994a8d6eccbc Backed out 3 changesets for frequently failing devtools/client/netmonitor/test/browser_net_autoscroll.js (bug 1438412) a=backout

Backed out 3 changesets (bug 1429824) for frequently failing devtools/client/netmonitor/test/browser_net_autoscroll.js (bug 1438412) a=backout push with failures:https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=9b69cc60e5848f2f8802c911fd00771b50eed41f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&selectedJob=162383724 failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=162383724&repo=mozilla-central&lineNumber=2158 backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=994a8d6eccbcdc6106794705bd77e3ac5f031be2&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable