Closed Bug 1329169 Opened 7 years ago Closed 7 years ago

Store StyleAnimation's name as nsAtom

Categories

(Core :: CSS Parsing and Computation, defect, P4)

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

(Blocks 1 open bug)

Details

(Whiteboard: [stylo])

Attachments

(2 files)

Thanks for filling. To be clear, I think it's worth to do it, at least from the stylo point of view, since we have much better integration between Servo and Gecko atoms that between Servo and Gecko strings.

I expect animation names to be relatively short, and copied frequently. If that is true also for Gecko, this might be a good thing to do.
FWIW in Gecko copying a string is usually cheap, since (except in certain cases) the underlying string buffer is shared with a COW scheme.  It looks like nsString and friends don't do a pointer comaprison to short circuit comparisons.  Atoms helps there.
bholley, this is what I mentioned in the meeting for using atom in animation name.
Blocks: stylo-perf
Summary: Store StyleAnimation's name as nsIAtom → stylo: Store StyleAnimation's name as nsIAtom
Oh, this is not a stylo bug, but something we want to change for stylo.
Summary: stylo: Store StyleAnimation's name as nsIAtom → Store StyleAnimation's name as nsIAtom
Whiteboard: [stylo]
Priority: -- → P4
(It's OK that this isn't going to make it for the initial stylo release in 57, right?  Tentatively setting "wontfix" for 57 since this is unassigned and 57 is nearly off of Nightly, and this doesn't sound critical.)
Flags: needinfo?(hikezoe)
Right. We have no time to do this in 57.
Flags: needinfo?(hikezoe)
Hmmm, this is a waste of memory and conversion on animation name strings... but that's probably not a big deal I guess.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Comment on attachment 8914214 [details]
Bug 1329169 - Use atom for animation-name property.

https://reviewboard.mozilla.org/r/185536/#review191216

Forgot to set review request.
Attachment #8914214 - Flags: review?(xidorn+moz)
Comment on attachment 8914214 [details]
Bug 1329169 - Use atom for animation-name property.

https://reviewboard.mozilla.org/r/185536/#review191274

r=me with the issues addressed.

::: layout/style/ServoBindings.cpp:649
(Diff revision 1)
> +Gecko_CopyAnimationNames(RawGeckoStyleAnimationListBorrowed aDest,
> +                         RawGeckoStyleAnimationListBorrowed aSrc)
> +{
> +  nsStyleAutoArray<StyleAnimation>* destArray =
> +    const_cast<nsStyleAutoArray<StyleAnimation>*>(aDest);

You should add
```c++
DECL_BORROWED_MUT_REF_TYPE_FOR(RawGeckoStyleAnimationList)
```
in ServoBindingTypes.h, and use `RawGeckoStyleAnimationListBorrowedMut` for `aDest` rather than passing a const pointer and do `const_cast` like this.

This is important for soundness and static analysis. And you would probably need to add something to analyzeHeapWrites.js to tell it that `aDest` is a safe argument.

::: layout/style/nsCSSRules.cpp:1748
(Diff revision 1)
> -  aCssText.Append(mName);
> +  nsDependentAtomString name(mName);
> +  aCssText.Append(name);

I think you can do
```c++
aCssText.Append(nsDependentAtomString(mName));
```
directly.

::: servo/components/style/properties/gecko.mako.rs:3276
(Diff revision 1)
> +        use gecko_bindings::structs::nsIAtom;
> +        use gecko_string_cache::atom_macro::nsGkAtoms__empty;
>          use properties::longhands::animation_name::single_value::SpecifiedValue as AnimationName;
> -        // XXX: Is there any effective ways?
> -        let atom = &self.gecko.mAnimations[index].mName;
> -        if atom.is_empty() {
> +
> +        let atom = self.gecko.mAnimations[index].mName.mRawPtr;
> +        if atom == unsafe { nsGkAtoms__empty } {

Why couldn't we just use `atom!("")` here?

::: servo/components/style/properties/gecko.mako.rs:3279
(Diff revision 1)
> -        // XXX: Is there any effective ways?
> -        let atom = &self.gecko.mAnimations[index].mName;
> -        if atom.is_empty() {
> +
> +        let atom = self.gecko.mAnimations[index].mName.mRawPtr;
> +        if atom == unsafe { nsGkAtoms__empty } {
>              AnimationName(None)
>          } else {
> -            AnimationName(Some(KeyframesName::from_ident(&atom.to_string())))
> +            AnimationName(Some(KeyframesName::from_atom((atom as *mut nsIAtom).into())))

I'm confused. Why do you need the `as *mut nsIAtom` here? Shouldn't atom have already been that type?
Attachment #8914214 - Flags: review?(xidorn+moz) → review+
Thanks for the review!

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #11)
> ::: layout/style/ServoBindings.cpp:649
> (Diff revision 1)
> > +Gecko_CopyAnimationNames(RawGeckoStyleAnimationListBorrowed aDest,
> > +                         RawGeckoStyleAnimationListBorrowed aSrc)
> > +{
> > +  nsStyleAutoArray<StyleAnimation>* destArray =
> > +    const_cast<nsStyleAutoArray<StyleAnimation>*>(aDest);
> 
> You should add
> ```c++
> DECL_BORROWED_MUT_REF_TYPE_FOR(RawGeckoStyleAnimationList)
> ```
> in ServoBindingTypes.h, and use `RawGeckoStyleAnimationListBorrowedMut` for
> `aDest` rather than passing a const pointer and do `const_cast` like this.
> 
> This is important for soundness and static analysis. And you would probably
> need to add something to analyzeHeapWrites.js to tell it that `aDest` is a
> safe argument.

Oh right.  I will fix it and push a try with hazzard tests.

> ::: servo/components/style/properties/gecko.mako.rs:3276
> (Diff revision 1)
> > +        use gecko_bindings::structs::nsIAtom;
> > +        use gecko_string_cache::atom_macro::nsGkAtoms__empty;
> >          use properties::longhands::animation_name::single_value::SpecifiedValue as AnimationName;
> > -        // XXX: Is there any effective ways?
> > -        let atom = &self.gecko.mAnimations[index].mName;
> > -        if atom.is_empty() {
> > +
> > +        let atom = self.gecko.mAnimations[index].mName.mRawPtr;
> > +        if atom == unsafe { nsGkAtoms__empty } {
> 
> Why couldn't we just use `atom!("")` here?

Will do.

> ::: servo/components/style/properties/gecko.mako.rs:3279
> (Diff revision 1)
> > -        // XXX: Is there any effective ways?
> > -        let atom = &self.gecko.mAnimations[index].mName;
> > -        if atom.is_empty() {
> > +
> > +        let atom = self.gecko.mAnimations[index].mName.mRawPtr;
> > +        if atom == unsafe { nsGkAtoms__empty } {
> >              AnimationName(None)
> >          } else {
> > -            AnimationName(Some(KeyframesName::from_ident(&atom.to_string())))
> > +            AnimationName(Some(KeyframesName::from_atom((atom as *mut nsIAtom).into())))
> 
> I'm confused. Why do you need the `as *mut nsIAtom` here? Shouldn't atom
> have already been that type?

You are right. The cast is not necessay, I did miss that mRawPtr is *mut.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=126edaad4f1f9d5d2cd15c0feda8f187dcae375f
Gecko_SetAnimationName was also needed to be added in treatAsSafeArgument in analyzeHeapWrites.js.
FWIW, it seems `DECL_BORROWED_MUT_REF_TYPE_FOR` declarations are usually put before their corresponding `DECL_BORROWED_REF_TYPE_FOR` declarations.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #14)
> FWIW, it seems `DECL_BORROWED_MUT_REF_TYPE_FOR` declarations are usually put
> before their corresponding `DECL_BORROWED_REF_TYPE_FOR` declarations.

Thanks! Changed the order locally.
Attached file Servo PR
Correct the summary since we renamed nsIAtom as nsAtom in Bug 1400460.
Summary: Store StyleAnimation's name as nsIAtom → Store StyleAnimation's name as nsAtom
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5ed2f3c94155
Use atom for animation-name property. r=xidorn
https://hg.mozilla.org/mozilla-central/rev/5ed2f3c94155
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.