Bug 25605 - Reduce the size of GlyphPage
Summary: Reduce the size of GlyphPage
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2009-05-06 22:04 PDT by Simon Fraser (smfr)
Modified: 2009-05-07 14:11 PDT (History)
1 user (show)

See Also:


Attachments
Patch, changelog (12.07 KB, patch)
2009-05-06 22:12 PDT, Simon Fraser (smfr)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2009-05-06 22:04:40 PDT
GlyphPage wastes a lot of space with padding, because it has a 256-size array of GlyphData, which is { unsigned short, pointer }. In 64-bit, that's 6 bytes of padding.
Comment 1 Simon Fraser (smfr) 2009-05-06 22:06:04 PDT
<rdar://problem/6864062>
Comment 2 Simon Fraser (smfr) 2009-05-06 22:12:12 PDT
Created attachment 30086 [details]
Patch, changelog
Comment 3 Darin Adler 2009-05-07 10:02:31 PDT
Comment on attachment 30086 [details]
Patch, changelog

How did you test the performance impact of this change?
Comment 4 Simon Fraser (smfr) 2009-05-07 10:41:23 PDT
I'm going to run PLTs today.
Comment 5 Simon Fraser (smfr) 2009-05-07 12:56:12 PDT
PLTs show this is a small perf gain (0.25%).
Comment 6 Darin Adler 2009-05-07 13:54:25 PDT
Comment on attachment 30086 [details]
Patch, changelog

> +    GlyphData()
> +        : glyph(0)
> +        , fontData(0)
> +    {
> +    }
> +    GlyphData(Glyph g, const SimpleFontData* f)
> +        : glyph(g)
> +        , fontData(f)
> +    {
> +    }

Could just write the above as one constructor with default arguments:

    GlyphData(Glyph g = 0, const SimpleFontData* f = 0)

instead.

> +        memcpy(m_glyphs, other.m_glyphs, GlyphPage::size * sizeof(m_glyphs[0]));
> +        memcpy(m_glyphFontData, other.m_glyphFontData, GlyphPage::size * sizeof(m_glyphFontData[0]));

This is in a member of GlyphPage, so you can omit the GlyphPage:: from the use of size. In fact, you should just use sizeof(m_glyphs) and sizeof(m_glyphFontData).

> +        memset(m_glyphs, 0, GlyphPage::size * sizeof(m_glyphs[0]));
> +        memset(m_glyphFontData, 0, GlyphPage::size * sizeof(m_glyphFontData[0]));

Ditto.

r=me
Comment 7 Simon Fraser (smfr) 2009-05-07 14:11:09 PDT
http://trac.webkit.org/changeset/43364