1570192 - New-tab page has some kinda-slow synchronous flexbox/grid layout

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect, P1)

Description

[Daniel Holbert [:dholbert]]

STR:
0. Install profiler from https://profiler.firefox.com/

1. Visit about:blank.
2. Ctrl+Shift+1 to start profiling.
3. Ctrl+T to open a new tab.
4. After a few seconds, Ctrl+Shift+2 to stop profiling.

ACTUAL RESULTS:
Profile shows a red bar for a period of jank, which includes some flexbox layout. In particular, there are some script-triggered sync reflows from an IntersectionObserver callback which checks .clientWidth, which I suspect are associated with image lazy-loading.

EXPECTED RESULTS:
I suspect the series of short script/IntersectionObserver-triggered reflows should probably be cheap or free, because layout is probably not changing much. I'm not sure whether the tiles are styled such that we can theoretically infer that the flex containers involved don't need to reflow, or if there's styling that we would need to add (along with optimizations) to reliably get that result -- but one way or another, we should try to be sure that no serious reflow work needs to happen after the initial layout.

Also/alternatively, perhaps the about:newtab code shouldn't be reading clientWidth in the IntersectionObserver callback, since that forces any of the callback's earlier restyling to be immediately flushed. We're probably not coalescing these multiple IntersectionObserver-callback-invocations' restyling at all right now, due to the fact that they're doing this flush.

Here's a profile I captured of the STR, using current Nightly: https://perfht.ml/2SWyogu (filtered for nsFlexContainerFrame)
The series of 9 reflows (each 2-3ms long) after timestamp 1.64s are the ones that I'm primarily concerned with improving/avoiding.

Comment 1

[Daniel Holbert [:dholbert]]

Actually, there's quite a bit of grid wrappers in this page, too; so if there are layout optimizations to be done here, it may involve some grid work as well.

Looking at the NewTab DOM in DevTools inspector, I see a flex container, which contains 5 nested grid containers, which contains several more nested flex containers (at least; I didn't dig too deep).

--> generifying component a bit to just "Layout"

Comment 2

[Emilio Cobos Álvarez (:emilio)]

So I guess this is the relevant code: https://searchfox.org/mozilla-central/rev/38c88cbf4be87dfa0636d15a5d3599a8ea0d1a72/browser/components/newtab/data/content/activity-stream.bundle.js#7062

Andrei, do you know why do you need to use clientWidth / clientHeight? Would it be a better idea to use IntersectionObserverEntry.boundingClientRect?

I suspect Daniel still wants to investigate a bit on the layout side, but I think AS could be doing better in this case too.

Comment 3

[Andrei Oprea [:andreio]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)

So I guess this is the relevant code: https://searchfox.org/mozilla-central/rev/38c88cbf4be87dfa0636d15a5d3599a8ea0d1a72/browser/components/newtab/data/content/activity-stream.bundle.js#7062

Andrei, do you know why do you need to use clientWidth / clientHeight? Would it be a better idea to use IntersectionObserverEntry.boundingClientRect?

It's used to request the appropriate sized image when the content scrolls into view.

Comment 4

[Andrei Oprea [:andreio]]

:gvn any idea if this can be optimized?

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to Andrei Oprea [:andreio] from comment #3)

It's used to request the appropriate sized image when the content scrolls into view.

Sure, I mean that, modulo dynamic resizes, the IntersectionObserverEntry should already have the client rect information, so you should be able to just use entry.boundingClientRect.width rather than element.clientWidth.

Comment 6

[Scott [:thecount] Downe]

I'll test and run some profiling using entry.boundingClientRect.width/height that's a great bit of info, thanks!

We were looking for a solution to doing this better, and kept trying to figure out a way to have the sizes pre defined, but it would cause some other issues, and entry.boundingClientRect.width/height just might solve it for us.

Comment 7

[Scott [:thecount] Downe]

Yeah that's much better, just fixing up a test it broke (we don't stub entry.boundingClientRect) but that seems straight forward, and I can probably push up a pretty simple patch :)

Comment 8

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/5214

Comment 9

[Ed Lee :Mardak]

dholbert, I believe thecount did some initial profiling with PR5214 and saw improvements.

If you want to apply to try out the change locally, simplest in this case is probably just modify the bundle.js then ./mach run:

