Calculating scroll anchoring bounding rect on taskcluster log takes long time

RESOLVED FIXED in Firefox 66

Status

()

defect
P3
normal
RESOLVED FIXED
4 months ago
2 months ago

People

(Reporter: rhunt, Assigned: rhunt)

Tracking

(Blocks 1 bug)

unspecified
mozilla67
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox66 fixed, firefox67 fixed)

Details

Attachments

(2 attachments)

Assignee

Description

4 months ago

Jeff sent me this link [1]. This is similar to bug 1521579, but the patches there don't solve it completely.

The page is one very long text node. We calculate a bounding rect for it, and it looks like the time it takes to union the translated rects of the continuations (~60k) can cause us to checkerboard.

I optimized it down to ~100ms by being less naive, but that's not enough.

The optimal fix here is to stop accumulating rects while selecting a text anchor if we intersect the viewing region at all. We don't actually need the full bounding rect in this case, because there are no descendants to traverse.

This is tricky though because of transforms and the way the traversal algorithm is written.

[1] https://taskcluster-artifacts.net/Z5xP_yj7RVG3iBjGxs4ajg/0/public/logs/live_backing.log

Why are we doing this calculation every scroll?

Flags: needinfo?(rhunt)
Assignee

Comment 2

4 months ago

Scrolling invalidates our current scroll anchor (as it depends on the scrollport).

We need to calculate scroll anchors before flushing layout so we can detect when the scroll anchor is reflowed. I'm not an expert on reflow, but there might be a way to detect if reflow isn't going to do anything and defer the recalculation to another time.

I'll look into it.

Flags: needinfo?(rhunt)
Assignee

Comment 3

4 months ago

Looks like we flush all the scroll anchors even if we don't have pending restyles or reflows [1].

We should not do that.

[1] https://searchfox.org/mozilla-central/rev/465dbfe030dfec7756b9b523029e90d48dd5ecce/layout/base/RestyleManager.cpp#3015

Assignee

Comment 4

4 months ago

Emilio, we need to select scroll anchors before restyling to detect scroll anchor suppressions. But if we're not actually going to restyle anything, we don't want to select anchors.

What's the best way to do that? I've attached a patch from scanning ServoStyleSet, but I'm really unfamiliar with this area.

We do something similar before reflows, by not selecting anchors until we're reflowing a dirty root.

Attachment #9039307 - Flags: feedback?(emilio)
Comment on attachment 9039307 [details] [diff] [review]
no-selection-restyle.patch

Review of attachment 9039307 [details] [diff] [review]:
-----------------------------------------------------------------

How have we invalidate the anchor before restyling, just via UserScrolled()? It's a bit unfortunate that that causes us to do work... Do those anchors really need to be recomputed before restyling?

But in any case yes, this seems an ok way to ensure we don't do work without anything to restyle. Also, do you really need to do it before restyling, or before updating the styles on the frame tree?

If so, moving the call inside the while (styleSet->StyleDocument()) loop would be a bit more precise.
Attachment #9039307 - Flags: feedback?(emilio) → feedback+

But I'm curious, why do we need to compute the new anchors before knowing if we need to suppress the adjustments? shouldn't we be able to do it more lazily? How can scrolling change the ScrollAnchorContainer?

Flags: needinfo?(rhunt)

Basically, the nearest scrollable frame of a given frame cannot change without reframing, so I'm not quite sure why that is needed... Could you put an example of when this matters?

Assignee

Comment 8

4 months ago

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

Comment on attachment 9039307 [details] [diff] [review]
no-selection-restyle.patch

Review of attachment 9039307 [details] [diff] [review]:

How have we invalidate the anchor before restyling, just via UserScrolled()?
It's a bit unfortunate that that causes us to do work... Do those anchors
really need to be recomputed before restyling?

But in any case yes, this seems an ok way to ensure we don't do work without
anything to restyle. Also, do you really need to do it before restyling, or
before updating the styles on the frame tree?

If so, moving the call inside the while (styleSet->StyleDocument()) loop
would be a bit more precise.

This is what happens.

  1. The user is scrolling via some input method like mouse wheel
  2. This calls ScrollAnchorContainer::UserScrolled [1], which invalidates our scroll anchor
  3. During the refresh tick we flush style or layout before painting

We need to flush anchor selections before layout so that we can track if the anchor has been reflowed.

For styling we need to have valid anchors so that we can detect scroll anchor suppression triggers. The triggers are detected in nsFrame::DidSetComputedStyle [2].

So I think an example where this matters would be if:

  1. The user scrolls
  2. The scroll event handler sets a style property that would trigger an anchor suppression

