1392170 - stylo: Investigate why Stylo is much slower on text-orientation-script-001.html than Gecko

Status: RESOLVED FIXED

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

Description

[Xidorn Quan [:xidorn] UTC+10]

This causes timeout on web-platform test css/css-writing-modes-3/text-orientation-script-001.html.

On Gecko, this takes about 1min or 2 to run this test, while on Stylo, it takes forever.

This test mostly just repeatedly calls getBoundingClientRect for each character cluster. It doesn't seem to change the document at all, so we are not supposed to run into styling for this.

However, when I attach a profiler on it, I see that we spend 18% of total time inside Servo_TraverseSubtree, and 45% of time inside ProcessPostTraversal, which doesn't sound reasonable.

Since the document doesn't change at all, I would expect we only flush style once.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Oh, hmmm, I was wrong. This test does indeed change the document when there is any failure happens. And it can append large number of nodes into <details>, which IIUC don't show its majority of content by default.

We may still need investigating why stylo wastes lots of time here, but basically the test itself is improvable.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

So with my change in w3c/web-platform-tests#6949 the test itself should be much faster. But it remains a question that why in the original version, Stylo is so much slower than Gecko.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Can you share a profile?

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

I wanted to provide that... but it seems pref-html doesn't give me reasonable symbols.

To reproduce, you need to:
1. checkout https://github.com/w3c/web-platform-tests
2. execute "./wpt serve" in it
3. open http://localhost:8000/css/css-writing-modes-3/text-orientation-script-001.html

Without Stylo, it should finish in seconds in a release build, while on Stylo, it takes forever.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

I took one... Taking a look.

 https://perfht.ml/2xhzk2D

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1392170: Don't call ResolveServoStyle unnecessarily for undisplayed and not restyled stuff.

Review commit: https://reviewboard.mozilla.org/r/170584/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/170584/

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: style: Inline a bunch of trivial stuff we're paying function calls for in Geckolib.

Review commit: https://reviewboard.mozilla.org/r/170586/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/170586/

Comment 8

[Emilio Cobos Álvarez (:emilio)]

This is just the dumb stuff, and helps quite a bit. I suspect bug 1383332 would help a bit here, but I'm still thinking we should try to make AllChildrenIterator faster.

With the above two patches, the profile becomes:

 https://perfht.ml/2xi7Tpx

Which is mostly traversal time, due to all the siblings of the restyled stuff.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8899354 [details]
style: Inline a bunch of trivial stuff we're paying function calls for in Geckolib.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170586/diff/1-2/

Comment 10

[Emilio Cobos Álvarez (:emilio)]

The profile with STYLO_THREADS=1 (https://perfht.ml/2wok5c3) unveils a bunch of other places where we're paying stupid function calls.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8899354 [details]
style: Inline a bunch of trivial stuff we're paying function calls for in Geckolib.

https://reviewboard.mozilla.org/r/170586/#review175798

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8899353 [details]
Bug 1392170: Don't call ResolveServoStyle unnecessarily for undisplayed and not restyled stuff.

https://reviewboard.mozilla.org/r/170584/#review176732

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8899353 [details]
Bug 1392170: Don't call ResolveServoStyle unnecessarily for undisplayed and not restyled stuff.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170584/diff/1-2/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f9d851dcc374
Don't call ResolveServoStyle unnecessarily for undisplayed and not restyled stuff. r=heycam

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f9d851dcc374

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Sounds like we might want to uplift this to Beta to improve stability for the Stylo56 experiment underway?

Comment 17

[Chris Peterson [:cpeterson]]

Emilio, did this patch fix the Battle.net crash bug 1391444? We should uplift it to Beta 56 because I see a few instances of that crash signature from Beta users in our experiment.

Comment 18

[Emilio Cobos Álvarez (:emilio)]

I think this shouldn't be a problem without the patch from bug 1389790, which wasn't uplifted. That didn't seem to make its way into beta.

Comment 19

[Gerry Chang [:gchang]]

Too late for 56. Mass won't fix for 56.