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

RESOLVED FIXED in Firefox 57

Status

()

Core
CSS Parsing and Computation
P3
normal
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: bz, Assigned: xidorn)

Tracking

(Blocks: 1 bug)

53 Branch
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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....
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.
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.
Depends on: 1388255
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.
Assignee: nobody → xidorn+moz
Priority: -- → P3
OK, yeah, it seems nsStylePosition is the only offending one, and with bug 1388255 done, we can simply add the static_asserts.
Comment hidden (mozreview-request)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a5c25a18b313e8de300a462ea3ace7c9cedc2719
(Reporter)

Comment 7

3 months ago
mozreview-review
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"
Attachment #8895215 - Flags: review?(bzbarsky) → review+
Comment hidden (mozreview-request)

Comment 9

3 months ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/372308629f8d
Assert that all style structs are under 504 bytes. r=bz

Comment 10

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/372308629f8d
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.