Closed Bug 1264830 Opened 4 years ago Closed 4 years ago

make style structs memmovable

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

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.
Please let me know how MOZ_NEEDS_MEMMOVABLE_MEMBERS works out for you!
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;
                       ^
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 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 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 on attachment 8741660 [details] [diff] [review]
Part 1: Make nsStyleDisplay::{mWillChange,mTransitions,mAnimations} regular nsTArrays.

see comment 8
Attachment #8741660 - Flags: review-
(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)
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)
I separated this out from the nsStyleAutoArray class addition.
Attachment #8742188 - Flags: review?(bobbyholley)
Attachment #8742187 - Flags: review?(bobbyholley) → review+
Attachment #8742188 - Flags: review?(bobbyholley) → review+
Attachment #8742189 - Flags: review?(bobbyholley) → review+
Attachment #8742190 - Flags: review?(bobbyholley) → review+
Attachment #8742191 - Flags: review+
You need to log in before you can comment on or make changes to this bug.