| Summary: | Use bitfields for data members in FontPlatformData. | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yongjun Zhang <yongjun_zhang> | ||||||||||||
| Component: | Platform | Assignee: | Yongjun Zhang <yongjun_zhang> | ||||||||||||
| Status: | ASSIGNED --- | ||||||||||||||
| Severity: | Normal | CC: | buildbot, ddkilzer, enrica, joepeck, kling, mitz, rniwa, yongjun_zhang | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Attachments: |
|
||||||||||||||
|
Description
Yongjun Zhang
2013-09-12 11:07:07 PDT
Created attachment 211452 [details]
Patch: use bitfields for boolean and enum members in FontPlatformData.
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 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. 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. (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! Created attachment 211674 [details]
Address review comments: move m_size after pointers for better packing, and change bool to unsigned.
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 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
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 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
(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! Created attachment 211810 [details]
Revert to bool to fix regressions of layout tests caused by this patch.
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. 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. |