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

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: bz, Assigned: hiro)

Tracking

(Blocks: 2 bugs)

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

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

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

7 months 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

7 months ago
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
(Assignee)

Comment 4

7 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ffb4985df0a1d514a1c17d136b0122cacff6817
(Assignee)

Comment 5

7 months 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

7 months 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

7 months 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

7 months 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

7 months 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

7 months ago
Attachment #8863292 - Attachment is obsolete: true

Comment 13

7 months 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

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d6c34e2c4bc7
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months 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.