1372642 - stylo: Animations are slower than Blink and Gecko.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P1)

Description

[Emilio Cobos Álvarez (:emilio)]

So I profiled the url in the bug's URL field, with the following settings:

 * Dots: 750
 * Engine: Web Animations.

Animation performance was considerably slower than Gecko (and Blink is faster than Gecko here).

I took a profile, and there's a bunch of stuff going on:

 * https://perfht.ml/2rscQIW

There's a lot of painting going on, but let's assume that's not something that differs between Gecko and Stylo here (if it is, it's a bug).

There's a lot of overhead in StrongRuleNode::ensure_child. Part (most?) of that overhead is going away in [1], but we should really look into reusing the same declaration block for animation rules so we take the fast path in update_rules_at_level.

There's also a bit of time spent cascading properties that I can look into.

But there are also 400+ms spent in Gecko_GetAnimationRule!

I'm not sure about how most of the time is spent there and whether it's reasonable to expect that kind of timings, but we're using a HashMap for the animation map.

Hiro, do you know if that is _required_ to be a HashMap? We only use it to key on TransitionProperty, which is an enum, and we only look the property once... Perhaps it could be some kind of sorted list we could binary search on or something?

If it definitely needs to be a HashMap, or it is really really convenient, can we at least make it an FnvHashMap? It's using SipHash right now which is quite slow.

[1]: https://github.com/servo/servo/pull/17294

Comment 1

[Bobby Holley (:bholley)]

High priority perf issue -> p1.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

OK, I will look this.
An important thing is that Gecko also uses a hashtable to store animation values.

https://hg.mozilla.org/mozilla-central/file/91134c95d68c/dom/animation/AnimValuesStyleRule.h#l56

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Bug 1356141 might be useful here. That is, Gecko only does a single traversal when only animation changes, but, unless this has been fixed since March, in Stylo we do two traversals (animation + normal).

Also, in bug 1371518 I will drop Servo_AnimationValueMap_Push (it's not used anywhere anymore) and make the hashmap key slightly simpler.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Adding bug 1374882 in dependency list. Actually bug 1374882 is an improvement for both of stylo and gecko.

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

I don't notice a significant difference between Stylo and Gecko for this test case anymore on Windows. If anything, Gecko seems to get slower faster (i.e. Stylo is marginally better).

Can anyone else confirm, particularly on Linux or Mac?

We still should work out why we get slower and slower over time but that's a separate bug.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Nope, I don't see any differences between gecko and stylo on linux. I feel stylo is bit faster than gecko but it's emotional.

(In reply to Brian Birtles (:birtles) from comment #5)
> We still should work out why we get slower and slower over time but that's a
> separate bug.

Agree.

Comment 7

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Brian Birtles (:birtles) from comment #5)
> Can anyone else confirm, particularly on Linux or Mac?
> 

I tried this site, however I don't feel the difference between gecko and stylo on macOS.
(Nightly is 2017-07-20, macOS is 10.12)

[layout.css.servo.enabled=true ] 1.2 ~ 7.3 fps (CPU Usage is around 160%)
[layout.css.servo.enabled=false] 1.3 ~ 7.9 fps (CPU Usage is around 100%)

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Thanks mantaroh!  I am going to close this. If there are another suspicious point in animation codes, let't open a new bug.

Comment 9

[Mantaroh Yoshinaga[:mantaroh]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> Thanks mantaroh!  I am going to close this. If there are another suspicious
> point in animation codes, let't open a new bug.

AFAICS, It looks like to consume script. This test site uses the web-animations.js(Polyfill)[1], and this site run Element.animate() when each animation ended. So this operation will block the content process.

[1] https://github.com/web-animations/web-animations-js

I took the profile which removing the polyfill from this site:
 https://perfht.ml/2v78PQE

In this case(i.e. using native web-animations api), blink will over 60fps, gecko is about 6fps.

I'm going to file this bug as DOM:Animations.

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #9)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> > Thanks mantaroh!  I am going to close this. If there are another suspicious
> > point in animation codes, let't open a new bug.
> 
> AFAICS, It looks like to consume script. This test site uses the
> web-animations.js(Polyfill)[1]

Are you saying the polyfill is not falling back to the native implementation?

> In this case(i.e. using native web-animations api), blink will over 60fps,
> gecko is about 6fps.

My guess is Chrome is running this on the compositor but we're not because the layer size is too small. But at this stage I'm more interested in why the frame rate continually drops since that suggests we (or the polyfill) are leaking something. (Although if these animations are fill:forwards/both, it could just be bug 1253476.)

> I'm going to file this bug as DOM:Animations.

Please add a link here to the bug when you've filed it.