Unnecessary padding in style structs

ASSIGNED
Assigned to

Status

()

Core
CSS Parsing and Computation
P4
normal
ASSIGNED
16 years ago
7 years ago

People

(Reporter: Daniel Bratell, Assigned: Daniel Bratell)

Tracking

({memory-footprint})

Trunk
memory-footprint
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 obsolete attachment)

(Assignee)

Description

16 years ago
I got VC7 to report about padding in structs and it told me that there was
padding in nsStyleFont, nsStyleBackground, nsStyleMargin, nsStylePadding,
nsStyleBorder, nsStyleBorderPadding, nsStyleOutline, nsStylePosition,
nsStyleList, nsStyleVisibility, nsStyleDisplay, nsStyleTable,
nsStyleTableBorder, nsStyleUIReset, nsStyleXUL and nsBorderEdge, all in
nsStyleStruct.h. Isn't those objects that should be as small as possible because
there can be very many of them? Or was that the old style system?

In most cases it's a single padding and I wouldn't be surprised if the same
padding would be added after the struct if the order of the members changed. I
will do the change anyway since I recon it can't hurt and it will at least save
a couple of bytes.
(Assignee)

Comment 1

16 years ago
Created attachment 109116 [details] [diff] [review]
Reorder some structs

There were a couple of structs that benefitted from reordering. nsStyleOutline
shrank by 4 bytes and nsStyleTableBorder by 4 bytes. Maybe nsStyleDisplay
shrank too, but I don't think so. nsStyleXUL clearly shrank by 4 bytes and
nsBorderEdge by 4 bytes. The rest I didn't touch because changing those would
only move the padding from within the struct to the end of the struct.
(Assignee)

Updated

16 years ago
Attachment #109116 - Flags: superreview?(brendan)
Attachment #109116 - Flags: review?(dbaron)
(Assignee)

Comment 2

16 years ago
Taking the bug. It's mine, all mine!
Assignee: dbaron → bratell
Target Milestone: --- → mozilla1.3beta
Comment on attachment 109116 [details] [diff] [review]
Reorder some structs

Looks good to me, sr=brendan@mozilla.org contingent on r=dbaron.

/be
Attachment #109116 - Flags: superreview?(brendan) → superreview+
Comment on attachment 109116 [details] [diff] [review]
Reorder some structs

This is too trivial to bother dbaron with. It doesn't need a true style system
peer. r=roc+moz
Attachment #109116 - Flags: review?(dbaron) → review+
It looks like we could reduce these structs enormously by packing the fields
into bitfields (just adding :N to various fields should be enough). Daniel, do
you want to do that?
This patch seems fine with me.

How much will bitfields help memory use anyway, and how much will they hurt
performance?  Do we really have that many of these structs to start with?
(Assignee)

Comment 7

16 years ago
Bitfields would cause problems if someone uses values != 1 to represent TRUE.

I could try bitfields iff I know that the size of these objects really matter
and if I can find a way to see what performance impact bitfields cause.

The current patch has no drawbacks so I'll check it in, but if someone can
answer the questions in the previous paragraph, I'll leave the bug open for
further work.
Status: NEW → ASSIGNED
In the old days (1994-1997) we eschewed bitfields for (all fixed by now, I
trust) portability reasons (some compilers had bugs), and more important,
because they were rarely an improvement over explicit flag bits or packed bools
in clarity, and a disimprovement over packed bools in time performance.

So if you have padding bytes already, pack bools into them rather than use a
flag word.  Of course, if you have many more than 4 flags, then it seems to me a
premature optimization to use packed bools rather than a PRUint32 flag word with
well-defined flag bit macros.

/be
(Assignee)

Comment 9

16 years ago
Looking at the structs, they seem to already have been a victim of some kind of
space optimization. All bools, save one, are PRPackedBools and there are many
flag fields in PRUint8:s. There are even some bitfields (4*4 bits) in
nsStyleBackground. 

If space was the only criteria some things could be done with stuff like this:
  PRUint8 mTextAlign;                   // [inherited] see nsStyleConsts.h
  PRUint8 mTextTransform;               // [inherited] see nsStyleConsts.h
  PRUint8 mWhiteSpace;                  // [inherited] see nsStyleConsts.h

I guess you could fit them all into a byte (or maybe two). The one struct that
has most potential is nsStyleDisplay that is ~14 bytes plus a string. There are
2 bools, 1 nsmargin and 8 "enum" fields in PRUint8:s. Knowing only a little
about those, I would still say that none uses all 8 bits and most uses only 2 or
maybe three. 