On further consideration, I'm not sure we need to validate scroll anchors before restyling. We would only need it to detect if we need to invalidate a scroll anchor, but that doesn't matter if our scroll anchor is already invalid.

I'll try out this change.

[1] https://searchfox.org/mozilla-central/rev/465dbfe030dfec7756b9b523029e90d48dd5ecce/layout/generic/ScrollAnchorContainer.cpp#243
[2] https://searchfox.org/mozilla-central/rev/465dbfe030dfec7756b9b523029e90d48dd5ecce/layout/generic/nsFrame.cpp#1131

Flags: needinfo?(rhunt)
Assignee

Comment 9

4 months ago

(In reply to Ryan Hunt [:rhunt] from comment #8)

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

Comment on attachment 9039307 [details] [diff] [review]
no-selection-restyle.patch

Review of attachment 9039307 [details] [diff] [review]:

How have we invalidate the anchor before restyling, just via UserScrolled()?
It's a bit unfortunate that that causes us to do work... Do those anchors
really need to be recomputed before restyling?

But in any case yes, this seems an ok way to ensure we don't do work without
anything to restyle. Also, do you really need to do it before restyling, or
before updating the styles on the frame tree?

If so, moving the call inside the while (styleSet->StyleDocument()) loop
would be a bit more precise.

This is what happens.

  1. The user is scrolling via some input method like mouse wheel
  2. This calls ScrollAnchorContainer::UserScrolled [1], which invalidates our
    scroll anchor
  3. During the refresh tick we flush style or layout before painting

We need to flush anchor selections before layout so that we can track if the
anchor has been reflowed.

For styling we need to have valid anchors so that we can detect scroll
anchor suppression triggers. The triggers are detected in
nsFrame::DidSetComputedStyle [2].

So I think an example where this matters would be if:

  1. The user scrolls
  2. The scroll event handler sets a style property that would trigger an
    anchor suppression

On further consideration, I'm not sure we need to validate scroll anchors
before restyling. We would only need it to detect if we need to invalidate a
scroll anchor, but that doesn't matter if our scroll anchor is already
invalid.

I'll try out this change.

[1]
https://searchfox.org/mozilla-central/rev/
465dbfe030dfec7756b9b523029e90d48dd5ecce/layout/generic/
ScrollAnchorContainer.cpp#243
[2]
https://searchfox.org/mozilla-central/rev/
465dbfe030dfec7756b9b523029e90d48dd5ecce/layout/generic/nsFrame.cpp#1131

On third consideration, we do need it so we know to queue scroll adjustments for the RecomputePosition fast path [1].

[1] https://searchfox.org/mozilla-central/rev/465dbfe030dfec7756b9b523029e90d48dd5ecce/layout/base/RestyleManager.cpp#779

Assignee

Comment 10

4 months ago

Moving the flush inside while (styleSet->StyleDocument()) could make sense.

What state is the style attached to frames at that point? Basically can anchor selection [1] run accurately with the style state at that point?

[1] https://searchfox.org/mozilla-central/rev/465dbfe030dfec7756b9b523029e90d48dd5ecce/layout/generic/ScrollAnchorContainer.cpp#158

The frame tree styles are updated in ProcessPostTraversal. So yes, I'd think so.

Assignee

Comment 13

4 months ago

With this patch we no longer calculate scroll anchors for every scroll. I do see it intermittently still though, but it's much better.

Comment 14

4 months ago
Pushed by rhunt@eqrion.net:
https://hg.mozilla.org/integration/autoland/rev/8ea0c3205e12
Only flush pending scroll anchor selection when we have restyling to do. r=emilio

Comment 15

4 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Assignee

Comment 16

3 months ago

Comment on attachment 9039352 [details]
Bug 1523052 - Only flush pending scroll anchor selection when we have restyling to do. r?emilio

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Scroll Anchoring

User impact if declined

The user will have poor scrolling performance on certain text heavy pages.

Is this code covered by automated tests?

No

Has the fix been verified in Nightly?

Yes

Needs manual test from QE?

No

If yes, steps to reproduce

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

This has been baking on nightly a while with no apparent crash or correctness regressions.

String changes made/needed

Attachment #9039352 - Flags: approval-mozilla-beta?
Assignee

Updated

3 months ago

Comment on attachment 9039352 [details]
Bug 1523052 - Only flush pending scroll anchor selection when we have restyling to do. r?emilio

Perf improvement for scrolling, has baked on Nightly, has test coverage.
Let's uplift for beta 8.

Attachment #9039352 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.