1388241 - stylo: We should aim to have all style structs be smaller than 504 bytes to avoid allocator slop

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Boris Zbarsky [:bzbarsky]]

Gecko arena-allocates style structs, so the exact sizes are not terribly important in terms of allocator slop: the only source of slop is whatever is due to the arena size not being an integral multiple of the struct size.  Since we use 8192-byte arenas and structs are all <1KiB in size (I hope!) the maximum slop is less than 12% of total memory allocated.

Stylo, on the other hand, directly mallocs the structs.  This means that we can get a lot more slop.  For example, see the data in bug 1367854 comment 17.  The "Position" struct ends up at 528 bytes including the Arc refcount.  jemalloc has a 512 byte bucket and then a 1024-byte bucket, so we end up using 1024 bytes for ever Position struct, so 48% of the allocation is slop.

In practice, what that means is that if we're mallocing stylo structs we should ensure that the structs are no larger than 504 bytes.  I believe that should get the Arc under 512 bytes, where jemaloc uses 16-byte-spaced buckets and slop will be minimal.  Ideally we'd static_assert that about all our struct sizes.

For the particular case of nsStylePosition, we can maybe make some of those uint8_t/uint16_t into smaller bitfields.  But that can't possibly save us more than 13 bytes, and we need to save 24.

Another place we might be able to save some space is in mZIndex.  It's an "integer or auto" but we end up using two machine words to represent it, as far as I can tell: nsStyleCoord has a uint8_t and a void*, for sizing purposes.  On 64-bit that's 16 bytes.  We should be able to cut that down to 5 bytes easily by storing the "is this auto?" boolean in a byte that packs with other uint8_t and the integer (if not boolean) in 4 bytes.  We could probably get down to 4 bytes if we put that "is this auto?" boolean into some bitfields with the other uint8/16 things.

Another thing we could do is address the "XXXmats we could optimize memory size here" in nsStyleGridLine.  We have 4 of those, so if we could squeeze some memory from each one that would help.

Finally, nsStyleGridTemplate is pretty huge and we have two of them.  Maybe we can rejigger how that works somehow....

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

nsStyleGridTemplate is 6 words, which means 48 bytes on 64bit platforms. If we can make them optional (by putting them behind a UniquePtr, for example), we would easily get 80 bytes back, so that we don't need to worry a lot about other fields.

What I'm concerned for making more use of bitfield is that, some parts of Gecko's style system seem to depend on being able to hold a reference to a field, which doesn't work with bitfield. So before we can get rid of Gecko's style system, we can probably only convert a small number of fields into bitfields.

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Oh, each nsStyleGridTemplate only represents a single property (grid-template-{columns,rows}). I guess it is easy to just have them behind a pointer, then.

Comment 3

[Bobby Holley (:bholley)]

Assigning to xidorn, since he's doing bug 1381821. Once that's done we can hopefully just add the static_asserts and be done here.

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

OK, yeah, it seems nsStylePosition is the only offending one, and with bug 1388255 done, we can simply add the static_asserts.

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1388241 - Assert that all style structs are under 504 bytes.

Review commit: https://reviewboard.mozilla.org/r/166384/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/166384/

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

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

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8895215 [details]
Bug 1388241 - Assert that all style structs are under 504 bytes.

https://reviewboard.mozilla.org/r/166384/#review171550

::: layout/style/nsStyleStruct.cpp:56
(Diff revision 1)
>  /* static */ const int32_t nsStyleGridLine::kMinLine;
>  /* static */ const int32_t nsStyleGridLine::kMaxLine;
>  
> +// We set the size limit of style structs to 504 bytes so that when they
> +// are allocated by Servo side with Arc, the total size doesn't exceed
> +// 512 bytes, which minimizes slop.

"allocator slop"

::: layout/style/nsStyleStruct.cpp:60
(Diff revision 1)
> +// are allocated by Servo side with Arc, the total size doesn't exceed
> +// 512 bytes, which minimizes slop.
> +static constexpr size_t kStyleStructSizeLimit = 504;
> +#define STYLE_STRUCT(name_, checkdata_cb_) \
> +  static_assert(sizeof(nsStyle##name_) <= kStyleStructSizeLimit, \
> +                "nsStyle" #name_ " becomes larger than the size limit");

"became"

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8895215 [details]
Bug 1388241 - Assert that all style structs are under 504 bytes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/166384/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/372308629f8d
Assert that all style structs are under 504 bytes. r=bz

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/372308629f8d