Closed Bug 1360776 Opened 2 years ago Closed 2 years ago

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

Categories

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

53 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: bzbarsky, Assigned: hiro)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
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: nobody → hikezoe
Status: NEW → ASSIGNED
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 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 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 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+
(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
Attachment #8863292 - Attachment is obsolete: true
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
https://hg.mozilla.org/mozilla-central/rev/d6c34e2c4bc7
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.