1690836 - Style recalculation during animation 4x slower than Chrome on this testcase

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Markus Stange [:mstange]]

Attachment: testcase

Profile: https://share.firefox.dev/3jgnHSS

In this testcase, we spend 4ms in Style on every frame, whereas Chrome only spends 1.2ms on it.
Only the background-position property is animated, but it looks like we also parse the CSS-variable-substituted background-image and animation properties.

This testcase was reduced from Lea's torture test at https://designftw.mit.edu/ .

Comment 1

[Emilio Cobos Álvarez (:emilio)]

So performance of variable substitution might be improvable here, though I think we do the cascade for all elements because of correctness.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

So this is particularly bad because the variables are in the background shorthand, which means we re-parse them quite a few times...

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1690836 - Rustfmt cascade.rs. r=#style,#layout-reviewers

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1690836 - Cache substituted values from shorthand properties during the cascade. r=#style,#layout-reviewers

This brings the time down to 1.6ms from 4.8ms on the test-case in the
bug. This should be improvable too, but I think this is a nice
improvement for regular styling as well.

Depends on D105186

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9455553189d2
Rustfmt cascade.rs. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/834a980cbd49
Cache substituted values from shorthand properties during the cascade. r=xidorn

Comment 6

[Sandor Molnar[:smolnar]]

Backed out for causing bc failures in browser_editAddressDialog also dt failures.

Backout link:https://hg.mozilla.org/integration/autoland/rev/56bb1f0e0fabda4010c772b1d125c9dc14ffebfc

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=AXaXRx-uR8SKK01s2Jogtw.0&resultStatus=testfailed%2Cbusted%2Cexception%2Crunning%2Cpending%2Crunnable&searchStr=linux%2C18.04%2Cx64%2Ctsan%2Copt%2Cmochitests%2Ctest-linux1804-64-tsan%2Fopt-mochitest-browser-chrome-e10s%2Cbc3&revision=834a980cbd49b0634e98e0a00441bb20ad4ceb40

Failure log bc: https://treeherder.mozilla.org/logviewer?job_id=330106922&repo=autoland&lineNumber=3200

Failure log dt: https://treeherder.mozilla.org/logviewer?job_id=330107935&repo=autoland&lineNumber=3549

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1690836 - Reduce the amount of code generated by UnparsedValues::substitute_variables. r=#style,#layout-reviewers

This reduces the amount of assembly instructions generated by this
function from 18k+ to ~800.

This should make reasoning about its stack space usage sane, and should
fix the ASAN stack overflows, but also we should take this regardless,
because it's saner and makes reading it simpler.

I also think that the writing_mode shenanigans is fixing a bug (I think
before this, we'd pick the first physical value which mapped to any of
the properties, which is wrong), but I haven't bothered looking for a
test-case that fails before my patch. The relevant WPTs
(css/css-logical/animation*) still pass.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/be5f469a4424
Rustfmt cascade.rs. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/ef1676ed357b
Cache substituted values from shorthand properties during the cascade. r=xidorn
https://hg.mozilla.org/integration/autoland/rev/825aae40c273
Reduce the amount of code generated by UnparsedValues::substitute_variables. r=boris

Comment 9

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/be5f469a4424
https://hg.mozilla.org/mozilla-central/rev/ef1676ed357b
https://hg.mozilla.org/mozilla-central/rev/825aae40c273