diff --git a/browser/components/newtab/data/content/activity-stream.bundle.js b/browser/components/newtab/data/content/activity-stream.bundle.js
--- a/browser/components/newtab/data/content/activity-stream.bundle.js
+++ b/browser/components/newtab/data/content/activity-stream.bundle.js
@@ -7057,9 +7057,11 @@ class DSImage_DSImage extends external_React_default.a.PureComponent {
   onSeen(entries) {
     if (this.state) {
-      if (entries.some(entry => entry.isIntersecting)) {
+      const entry = entries.find(e => e.isIntersecting);
+
+      if (entry) {
         if (this.props.optimize) {
           this.setState({
-            containerWidth: external_ReactDOM_default.a.findDOMNode(this).clientWidth,
-            containerHeight: external_ReactDOM_default.a.findDOMNode(this).clientHeight
+            containerWidth: entry.boundingClientRect.width,
+            containerHeight: entry.boundingClientRect.height
           });
         }

Alternatively, you could apply the .jsx changes then follow the instructions here to generate the bundle:
https://firefox-source-docs.mozilla.org/browser/components/newtab/docs/

Comment 10

[Scott [:thecount] Downe]

I wonder if we should split this bug up?

There were some easy wins with entry.boundingClientRect and was clear to me that it not only is it something we can land right away, it might be an uplift request.

There were some other good ideas about other things we can investigate in this bug.

We can keep this bug open as a sort of meta bug, and file new bugs for entry.boundingClientRect fixes, layout optimizations, or grid work.

Although if we suspect entry.boundingClientRect is enough of the work and can sufficiently close this bug, we can do that too.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

(In reply to Scott [:thecount] Downe from comment #10)

I wonder if we should split this bug up?

Yeah, probably Daniel wanted to investigate the slow reflows themselves, to see if there's something to optimize in Gecko itself. We can probably revert that patch once it lands or something of that sort.

There were some easy wins with entry.boundingClientRect and was clear to me that it not only is it something we can land right away, it might be an uplift request.

Awesome! ^.^

Comment 12

[Daniel Holbert [:dholbert]]

Thanks, Scott & Emilio! I field https://bugzilla.mozilla.org/show_bug.cgi?id=1570770 with another incremental-layout scenario on the New-tab page -- let's use that one to dig into possible gecko changes here, and morph/reduce this bug here to be focused on the problem that the patch is addressing (removing the sync layout flushes during pageload).

Comment 13

[Scott [:thecount] Downe]

I think now that these are split up, I'll flag this one as a potential uplift.

Comment 14

[Scott [:thecount] Downe]

[Tracking Requested - why for this release]: Performance improvement with potential UX and revenue impact.

Comment 15

[Ciprian Muresan [:cmuresan], Ecosystem QA]

https://hg.mozilla.org/mozilla-central/rev/49f075579f00

Comment 16

[Marius Coman [:mcoman], Ecosystem QA - PTO until 8th of November, for any requests ping :cmuresan]

I have verified that this issue is no longer reproducible with the latest Firefox Nightly (70.0a1 Build ID - 20190804214813) installed, on Windows 10 x64, Arch Linux and Mac 10.14.5. Now, there are no red bars displayed in the profile.

Comment 17

[Scott [:thecount] Downe]

Attachment: Uplift 1570192 - Potential perf fixes in DS image onSeen

Comment 18

[Scott [:thecount] Downe]

Comment on attachment 9084031 [details]
Uplift 1570192 - Potential perf fixes in DS image onSeen

Beta/Release Uplift Approval Request

• User impact if declined: UX and revenue impact because of performance regression.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: STR:

0. Install profiler from https://profiler.firefox.com/

   Visit about:blank.
   Ctrl+Shift+1 to start profiling.
   Ctrl+T to open a new tab.
   After a few seconds, Ctrl+Shift+2 to stop profiling.

EXPECTED RESULTS:
I suspect the series of short script/IntersectionObserver-triggered reflows should probably be cheap or free, because layout is probably not changing much. I'm not sure whether the tiles are styled such that we can theoretically infer that the flex containers involved don't need to reflow, or if there's styling that we would need to add (along with optimizations) to reliably get that result -- but one way or another, we should try to be sure that no serious reflow work needs to happen after the initial layout.

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Small set of changes, tests, and verified on nightly.
• String changes made/needed: none

Comment 19

[Marius Coman [:mcoman], Ecosystem QA - PTO until 8th of November, for any requests ping :cmuresan]

I have verified that this issue is no longer reproducible with the latest Firefox Beta(69.0b12 Build ID - 20190807220259) installed, on Windows 10 x64, Arch Linux and Mac 10.14.5. Now, there are no red bars displayed in the profile.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

I'm confused, this doesn't appear to have been uplifted to 69 yet?

Comment 21

[Scott [:thecount] Downe]

Hm, yeah I thought it was waiting on approval before we can uplift. (might be missing something in that process though)

The patch is r+'ed and then I added the attachment for Beta/Release Uplift Approval Request.

I see that I set a qe-verify+ but I don't remember explicitly doing that.

Also performance tests can have quite a bit of noise and results from test to test, so I'm not super surprised if a profile in beta produced reasonable results even without this landed.

I usually verify this sort of thing with screen recordings and count the frames, but it's super time consuming.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Setting 69 back to affected for now under the assumption that there was a misunderstanding around the verification here.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9084031 [details]
Uplift 1570192 - Potential perf fixes in DS image onSeen

Fixes a perf issue on the New Tab Page. Approved for 69.0b13. Note that this is a branch-specific patch and we shouldn't graft what landed on m-c.

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/6b5f7122a0b3

Comment 25

[Marius Coman [:mcoman], Ecosystem QA - PTO until 8th of November, for any requests ping :cmuresan]

I have verified that this issue is no longer reproducible with the latest Firefox Beta (69.0b13 Build ID - 20190812173625) installed, on Windows 10 x64, Arch Linux and Mac 10.14.5. In order to verify this issue, I've recorded two profiles one with Firefox Beta 69.0b12 and one with Firefox Beta 69.0b13 and compared the results.