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

RESOLVED FIXED in Firefox 55

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bzbarsky, Assigned: hiro)

Tracking

(Blocks: 1 bug)

53 Branch
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
(Assignee)

Comment 1

2 years ago
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.
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.
Blocks: 1360881
Priority: -- → P1
I'm seeing this overhead show up in profiles. We should fix this.
(Assignee)

Updated

2 years ago
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
(Assignee)

Comment 5

2 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 8

2 years ago
mozreview-review
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

2 years ago
mozreview-review
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.
Attachment #8863291 - Flags: review?(emilio+bugs) → review+

Comment 10

2 years ago
mozreview-review
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 :)
Attachment #8863292 - Flags: review?(emilio+bugs) → review+
(Assignee)

Comment 11

2 years ago
(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 hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8863292 - Attachment is obsolete: true

Comment 13

2 years ago
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d6c34e2c4bc7
Pass AnimationValueMap raw pointer instead of Arc to Gecko_GetAnimationRule(). r=emilio

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d6c34e2c4bc7
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.