But this is still academic. We don't know if these structs are an issue. I
should add counter and see if I can get some statistics from them. 
(Assignee)

Comment 10

16 years ago
I added a counter on nsStyleDisplay and opened a number of windows with normal
pages ( http://www.mozilla.org/ and such). It seems like a "normal" page will
have ~175 nsStyleDisplay structs. 3 browser windows and a mailnews windows added
up to ~800 structs. If the other types are as common there could be many
thousands of structs and then it might be useful to trim them. Unless it hurts
performance...
I suspect that would be one of the more common structs.  The smaller structs are
also likely to be less common since there are fewer properties to cause new
structs to be needed or to prevent sharing in the rule tree, and the inherited
structs are going to inherit in many cases.
Even if the population of these structs is small on average, or always, you
might save some code space by cleaning things up to be more consistent.  Not a
high priority, obviously.  Readability of the structs themselves might benefit too.

/be
(Assignee)

Comment 13

16 years ago
Comment on attachment 109116 [details] [diff] [review]
Reorder some structs

Checked in.

It even affected code size:
				 +4	atexit
				 +4    
nsStyleDisplay::CalcDifference(nsStyleDisplay const &) const
				 -4    
nsStyleTableBorder::CalcDifference(nsStyleTableBorder const &) const
				 -8    
nsStyleUIReset::CalcDifference(nsStyleUIReset const &) const
				-12	nsStyleXUL::CalcDifference(nsStyleXUL
const &) const

The CalcDifference functions only does comparisons of fields. Obviously the
order made some of them shorter/faster. That's another area of optimization if
someone has a lot of extra time.
Attachment #109116 - Attachment is obsolete: true
(Assignee)

Comment 14

16 years ago
Data:

sizeof(struct) not including pointed to data in nsStyleFont (nsString),
nsStyleContent (nsStyleCounterData*, nsStyleContentData*), nsStyleUserInterface
(nsString), nsStyleBorder (nsBorderColors*) and nsStyleQuotes (nsString*)

nsStyleColor           4  
nsStyleBackground     32  
nsStyleMargin         40  
nsStylePadding        40 
nsStyleBorder        100 
nsStyleBorderPadding  20  
nsStyleOutline        52 
nsStyleList           36 
nsStylePosition       80
sStyleTextReset       12  
nsStyleText           36 
nsStyleDisplay        44 
nsStyleVisibility     12 
nsStyleTableBorder    20
nsStyleTable          12
nsStyleQuotes          8 
nsStyleContent        32 
nsStyleUIReset         6
nsStyleUserInterface  20
nsStyleXUL            12 

Total size of structs in memory:
Starting Mozilla and opening Profile Manager:    8 KB
Loading http://www.mozilla.org/start/           30 KB
Opening new empty window:                       43 KB
Loading www.cnn.com in the empty window        115 KB
Reopening window and loading www.cnn.com again 115 KB
Opening two empty tabs and a mailnews window   160 KB

At the end the memory usage was distributed as:

                       Bytes	Count
nsStyleFont            10960*	274  (7%)
nsStyleColor             504	126
nsStyleBackground      13984	437  (9%)
nsStyleMargin           7480	187  (5%)
nsStylePadding          9320	233  (6%)
nsStyleBorder          44600*	446  (29%)
nsStyleBorderPadding       0	  0
nsStyleOutline          <bad data, few>
nsStyleList             3996	111
nsStylePosition        24560	307  (16%)
nsStyleTextReset        2424	202
nsStyleText             6192	172   (4%)
nsStyleDisplay         26092	593  (17%)
nsStyleVisibility       1008	 84
nsStyleTableBorder       840	 42
nsStyleTable             384	 32
nsStyleQuotes             56*	  7
nsStyleContent           480*	 15
nsStyleUIReset            72	 12
nsStyleUserInterface    1800*	 90
nsStyleXUL              1440	120

(*) - Not the whole size

Comment 15

16 years ago
The first field in a structure is usually cheaper to access (it can be addressed
through the structure's own address rather than an offset address, and compilers
know that).  It's good to pack structures with this knowledge, placing the most
commonly-accessed field at the start.  That usually amounts to moderately good
code space savings (probably negligable improvements speed-wise).
OS: Windows 2000 → All
Hardware: PC → All
(Assignee)

Comment 16

16 years ago
So it totals maybe 1-2% of the total used memory for pages. Not worth killing
for, but measurable at least. There are only a few structs worth optimizing
sizewise and nsStyleBorder is the number 1 of those:

20  nsStyleSides  mBorder;          // [reset] length, enum (see nsStyleConsts.h)
     1  PRUint8       mLeftUnit;
     1  PRUint8       mTopUnit;
     1  PRUint8       mRightUnit;
     1  PRUint8       mBottomUnit;
     4  nsStyleUnion  mLeftValue;  // Size is max(float, int)
     4  nsStyleUnion  mTopValue;
     4  nsStyleUnion  mRightValue;
     4  nsStyleUnion  mBottomValue;
20   nsStyleSides  mBorderRadius;    // [reset] length, percent, inherit
1    PRUint8       mFloatEdge;       // [reset] see nsStyleConsts.h
3    <padding>
4    nsBorderColors** mBorderColors; // [reset] multiple levels of color for a
border.
1    PRPackedBool  mHasCachedBorder;
3    <padding>
16   nsMargin      mCachedBorder;
     4*4  nscoord left, top, right, bottom;
4*1  PRUint8       mBorderStyle[4];  // [reset] See nsStyleConsts.h
4*4  nscolor       mBorderColor[4];  // [reset] the colors for a simple border.  
  
3*4  nscoord       mBorderWidths[3]; // XXX remove with deprecated methods
 <no end padding>

I see a couple of things that could be done here. Reordering would save one word
padding. Border radius must be very rare so those could be moved externally
(20->4; saving 16 bytes). mBorderStyle uses 13 different value and one exra bit
so it's hard to save more than 1 byte at a high cost. mBorderWidths' 12 bytes is
very strange. They are used to save the width of three kinds of borders and the
values are constant (1, 3 and 5 pixels). It looks like work not finished. Does
anyone have the story?

The second struct is nsStyleDisplay:
16   nsString  mBinding;           // [reset] absolute url string
16   nsRect    mClip;              // [reset] offsets from upper-left border edge
1    PRUint8 mDisplay;             // [reset] see nsStyleConsts STYLE_DISPLAY_*
1    PRUint8 mOriginalDisplay;     // [reset] saved mDisplay pos:absolute/fixed
1    PRUint8 mAppearance;          // [reset]
1    PRUint8 mPosition;            // [reset] see nsStyleConsts
1    PRUint8 mFloats;              // [reset] see nsStyleConsts STYLE_FLOAT_*
1    PRUint8 mBreakType;           // [reset] see nsStyleConsts STYLE_CLEAR_*
1    PRPackedBool mBreakBefore;    // [reset] 
1    PRPackedBool mBreakAfter;     // [reset] 
1    PRUint8   mOverflow;          // [reset] see nsStyleConsts.h
1    PRUint8   mClipFlags;         // [reset] see nsStyleConsts.h
2 <end padding>  

Don't know about the string. If it's rare it could be a pointer. mDisplay has 31
possible values -> 4 bits, mOriginalDisplay too -> 4 bits. mAppearance?
mPosition has 4 different values -> 2 bits, mFloat also -> 2 bits, mBreakType
has 8 different values -> 3 bits. The two bools -> 2 bits. mOverflow, 7 values
-> 3 bits. It should therefore be possible to shrink it from 44 bytes + string
to 28 bytes + string. 
Keywords: footprint
OS: All → Windows 2000
Hardware: All → PC
(Assignee)

Comment 17

16 years ago
nsStylePosition finally:

20   nsStyleSides  mOffset;                // [reset]
8    nsStyleCoord  mWidth;                 // [reset] coord, perc, auto, inherit
     4  nsStyleUnit   mUnit; // enum - probably int = 4 bytes
     4  nsStyleUnion  mValue;
8    nsStyleCoord  mMinWidth;              // [reset] coord, perc, inherit
8    nsStyleCoord  mMaxWidth;              // [reset] coord, perc, null, inherit
8    nsStyleCoord  mHeight;                // [reset] coord, perc, auto, inherit
8    nsStyleCoord  mMinHeight;             // [reset] coord, perc, inherit
8    nsStyleCoord  mMaxHeight;             // [reset] coord, perc, null, inherit
1    PRUint8       mBoxSizing;             // [reset] see nsStyleConsts.h
3    <padding>
8    nsStyleCoord  mZIndex;                // [reset] 

The only things I can see is that mBoxSizing only has three possible values and
don't need 8 bit and that it seems kind of wrong to use a nsStyleCoord for
zIndex. Isn't that just an int?
QA Contact: ian → style-system
Just a note:  I don't want these using bitfields because I'm hoping to make a bunch of the code that gets/sets them be table-driven.  That will require dealing with pointers and offsets to these members.
OS: Windows 2000 → All
Priority: -- → P4
Hardware: x86 → All
Target Milestone: mozilla1.3beta → ---
You need to log in before you can comment on or make changes to this bug.