1360776 - stylo: Why does get_animation_rule need to malloc/free stuff?

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

I'm seeing this somewhat in profiles.  I _think_ this is the AnimationValueMap allocation, but not 100% sure.  Maybe it's the buffer inside it? In any case, in cases when there are no animation rules we really shouldn't need to malloc/free here.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

I guess the allocation in question probably done during normal traversal.  I think we should not call get_animation_rules() in normal traversal, we should use old animation styles there instead. That's because the old styles, that are processed in animation-only restyles, are exactly the same styles what we want.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

In particular, the whole Arc<RwLock<>> business in that function looks pretty bad (I left a comment there the other day about it).

Is there a particular reason that needs synchronization and reference counting, or is it just for convenience because of the binding sugar we have on top of those?

If the second, we should improve the ergonomics so passing a stack pointer to C++ isn't that scary... But we shouldn't RwLock that.

Comment 3

[Bobby Holley (:bholley)]

I'm seeing this overhead show up in profiles. We should fix this.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ffb4985df0a1d514a1c17d136b0122cacff6817

Comment 5

[Hiroyuki Ikezoe (:hiro)]

The try looks good. Though the try does not include what I commented in comment 1, I think Emilio's insight is more important than mine. :-)
So, how can I check the allocation was reduced?

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1360776 - Pass AnimationValueMap raw pointer instead of Arc to Gecko_GetAnimationRule().

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

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1360776 - There is no need to clone for animation/transition declaration block.

Because it's a newly created Arc<Locked<PropertyDeclarationBlock> in
get_animation_rule.

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

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8863291 [details]
Bug 1360776 - Pass AnimationValueMap raw pointer instead of Arc to Gecko_GetAnimationRule().

https://reviewboard.mozilla.org/r/135086/#review137960

::: servo/components/style/gecko/wrapper.rs:426
(Diff revision 1)
>      // Also, we should try to reuse the PDB, to avoid creating extra rule nodes.
> -    let animation_values = Arc::new(RwLock::new(AnimationValueMap::new()));
> +    let mut animation_values = AnimationValueMap::new();

Also, I have not changed AnimationValueMap to PDB yet because I don't still understand how we create the extra rule nodes when we use this AnimationValueMap.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8863291 [details]
Bug 1360776 - Pass AnimationValueMap raw pointer instead of Arc to Gecko_GetAnimationRule().

https://reviewboard.mozilla.org/r/135086/#review138034

Thanks for fixing this!

::: servo/components/style/gecko/wrapper.rs:426
(Diff revision 1)
>      // Also, we should try to reuse the PDB, to avoid creating extra rule nodes.
> -    let animation_values = Arc::new(RwLock::new(AnimationValueMap::new()));
> +    let mut animation_values = AnimationValueMap::new();

So when we insert the animation rule in the rule tree, we usually create a new child rule node of the node that happens to be before it.

If the PDB is the same, we don't. It's basically this line of code: http://searchfox.org/mozilla-central/rev/3dc6ceb42746ab40f1441e1e659ffb8f62ae78e3/servo/components/style/rule_tree/mod.rs#204

It's not a huge deal for now I guess.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8863292 [details]
Bug 1360776 - There is no need to clone for animation/transition declaration block.

https://reviewboard.mozilla.org/r/135088/#review138036

Good catch :)

Comment 11

[Hiroyuki Ikezoe (:hiro)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #9)

> ::: servo/components/style/gecko/wrapper.rs:426
> (Diff revision 1)
> >      // Also, we should try to reuse the PDB, to avoid creating extra rule nodes.
> > -    let animation_values = Arc::new(RwLock::new(AnimationValueMap::new()));
> > +    let mut animation_values = AnimationValueMap::new();
> 
> So when we insert the animation rule in the rule tree, we usually create a
> new child rule node of the node that happens to be before it.
> 
> If the PDB is the same, we don't. It's basically this line of code:
> http://searchfox.org/mozilla-central/rev/
> 3dc6ceb42746ab40f1441e1e659ffb8f62ae78e3/servo/components/style/rule_tree/
> mod.rs#204
> 
> It's not a huge deal for now I guess.

Thanks for the pointer. I will keep this in mind.

A final try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5699920cb8236cb1ef2f5f17adb3e9069204bdf

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8863291 [details]
Bug 1360776 - Pass AnimationValueMap raw pointer instead of Arc to Gecko_GetAnimationRule().

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d6c34e2c4bc7
Pass AnimationValueMap raw pointer instead of Arc to Gecko_GetAnimationRule(). r=emilio

Comment 14

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

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