1524375 - Firefox hangs on Google News for several seconds

Status: RESOLVED DUPLICATE of bug 1524056

(Core :: Layout, defect)

Description

[Sören Hentzschel]

Attachment: googlenews.png

Firefox hangs on Google News for several seconds. First reported in the German speaking Firefox support and confirmed by multiple users. It was able to reproduce the issue as guest but not as logged-in user.

STR:

1. Open https://news.google.com/?hl=de&gl=AT&ceid=AT:de
2. Click on "Weitere ansehen" below a topic
3. => Performance is okay
4. Select a topic on the left side, for example "Wissenschaft & Technik"
5. Repeat step 2

Expected:

Element expands immediately.

Actual.

Element needs a few seconds to expand.

I attached a screenshot which shows that Firefox spends a few seconds in Layout with a framerate of ~ 0 FPS.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Hallo :)

I tried to repro this but I couldn't.

If you can reproduce this consistently, is there any chance you could paste a link with a profile taken from https://perf-html.io/ instead of devtools? That gives us much more information about what's executing at a given time.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Ah, so I think I caught something interesting while browsing on that website:

http://bit.ly/2WA8VuK

There are a few interesting bits there. Mats, looks like a massively nested grid + flex reflow on the top of the stack, where most of the time is spent doing measuring reflows... Do we have a bug for something like that? We do have optimizations for that in Flexbox1, not sure we have them for Grid. It took a bit to take those right, but probably most of the stuff the grid and flex code depends on / read from the measuring reflow is similar?

Comment 3

[Emilio Cobos Álvarez (:emilio)]

I was able to repro that when scrolling down and loading more items, IIRC.

Comment 4

[Sören Hentzschel]

Hi,

I captured a performance profile:
https://perf-html.io/public/6417861d7caa3fc22a55cb0f3e6250b2af468f49/calltree/?globalTrackOrder=11-12-13-0-1-2-3-4-5-6-7-8-9-10&hiddenGlobalTracks=1-2-3-4-5-6-7-8-9&thread=11&v=3

Comment 5

[Mats Palmgren (inactive)]

We don't cache measuring-reflow results in Grid yet. Doing that would
likely help a lot in a case like this. I would guess measuring flex/grid
items is similar, so we can probably re-use some of that code.
I don't really understand what the last paragraph is about though:
https://searchfox.org/mozilla-central/rev/6e3cc153566f5f288ae768a2172385b8436d61dd/layout/generic/nsFlexContainerFrame.cpp#1718-1722

The measuring-reflow code for grid items is here:
https://searchfox.org/mozilla-central/rev/6e3cc153566f5f288ae768a2172385b8436d61dd/layout/generic/nsGridContainerFrame.cpp#3353

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #5)

I don't really understand what the last paragraph is about though:
https://searchfox.org/mozilla-central/rev/6e3cc153566f5f288ae768a2172385b8436d61dd/layout/generic/nsFlexContainerFrame.cpp#1718-1722

It's possible that paragraph is off-base... It's trying to justify why we need to include the item's ComputedBSize as part of the cache key. Superficially, it might seem like we wouldn't need to bother checking it, because we already clear the cached measurements whenever 'height' and 'width' change (or whenever anything invalidates our cached intrinsic measurements).

From looking at the reftest that landed with the patch that added this comment (and started including ComputedBSize in the cache key), I suspect the real reason is that:

• the flex item might be measured twice in the same overall reflow, first in an "indefinite percent-basis" scenario and then later in a "definite percent-basis" scenario.
• And if the flex item happens to involve percent heights, then the definiteness of the percent basis will influence the resolution of those heights.
• So, the same element with the same styles may produce a mComputedBSize of NS_INTRINSICSIZE in the first of those reflows, vs. some resolved percentage value in the second of those reflows. And it's invalid for us to skip over the second reflow and reuse the measurement from the first one. So, checking for ReflowInput::ComputedBSize differences (between cached value's cache key & the current ReflowInput) will help us avoid making that mistake.

Comment 7

[Daniel Holbert [:dholbert]]

Let's dupe this to bug 1524056 (filed slightly earlier & that's the one that the QF performance-squad ended up triaging)