1618678 - geckoservo::glue::traverse_subtree was slower in Fenix compare to Fennec

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Sean Feng [:sefeng211]]

Fennec: https://perfht.ml/2TpGIpH
Fenix: https://perfht.ml/2Pv9h3w

Phone: Moto G5.

The profile was taken by loading https://stackoverflow.com/questions/927358/how-do-i-undo-the-most-recent-commits-in-git.

Based on the profile, Fennec was 20ms and Fenix took 100ms. And there's another call path in Fenix profile, which also took 30ms.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

I'm a bit confused by that callstack as it's only showing the StyleSubtree calls that appear under the frame constructor, which are usually just for native anonymous content and such.

These are probably scrollbar styles.

We added a caching mechanism for them in bug 1554571, but that also included a bunch of exit conditions and extra rules. So the individual StyleSubtree calls are expected to be more expensive, but we should not have many of them.

That being said, give Geckoview has their own CSS for scrollbars, it may be that they're not hitting the optimized path.

At a glance, this:

• https://searchfox.org/mozilla-central/rev/b2ccce862ef38d0d150fcac2b597f7f20091a0c7/layout/style/res/geckoview.css#25

Seems like it'd conflict with this:

• https://searchfox.org/mozilla-central/rev/b2ccce862ef38d0d150fcac2b597f7f20091a0c7/layout/base/nsCSSFrameConstructor.cpp#3870

So we would be doing much more expensive styling of all the scrollbar thumbs and such. Does commenting out the scrollbar-related rules in geckoview.css help with this? Can you check whether you're hitting the allowStyleCaching=false?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Alternative approach of verifying that hypotheses:

• Turn layout.css.cached-scrollbar-styles.enabled to false
• Remove this rule
• Are the times the closer to the fennec ones?

Comment 3

[Sean Feng [:sefeng211]]

NI myself for verifying the hypotheses.

Comment 4

[Sean Feng [:sefeng211]]

Hi Emilio,

I tried the #comment 2 approach and it improved it a lot. Looking at the profiles, without change https://perfht.ml/2VMV1aB and with change https://perfht.ml/38wnBzI , it cuts more than half the time!

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Ok, that's great to know... I'll take a look at this next week.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1618678 - Make scrollbar style caching work on Android. r=heycam

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1618678 - Make disabling layout.css.cached-scrollbar-styles.enabled not a massive perf regression. r=heycam

(Because of all the extra declarations)

Depends on D65915

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1618678 - Simplify a bit GeckoView's scrollbars.css. r=heycam

It seems very paranoid about stuff that can't happen.

Depends on D65916

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a6b513d590d4
Make scrollbar style caching work on Android. r=heycam
https://hg.mozilla.org/integration/autoland/rev/85020a807040
Make disabling layout.css.cached-scrollbar-styles.enabled not a massive perf regression. r=heycam
https://hg.mozilla.org/integration/autoland/rev/bbcf0278b31d
Simplify a bit GeckoView's scrollbars.css. r=heycam

Comment 10

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/a6b513d590d4
https://hg.mozilla.org/mozilla-central/rev/85020a807040
https://hg.mozilla.org/mozilla-central/rev/bbcf0278b31d