1264830 - make style structs memmovable

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

stylo needs style structs to be memmovable, but there are a couple of non-memmovable members on them currently.

Comment 1

[(no longer active)]

Please let me know how MOZ_NEEDS_MEMMOVABLE_MEMBERS works out for you!

Comment 2

[Cameron McCormack (:heycam)]

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 3

[Cameron McCormack (:heycam)]

Attachment: Part 1: Make nsStyleDisplay::{mWillChange,mTransitions,mAnimations} regular nsTArrays.

Comment 4

[Cameron McCormack (:heycam)]

Attachment: Part 2: Change nsStyleImageLayers::mLayers to a custom array type similar to AutoTArray<...,1> but memmovable.

Comment 5

[Cameron McCormack (:heycam)]

Attachment: Part 3: Require all style structs be memmovable.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3804a91720d4d70fecd3f60a6070cd998464ac7

Comment 6

[Bobby Holley (:bholley)]

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.

Comment 7

[Bobby Holley (:bholley)]

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.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 9

[Bobby Holley (:bholley)]

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.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 2: Change nsStyleImageLayers::mLayers to a custom array type similar to AutoTArray<...,1> but memmovable (alternative approach).

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8741660 [details] [diff] [review]
Part 1: Make nsStyleDisplay::{mWillChange,mTransitions,mAnimations} regular nsTArrays.

see comment 8

Comment 12

[Cameron McCormack (:heycam)]

(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.

Comment 13

[Cameron McCormack (:heycam)]

Attachment: Part 1: Add an nsStyleAutoArray array type, similar to AutoTArray<...,1> but memmovable.

This is your approach, moved out to the top level, and with a couple of additional methods that are needed for mTransitions/mAnimations.

Comment 14

[Cameron McCormack (:heycam)]

Attachment: Part 2: Change nsStyleImageLayers::mLayers to use nsStyleAutoArray.

I separated this out from the nsStyleAutoArray class addition.

Comment 15

[Cameron McCormack (:heycam)]

Attachment: Part 3: Change nsStyleDisplay::{mTransitions,mAnimations} to use nsStyleAutoArray.

Comment 16

[Cameron McCormack (:heycam)]

Attachment: Part 4: Change nsStyleDisplay::mWillChange to use nsTArray.

Comment 17

[Cameron McCormack (:heycam)]

Attachment: Part 5: Require all style structs be memmovable. r=bholley

Comment 18

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1dd8438442b9

Comment 19

[Pulsebot]

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

[Carsten Book [:Tomcat]]

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