Closed Bug 1345804 Opened 3 years ago Closed 3 years ago

Change mWillChange to hold list of nsIAtom rather than string

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement
Priority:
Not set

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

Details

(Whiteboard: [stylo])

Attachments

(3 files)

Bug 1345804 part 1 - Constify several stuff in nsIAtom.
59 bytes, text/x-review-board-request
erahm
: review+
Details
Bug 1345804 part 2 - Add new AtomIdent unit to nsCSSValue.
59 bytes, text/x-review-board-request
heycam
: review+
Details
Bug 1345804 part 3 - Change will-change to store nsIAtom rather than nsString.
59 bytes, text/x-review-board-request
heycam
: review+
Details 
There are several fields in Gecko's style struct hold strings, and for those fields currently we can only generate new string and copy it from servo to gecko every time, which is undesired.

will-change is one of them that we are going to do such copy, and that work is in https://github.com/servo/servo/pull/15813

The work there is currently blocked on initializing string array of mWillChange. And I think rather than adding a new FFI function for that, we can probably start trying changing the Gecko field to take atoms.
Comment on attachment 8845698 [details]
Bug 1345804 part 2 - Add new AtomIdent unit to nsCSSValue.

https://reviewboard.mozilla.org/r/118840/#review120804
Attachment #8845698 - Flags: review?(cam) → review+
Comment on attachment 8845699 [details]
Bug 1345804 part 3 - Change will-change to store nsIAtom rather than nsString.

https://reviewboard.mozilla.org/r/118842/#review120806

::: layout/style/nsRuleNode.cpp:6469
(Diff revision 1)
> -        item->mValue.GetStringValue(buffer);
> +      if (atom == nsGkAtoms::transform) {
> -        display->mWillChange.AppendElement(buffer);
> -
> -        if (buffer.EqualsLiteral("transform")) {
> -          display->mWillChangeBitField |= NS_STYLE_WILL_CHANGE_TRANSFORM;
> +        display->mWillChangeBitField |= NS_STYLE_WILL_CHANGE_TRANSFORM;
> -        }
> +      }
> -        if (buffer.EqualsLiteral("opacity")) {
> +      if (atom == nsGkAtoms::opacity) {
> -          display->mWillChangeBitField |= NS_STYLE_WILL_CHANGE_OPACITY;
> +        display->mWillChangeBitField |= NS_STYLE_WILL_CHANGE_OPACITY;
> -        }
> +      }
> -        if (buffer.EqualsLiteral("scroll-position")) {
> +      if (atom == nsGkAtoms::scrollPosition) {
> -          display->mWillChangeBitField |= NS_STYLE_WILL_CHANGE_SCROLL;
> +        display->mWillChangeBitField |= NS_STYLE_WILL_CHANGE_SCROLL;
> -        }
> +      }

While you're here, do you want to change these to "else if"s?
Attachment #8845699 - Flags: review?(cam) → review+
Comment on attachment 8845698 [details]
Bug 1345804 part 2 - Add new AtomIdent unit to nsCSSValue.

https://reviewboard.mozilla.org/r/118840/#review120822

::: layout/style/nsCSSValue.cpp:992
(Diff revision 2)
> +  GetStringValue(buffer);
> +  nsCOMPtr<nsIAtom> atom = NS_Atomize(buffer);

It would probably be more efficient to use GetStringBufferValue() rather than GetStringValue().

::: layout/style/nsCSSValue.cpp:2211
(Diff revision 2)
> +    case eCSSUnit_AtomIdent:
> +      n += mValue.mAtom->SizeOfIncludingThis(aMallocSizeOf);
> +      break;

Isn't this going to double-count atoms (and thus cause errors in the tool that checks the memory reporting stuff)?
Won't nsString share the string buffer, rather than copy its contents?
Comment on attachment 8845698 [details]
Bug 1345804 part 2 - Add new AtomIdent unit to nsCSSValue.

https://reviewboard.mozilla.org/r/118840/#review120822

> It would probably be more efficient to use GetStringBufferValue() rather than GetStringValue().

Yeah, I guess that's true given that GetStringValue also does a strlen...

> Isn't this going to double-count atoms (and thus cause errors in the tool that checks the memory reporting stuff)?

Given the SizeOfIncludingThis impl of DynamicAtom and StaticAtom, I don't think so: https://dxr.mozilla.org/mozilla-central/rev/58753259bfeb3b818eac7870871b0aae1f8de64a/xpcom/ds/nsAtomTable.cpp#220-236
Comment on attachment 8845698 [details]
Bug 1345804 part 2 - Add new AtomIdent unit to nsCSSValue.

https://reviewboard.mozilla.org/r/118840/#review120822

> Given the SizeOfIncludingThis impl of DynamicAtom and StaticAtom, I don't think so: https://dxr.mozilla.org/mozilla-central/rev/58753259bfeb3b818eac7870871b0aae1f8de64a/xpcom/ds/nsAtomTable.cpp#220-236

Oh, hmmm, probably you're right. DynamicAtom and StaticAtom themselves are also shared all around, so they probably shouldn't be counted.
FWIW, there doesn't seem to be any test catching the atom double-count.
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #15)
> FWIW, there doesn't seem to be any test catching the atom double-count.

You can do a DMD [1] run to verify this.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Performance/DMD
Comment on attachment 8845697 [details]
Bug 1345804 part 1 - Constify several stuff in nsIAtom.

https://reviewboard.mozilla.org/r/118838/#review121680
Attachment #8845697 - Flags: review?(erahm) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/36f65870a49f
part 1 - Constify several stuff in nsIAtom. r=erahm
https://hg.mozilla.org/integration/autoland/rev/35b823de3de5
part 2 - Add new AtomIdent unit to nsCSSValue. r=heycam
https://hg.mozilla.org/integration/autoland/rev/c01ab228256b
part 3 - Change will-change to store nsIAtom rather than nsString. r=heycam
You need to log in before you can comment on or make changes to this bug.