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)
Core
CSS Parsing and Computation
Not set
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: xidorn, Assigned: xidorn)
Details
(Whiteboard: [stylo])
Attachments
(3 files)
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•3 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 7•3 years ago
|
||
mozreview-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 hidden (mozreview-request) |
Comment 9•3 years ago
|
||
mozreview-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)?
Comment 10•3 years ago
|
||
Won't nsString share the string buffer, rather than copy its contents?
Assignee | ||
Comment 11•3 years ago
|
||
mozreview-review-reply |
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
Assignee | ||
Comment 12•3 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•3 years ago
|
||
FWIW, there doesn't seem to be any test catching the atom double-count.
Comment 16•3 years ago
|
||
(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 17•3 years ago
|
||
mozreview-review |
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+
Comment 18•3 years ago
|
||
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
Comment 19•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/36f65870a49f https://hg.mozilla.org/mozilla-central/rev/35b823de3de5 https://hg.mozilla.org/mozilla-central/rev/c01ab228256b
Status: NEW → RESOLVED
Closed: 3 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.
Description
•