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.
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.
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, email@example.com 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?
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
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.
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
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
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
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
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; // [reset] See nsStyleConsts.h 4*4 nscolor mBorderColor; // [reset] the colors for a simple border. 3*4 nscoord mBorderWidths; // 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.
OS: All → Windows 2000
Hardware: All → PC
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?
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.