Bug 121242 – Use bitfields for data members in FontPlatformData.

• Status: ASSIGNED
• Component: Platform

[Yongjun Zhang]

Some data members in FontPlatformData are in type boolean or enum; we could use bitfields to pack them.

[Yongjun Zhang]

Created attachment 211452 [details]
Patch: use bitfields for boolean and enum members in FontPlatformData.

[Joseph Pecoraro]

Any measurements on how much this reduces memory size?

For example opening a page like nytimes.com, apple.com, or the html5 spec. How much memory saving do we see? Andreas Kling might have some good ideas on making measurements.

[Yongjun Zhang]

(In reply to comment #2)
> Any measurements on how much this reduces memory size?
> 
> For example opening a page like nytimes.com, apple.com, or the html5 spec. How much memory saving do we see? Andreas Kling might have some good ideas on making measurements.

In OS X, sizeof(FontPlatformData) reduced from 48 bytes to 40 bytes.  It is hard to see the savings for one page load like nytimes.com, since the number of FontPlatformData objects is small.  That said, if we load a lot of sites with different fonts we should see some meaningful savings.

[Darin Adler]

Comment on attachment 211452 [details]
Patch: use bitfields for boolean and enum members in FontPlatformData.

View in context: https://bugs.webkit.org/attachment.cgi?id=211452&action=review

> Source/WebCore/ChangeLog:7
> +        Some data members in FontPlatformData are in type boolean or enum; we could use bitfields to pack them
> +        and reduce FontPlatformData's memory footprint.

How much does this save?

> Source/WebCore/platform/graphics/FontPlatformData.h:227
>      float m_size;
> -    FontWidthVariant m_widthVariant;
>  
> -private:
>  #if OS(DARWIN)
>      NSFont* m_font;
>  #elif PLATFORM(WIN)

I believe that having the float (32-bit) before the pointers (64-bit) will waste space on 64-bit.

> Source/WebCore/platform/graphics/FontPlatformData.h:257
> +    bool m_syntheticBold : 1;
> +    bool m_syntheticOblique : 1;
> +    unsigned m_orientation : 2; // FontOrientation
> +    unsigned m_widthVariant : 3; // FontWidthVariant
> +
> +    bool m_isColorBitmapFont : 1;
> +    bool m_isCompositeFontReference : 1;
>  #if OS(DARWIN)
> -    bool m_isPrinterFont;
> +    bool m_isPrinterFont : 1;
>  #endif
>  
>  #if PLATFORM(WIN)
> -    bool m_useGDI;
> +    bool m_useGDI : 1;
>  #endif

On the Windows compiler, I believe these will only pack together if they all use the same type. So if we want this to be small there, they need to all be unsigned instead of a mix of unsigned and bool.

[Yongjun Zhang]

(In reply to comment #4)
> (From update of attachment 211452 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=211452&action=review
> 
> > Source/WebCore/ChangeLog:7
> > +        Some data members in FontPlatformData are in type boolean or enum; we could use bitfields to pack them
> > +        and reduce FontPlatformData's memory footprint.
> 
> How much does this save?

This saves 8 bytes in 64 bit (48 bytes vs. 40 bytes). 

> 
> > Source/WebCore/platform/graphics/FontPlatformData.h:227
> >      float m_size;
> > -    FontWidthVariant m_widthVariant;
> >  
> > -private:
> >  #if OS(DARWIN)
> >      NSFont* m_font;
> >  #elif PLATFORM(WIN)
> 
> I believe that having the float (32-bit) before the pointers (64-bit) will waste space on 64-bit.

Indeed!  Moving m_size after the pointers saves another 8 bytes (40 bytes vs. 32 bytes).

> 
> > Source/WebCore/platform/graphics/FontPlatformData.h:257
> > +    bool m_syntheticBold : 1;
> > +    bool m_syntheticOblique : 1;
> > +    unsigned m_orientation : 2; // FontOrientation
> > +    unsigned m_widthVariant : 3; // FontWidthVariant
> > +
> > +    bool m_isColorBitmapFont : 1;
> > +    bool m_isCompositeFontReference : 1;
> >  #if OS(DARWIN)
> > -    bool m_isPrinterFont;
> > +    bool m_isPrinterFont : 1;
> >  #endif
> >  
> >  #if PLATFORM(WIN)
> > -    bool m_useGDI;
> > +    bool m_useGDI : 1;
> >  #endif
> 
> On the Windows compiler, I believe these will only pack together if they all use the same type. So if we want this to be small there, they need to all be unsigned instead of a mix of unsigned and bool.

Good to know!  Looks like WebKit already does that in other classes (e.g. RenderStyle.h), I will change to unsigned.

thanks!

[Yongjun Zhang]

Created attachment 211674 [details]
Address review comments: move m_size after pointers for better packing, and change bool to unsigned.

[Build Bot]

Comment on attachment 211674 [details]
Address review comments: move m_size after pointers for better packing, and change bool to unsigned.

Attachment 211674 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1861535

New failing tests:
css2.1/t1202-counter-04-b.html
fast/text/fallback-traits-fixup.html
css2.1/t1202-counters-04-b.html
platform/mac/fast/text/vertical-no-sideways.html
fast/css/font-family-pictograph.html

[Build Bot]

Created attachment 211677 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 211674 [details]
Address review comments: move m_size after pointers for better packing, and change bool to unsigned.

Attachment 211674 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1911506

New failing tests:
css2.1/t1202-counter-04-b.html
fast/text/fallback-traits-fixup.html
css2.1/t1202-counters-04-b.html
platform/mac/fast/text/vertical-no-sideways.html
fast/css/font-family-pictograph.html

[Build Bot]

Created attachment 211680 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Yongjun Zhang]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 211452 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=211452&action=review
> > 
> > > Source/WebCore/ChangeLog:7
> > > +        Some data members in FontPlatformData are in type boolean or enum; we could use bitfields to pack them
> > > +        and reduce FontPlatformData's memory footprint.
> > 
> > How much does this save?
> 
> This saves 8 bytes in 64 bit (48 bytes vs. 40 bytes). 
> 
> > 
> > > Source/WebCore/platform/graphics/FontPlatformData.h:227
> > >      float m_size;
> > > -    FontWidthVariant m_widthVariant;
> > >  
> > > -private:
> > >  #if OS(DARWIN)
> > >      NSFont* m_font;
> > >  #elif PLATFORM(WIN)
> > 
> > I believe that having the float (32-bit) before the pointers (64-bit) will waste space on 64-bit.
> 
> Indeed!  Moving m_size after the pointers saves another 8 bytes (40 bytes vs. 32 bytes).
> 
> > 
> > > Source/WebCore/platform/graphics/FontPlatformData.h:257
> > > +    bool m_syntheticBold : 1;
> > > +    bool m_syntheticOblique : 1;
> > > +    unsigned m_orientation : 2; // FontOrientation
> > > +    unsigned m_widthVariant : 3; // FontWidthVariant
> > > +
> > > +    bool m_isColorBitmapFont : 1;
> > > +    bool m_isCompositeFontReference : 1;
> > >  #if OS(DARWIN)
> > > -    bool m_isPrinterFont;
> > > +    bool m_isPrinterFont : 1;
> > >  #endif
> > >  
> > >  #if PLATFORM(WIN)
> > > -    bool m_useGDI;
> > > +    bool m_useGDI : 1;
> > >  #endif
> > 
> > On the Windows compiler, I believe these will only pack together if they all use the same type. So if we want this to be small there, they need to all be unsigned instead of a mix of unsigned and bool.
> 
> Good to know!  Looks like WebKit already does that in other classes (e.g. RenderStyle.h), I will change to unsigned.

Changing unsigned to bool causes some layout test to fail.  I think it probably changed FontPlatformData::hash() result.  I will revert back to use bool.

> 
> thanks!

[Yongjun Zhang]

Created attachment 211810 [details]
Revert to bool to fix regressions of layout tests caused by this patch.

[Ryosuke Niwa]

Comment on attachment 211810 [details]
Revert to bool to fix regressions of layout tests caused by this patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=211810&action=review

> Source/WebCore/platform/graphics/FontPlatformData.h:247
> +    bool m_syntheticBold : 1;
> +    bool m_syntheticOblique : 1;
> +    unsigned m_orientation : 2; // FontOrientation
> +    unsigned m_widthVariant : 3; // FontWidthVariant

We need to use the same type (e.g. unsigned) for all bitfields in order for this object to be packed on MSVC.

[Csaba Osztrogonác]

Comment on attachment 211452 [details]
Patch: use bitfields for boolean and enum members in FontPlatformData.

Cleared Darin Adler's review+ from obsolete attachment 211452 [details] so that this bug does not appear in http://webkit.org/pending-commit.