Closed Bug 1570192 Opened 4 months ago Closed 3 months ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 70
Iteration:
70.2 - Jul 22 - Aug 4
Tracking Status
firefox69 + verified
firefox70 + verified

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 2 open bugs)

Details

(Keywords: github-merged, perf, Whiteboard: [qf:p2:pageload])

Attachments

(2 files)

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.

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"

Component: Layout: Flexbox → Layout
Summary: New-tab page has some kinda-slow synchronous flexbox layout → New-tab page has some kinda-slow synchronous flexbox/grid layout

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.

Flags: needinfo?(andrei.br92)

(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.

Flags: needinfo?(andrei.br92)

:gvn any idea if this can be optimized?

Flags: needinfo?(gsuntop)

(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.

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.

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 :)

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/

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.

(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! ^.^

Whiteboard: [qf] → [qf:p2:pageload]

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).

Component: Layout → New Tab Page
Product: Core → Firefox
See Also: → 1570770

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

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

Blocks: 1570745
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED

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.

Status: RESOLVED → VERIFIED
Flags: needinfo?(gsuntop)
Iteration: --- → 70.2 - Jul 22 - Aug 4
Keywords: github-merged
Target Milestone: --- → Firefox 70

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:
  1. 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
Attachment #9084031 - Flags: approval-mozilla-beta?
Flags: qe-verify+

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.

Flags: qe-verify+

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

Flags: needinfo?(mcoman)

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.

Priority: P2 → P1

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

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.

Attachment #9084031 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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.

Flags: needinfo?(mcoman)
You need to log in before you can comment on or make changes to this bug.