1521579 - Extremely poor scrolling performance with scroll anchoring enabled on large buildbot logs

Status: VERIFIED FIXED

(Core :: Layout, defect, P2)

Description

[Josh Matthews [:jdm]]

With layout.css.scroll-anchoring.enabled, the performance of scrolling to the bottom of the page and then using the arrow keys to scroll upwards is impressively bad. With the preference disabled, it is very smooth.

Profile: https://perf-html.io/public/c4d27364cff67fbf5ee667ecce8995ca0a18a3e4/calltree/?globalTrackOrder=0-1-2-3-4-5-6-7-8&hiddenGlobalTracks=1-2-3-4-5-6-7&localTrackOrderByPid=42651-22-0-1-2-3-4-5-6-7-8-9-10-11-12-13-14-15-16-17-18-19-20-21~42668-0~&thread=30&v=3

Looks like something like 90% of time is being spent under window.onscroll, which ends up flushing layout and doing a lot of scroll anchor work.

Comment 1

[Ryan Hunt [:rhunt]]

Ouch. It's probably not great we're flushing layout in a scroll event, but we shouldn't be taking that long to do anchor selection. I'll see how we can optimize it.

It looks like getting transforms from frames is taking ~23% of the time.

Comment 2

[Ryan Hunt [:rhunt]]

I've begun looking at optimizing SelectAnchor. There's some good room for improvement, but some if it will take some careful thought.

I'm putting up two patches that help the performance on this test case quite a bit.

Comment 3

[Ryan Hunt [:rhunt]]

Attachment: Bug 1521579 - Skip text frames that are continuations. r?dholbert

A continuation text frame's rect will be considered when visiting the primary
frame via 'FindScrollAnchoringBoundingRect', so we have no reason to compute
the same rect again if for some reason we have excluded the primary text frame.

Comment 4

[Ryan Hunt [:rhunt]]

Attachment: Bug 1521579 - Only transform one bounding rect for text nodes when computing scroll anchoring bounding rect. r?dholbert

When visiting a text frame with many continuations, traversing ancestors to compute the
transform to the ancestor scroll frame can become very hot. This commit changes the
algorithm to translate all the text continuations to an ancestor that can then be
transformed just once.

Depends on D17729

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/39b844d85e7a
Skip text frames that are continuations. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/e1865f9630af
Only transform one bounding rect for text nodes when computing scroll anchoring bounding rect. r=dholbert

Comment 6

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/39b844d85e7a
https://hg.mozilla.org/mozilla-central/rev/e1865f9630af

Comment 7

[Julien Cristau [:jcristau]]

Flagging for QA verification. I guess we'll want this on 66 since scroll anchoring is riding that train?

Comment 8

[laszlo.bialis]

Hi Josh,

I wanted to verify this issue, but didn't have access to a large buildbot log. The one in the Url attached: https://build.servo.org/builders/linux-rel-wpt/builds/11555/steps/test__1/logs/stdio doesn't seem to be available anymore.
Can you help me out please?

Comment 9

[Josh Matthews [:jdm]]

https://build.servo.org/builders/mac-rel-wpt2/builds/10800/steps/test/logs/stdio/text

Comment 10

[laszlo.bialis]

Thank you for the log!

Unfortunately I couldn't reproduce the issue on older builds, I've tried on several older Nightly builds (build ID: 20190120094340 and on 20190119095933) on several OS's (Win 10x64, Win7 x32 and x64, Ubuntu 16.04 x32 and macOS ) but I could not notice any difference from the latest Nightly (20190204214259) nor something "impressively bad" while using the arrow keys to scroll upwards.

I've also checked with layout.css.scroll-anchoring.enabled set both to true and false, but couldn't see any change.

Is there something I'm missing or am I doing something wrong?

Comment 11

[Josh Matthews [:jdm]]

https://build.servo.org/builders/mac-rel-css1/builds/6527/steps/test/logs/test-wpt.log/text may work, in that case. Warning, 100mb+ log file.

Comment 12

[laszlo.bialis]

Hi,

I've tried again with the latest log provided in comment 11, compared between Nightly 66.0a1 20190119215456 and Nightly 67.0a1 20190207094841 on Win 10 x64 but still can't see any performance differences between the two builds while scrolling up with the up arrow key.

So in my view I couldn't verify a problem that doesn't seem to reproduce in any of the builds (old vs. new).

Josh could you please advise if the issue can be marked as verified on Nightly 67.0a1?

Comment 13

[Josh Matthews [:jdm]]

Yes, I don't think it's worth trying to keep chasing it on the old builds.

Comment 14

[laszlo.bialis]

Based on comment 13 I'm marking this as verified.

Thank you Josh!

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Ryan, since 66 is set to affected, should we consider uplifting this to Beta66? Thanks!

Comment 16

[Ryan Hunt [:rhunt]]

Comment on attachment 9039301 [details]
Bug 1521579 - Only transform one bounding rect for text nodes when computing scroll anchoring bounding rect. r?dholbert

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Scroll Anchoring

User impact if declined

User will have poor scrolling performance on some 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)

It's been cooking nightly for a while with no apparent crash or correctness regressions.

String changes made/needed

Comment 17

[Ryan Hunt [:rhunt]]

Comment on attachment 9039300 [details]
Bug 1521579 - Skip text frames that are continuations. r?dholbert

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Scroll Anchoring

User impact if declined

User will have poor scrolling performance on some 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)

It's been cooking nightly for a while with no apparent crash or correctness regressions.

String changes made/needed

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 9039300 [details]
Bug 1521579 - Skip text frames that are continuations. r?dholbert

Perf fix for behavior with scroll anchoring enabled, verified in nightly.
OK to uplift for beta 8.

Comment 19

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/releases/mozilla-beta/rev/87d73e0f898f
https://hg.mozilla.org/releases/mozilla-beta/rev/bf56566e9cdb

Comment 20

[laszlo.bialis]

I've checked again by comparing Beta 66.0b8 20190214195736 to Nightly 67.0a1 20190215095516 on Win 10 x64 and the results are the same as before (see Comment 12 and Comment 13). I've used the following log as the ones provided above are not available anymore https://build.servo.org/builders/mac-rel-wpt2/builds/10842/steps/test/logs/test-wpt.log
Marking as verified.