Closed
Bug 1264830
Opened 8 years ago
Closed 8 years ago
make style structs memmovable
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: heycam, Assigned: heycam)
References
Details
Attachments
(5 files, 4 obsolete files)
2.94 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
6.95 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
5.36 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
1.74 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
16.47 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
stylo needs style structs to be memmovable, but there are a couple of non-memmovable members on them currently.
Comment 1•8 years ago
|
||
Please let me know how MOZ_NEEDS_MEMMOVABLE_MEMBERS works out for you!
Assignee | ||
Comment 2•8 years ago
|
||
It works great, thank you! https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab00a556caf9&selectedJob=19524671 /builds/slave/try-l64-st-an-d-00000000000000/build/src/obj-firefox/dist/include/nsStyleStruct.h:761:22: error: class 'nsStyleBackground' cannot have non-memmovable member 'mImage' of type 'nsStyleImageLayers' nsStyleImageLayers mImage; ^ /builds/slave/try-l64-st-an-d-00000000000000/build/src/obj-firefox/dist/include/nsStyleStruct.h:696:24: note: 'nsStyleImageLayers' is a non-memmove()able type because member 'mLayers' is a non-memmove()able type 'AutoTArray<nsStyleImageLayers::Layer, 1>' AutoTArray<Layer, 1> mLayers; ^ /builds/slave/try-l64-st-an-d-00000000000000/build/src/obj-firefox/dist/include/nsStyleStruct.h:2494:27: error: class 'nsStyleDisplay' cannot have non-memmovable member 'mWillChange' of type 'AutoTArray<nsString, 1>' AutoTArray<nsString, 1> mWillChange; ^ /builds/slave/try-l64-st-an-d-00000000000000/build/src/obj-firefox/dist/include/nsStyleStruct.h:2519:43: error: class 'nsStyleDisplay' cannot have non-memmovable member 'mTransitions' of type 'AutoTArray<mozilla::StyleTransition, 1>' AutoTArray<mozilla::StyleTransition, 1> mTransitions; // [reset] ^ /builds/slave/try-l64-st-an-d-00000000000000/build/src/obj-firefox/dist/include/nsStyleStruct.h:2527:42: error: class 'nsStyleDisplay' cannot have non-memmovable member 'mAnimations' of type 'AutoTArray<mozilla::StyleAnimation, 1>' AutoTArray<mozilla::StyleAnimation, 1> mAnimations; // [reset] ^ /builds/slave/try-l64-st-an-d-00000000000000/build/src/obj-firefox/dist/include/nsStyleStruct.h:3511:25: error: class 'nsStyleSVGReset' cannot have non-memmovable member 'mMask' of type 'nsStyleImageLayers' nsStyleImageLayers mMask; ^ /builds/slave/try-l64-st-an-d-00000000000000/build/src/obj-firefox/dist/include/nsStyleStruct.h:696:24: note: 'nsStyleImageLayers' is a non-memmove()able type because member 'mLayers' is a non-memmove()able type 'AutoTArray<nsStyleImageLayers::Layer, 1>' AutoTArray<Layer, 1> mLayers; ^
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8741660 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 4•8 years ago
|
||
Attachment #8741662 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 5•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3804a91720d4d70fecd3f60a6070cd998464ac7
Attachment #8741663 -
Flags: review?(bobbyholley)
Comment 6•8 years ago
|
||
Comment on attachment 8741660 [details] [diff] [review] Part 1: Make nsStyleDisplay::{mWillChange,mTransitions,mAnimations} regular nsTArrays. Review of attachment 8741660 [details] [diff] [review]: ----------------------------------------------------------------- nsTArray doesn't do any heap allocating until the first element added, right? r=me assuming that.
Attachment #8741660 -
Flags: review?(bobbyholley) → review+
Comment 7•8 years ago
|
||
Comment on attachment 8741663 [details] [diff] [review] Part 3: Require all style structs be memmovable. Review of attachment 8741663 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/nsStyleStruct.h @@ +3581,5 @@ > uint8_t mVectorEffect; // [reset] see nsStyleConsts.h > uint8_t mMaskType; // [reset] see nsStyleConsts.h > }; > > +struct MOZ_NEEDS_MEMMOVABLE_MEMBERS nsStyleVariables This isn't technically a style struct. right? I guess it doesn't hurt to make it anyway.
Attachment #8741663 -
Flags: review?(bobbyholley) → review+
Um, don't mTransitions and mAnimations have one element by default (just like nsStyleImageLayers users) -- such that this substantially increases the amount of memory allocations that happen?
Flags: needinfo?(cam)
Comment 9•8 years ago
|
||
Comment on attachment 8741662 [details] [diff] [review] Part 2: Change nsStyleImageLayers::mLayers to a custom array type similar to AutoTArray<...,1> but memmovable. I played around with this and think it could be a little simpler. I'll upload my proposed solution.
Attachment #8741662 -
Flags: review?(bobbyholley)
Comment 10•8 years ago
|
||
Attachment #8741900 -
Flags: review?(cam)
Comment on attachment 8741660 [details] [diff] [review] Part 1: Make nsStyleDisplay::{mWillChange,mTransitions,mAnimations} regular nsTArrays. see comment 8
Attachment #8741660 -
Flags: review-
Assignee | ||
Comment 12•8 years ago
|
||
(In reply to David Baron [:dbaron] ⌚️UTC-7 from comment #8) > Um, don't mTransitions and mAnimations have one element by default (just > like nsStyleImageLayers users) -- such that this substantially increases the > amount of memory allocations that happen? I somehow missed that. :( I guess I'll templatize LayerArray and use the same for mTransitions and mAnimations. bholley: I like the alternative approach. Let's go with it, as long as users of mTransitions and mAnimations don't need to use nsTArray methods that would assume a contiguous array of elements. (In reply to Bobby Holley (busy) from comment #6) > Comment on attachment 8741660 [details] [diff] [review] > nsTArray doesn't do any heap allocating until the first element added, right? Right. (In reply to Bobby Holley (busy) from comment #7) > > +struct MOZ_NEEDS_MEMMOVABLE_MEMBERS nsStyleVariables > > This isn't technically a style struct. right? I guess it doesn't hurt to > make it anyway. nsStyleVariables is a style struct, though it doesn't hold any non-custom properties.
Flags: needinfo?(cam)
Assignee | ||
Comment 13•8 years ago
|
||
This is your approach, moved out to the top level, and with a couple of additional methods that are needed for mTransitions/mAnimations.
Attachment #8741662 -
Attachment is obsolete: true
Attachment #8741900 -
Attachment is obsolete: true
Attachment #8741900 -
Flags: review?(cam)
Attachment #8742187 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 14•8 years ago
|
||
I separated this out from the nsStyleAutoArray class addition.
Attachment #8742188 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 15•8 years ago
|
||
Attachment #8741660 -
Attachment is obsolete: true
Attachment #8742189 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 16•8 years ago
|
||
Attachment #8742190 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 17•8 years ago
|
||
Attachment #8741663 -
Attachment is obsolete: true
Assignee | ||
Comment 18•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1dd8438442b9
Updated•8 years ago
|
Attachment #8742187 -
Flags: review?(bobbyholley) → review+
Updated•8 years ago
|
Attachment #8742188 -
Flags: review?(bobbyholley) → review+
Updated•8 years ago
|
Attachment #8742189 -
Flags: review?(bobbyholley) → review+
Updated•8 years ago
|
Attachment #8742190 -
Flags: review?(bobbyholley) → review+
Updated•8 years ago
|
Attachment #8742191 -
Flags: review+
Comment 19•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f5b11b6680d https://hg.mozilla.org/integration/mozilla-inbound/rev/38c2a55b608c https://hg.mozilla.org/integration/mozilla-inbound/rev/725e1fb867eb https://hg.mozilla.org/integration/mozilla-inbound/rev/bb030d6a1d9f https://hg.mozilla.org/integration/mozilla-inbound/rev/167c5e5e4b5c
Comment 20•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4f5b11b6680d https://hg.mozilla.org/mozilla-central/rev/38c2a55b608c https://hg.mozilla.org/mozilla-central/rev/725e1fb867eb https://hg.mozilla.org/mozilla-central/rev/bb030d6a1d9f https://hg.mozilla.org/mozilla-central/rev/167c5e5e4b5c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•