Bug 116863 – Share FontGlyphs

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Antti Koivisto]

Save memory and improve performance.

[Antti Koivisto]

Created attachment 203045 [details]
WIP

[WebKit Commit Bot]

Attachment 203045 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/css/CSSFontSelector.cpp', u'Source/WebCore/page/Settings.cpp', u'Source/WebCore/platform/graphics/Font.cpp', u'Source/WebCore/platform/graphics/Font.h', u'Source/WebCore/platform/graphics/FontCache.cpp', u'Source/WebCore/platform/graphics/FontCache.h', u'Source/WebCore/platform/graphics/FontFallbackList.cpp', u'Source/WebCore/platform/graphics/FontFallbackList.h']" exit_code: 1
Source/WebCore/platform/graphics/Font.cpp:222:  Missing spaces around /  [whitespace/operators] [3]
Source/WebCore/platform/graphics/Font.cpp:320:  Missing spaces around /  [whitespace/operators] [3]
Source/WebCore/platform/graphics/FontFallbackList.h:93:  The parameter name "fontSelector" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 203045 [details]
WIP

Attachment 203045 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/724146

[EFL EWS Bot]

Comment on attachment 203045 [details]
WIP

Attachment 203045 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/742075

[Early Warning System Bot]

Comment on attachment 203045 [details]
WIP

Attachment 203045 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/742074

[EFL EWS Bot]

Comment on attachment 203045 [details]
WIP

Attachment 203045 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/670169

[Build Bot]

Comment on attachment 203045 [details]
WIP

Attachment 203045 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/715067

[Antti Koivisto]

Looks like these are failing in bots

Regressions: Unexpected text-only failures (2)
  fast/ruby/nested-ruby.html [ Failure ]
  svg/W3C-SVG-1.1/fonts-desc-02-t.svg [ Failure ]

Regressions: Unexpected image-only failures (2)
  fast/text/international/lang-sensitive-fonts.html [ ImageOnlyFailure ]
  platform/mac/fonts/han-disunification.html [ ImageOnlyFailure ]

[Build Bot]

Comment on attachment 203045 [details]
WIP

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

New failing tests:
platform/mac/fonts/han-disunification.html
svg/W3C-SVG-1.1/fonts-desc-02-t.svg
fast/ruby/nested-ruby.html
fast/text/international/lang-sensitive-fonts.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 203045 [details]
WIP

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

New failing tests:
platform/mac/fonts/han-disunification.html
svg/W3C-SVG-1.1/fonts-desc-02-t.svg
fast/ruby/nested-ruby.html
fast/text/international/lang-sensitive-fonts.html

[Build Bot]

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

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

[Antti Koivisto]

Created attachment 203148 [details]
patch

[Early Warning System Bot]

Comment on attachment 203148 [details]
patch

Attachment 203148 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/743257

[Early Warning System Bot]

Comment on attachment 203148 [details]
patch

Attachment 203148 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/672477

[EFL EWS Bot]

Comment on attachment 203148 [details]
patch

Attachment 203148 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/745034

[Antti Koivisto]

Created attachment 203153 [details]
patch 2

[Early Warning System Bot]

Comment on attachment 203153 [details]
patch 2

Attachment 203153 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/689255

[Early Warning System Bot]

Comment on attachment 203153 [details]
patch 2

Attachment 203153 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/660166

[EFL EWS Bot]

Comment on attachment 203153 [details]
patch 2

Attachment 203153 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/659110

[Antti Koivisto]

Created attachment 203185 [details]
patch 3

[Andreas Kling]

Comment on attachment 203185 [details]
patch 3

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

Pretty neat.
r=me with some whining:

> Source/WebCore/ChangeLog:8
> +        Style system generates many Font objects that are indentical or similar enough to have identical FontGlyphs. 

Typo, identical.

> Source/WebCore/ChangeLog:10
> +        It also improves perfomrance as the glyph cache and the width cache hang from FontGlyphs and the hit rate of

Typo, performance.

> Source/WebCore/css/CSSFontSelector.cpp:69
> +    , m_uniqueId(++fontSelectorId)

Can't this overflow? You're not even using a 64-bit type!

> Source/WebCore/css/CSSFontSelector.cpp:631
> +        DEFINE_STATIC_LOCAL(String, webkitPrefix, ("-webkit-"));
> +        if (familyName.startsWith(webkitPrefix))
> +            return true;

String::startsWith() has a nice overload for this, it's better to just check startsWith("-webkit-")

> Source/WebCore/css/CSSFontSelector.h:54
> +    virtual ~CSSFontSelector() FINAL OVERRIDE;

... na-na nah nah naaaaaa!

Sorry to ruin the '80s song, but we should just mark the class FINAL instead of doing it to every virtual method.

> Source/WebCore/platform/graphics/Font.cpp:169
> +    if (a.fontDescriptionCacheKey != b.fontDescriptionCacheKey)
> +        return false;
> +    if (a.families != b.families)
> +        return false;
> +    if (a.fontSelectorId != b.fontSelectorId || a.fontSelectorVersion != b.fontSelectorVersion || a.fontSelectorFlags != b.fontSelectorFlags)
> +        return false;

I would order these comparisons by cost from lowest to highest, so we can fail faster.

> Source/WebCore/platform/graphics/Font.cpp:202
> +    if (!key.fontDescriptionCacheKey.size || key.families.isEmpty())
> +        return 0;

The families Vector should never be empty, so I don't think this test is correct.

> Source/WebCore/platform/graphics/Font.h:317
> +void pruneUnreferencedFromFontGlyphsCache();

Mildly awkward name. I'd call it pruneUnreferencedEntriesFromFontGlyphsCache(); (because reasons.)

> Source/WebCore/platform/graphics/FontCache.cpp:80
> +    FontDescriptionFontDataCacheKey m_fontDescriptioKey;

Typo, m_fontDescriptioKey.

> Source/WebCore/platform/graphics/FontCache.h:67
> +    FontDescriptionFontDataCacheKey(unsigned size = 0)

This should be marked explicit.

[Antti Koivisto]

> > Source/WebCore/css/CSSFontSelector.cpp:69
> > +    , m_uniqueId(++fontSelectorId)
> 
> Can't this overflow? You're not even using a 64-bit type!

Yeah, in theory though that would take a while. Also it is unlikely that any fonts from a colliding font selector are alive at that point. If that happens you may get a wrong font.

> > Source/WebCore/css/CSSFontSelector.cpp:631
> > +        DEFINE_STATIC_LOCAL(String, webkitPrefix, ("-webkit-"));
> > +        if (familyName.startsWith(webkitPrefix))
> > +            return true;
> 
> String::startsWith() has a nice overload for this, it's better to just check startsWith("-webkit-")

Ok. Didn't know that.

> 
> > Source/WebCore/css/CSSFontSelector.h:54
> > +    virtual ~CSSFontSelector() FINAL OVERRIDE;
> 
> ... na-na nah nah naaaaaa!
> 
> Sorry to ruin the '80s song, but we should just mark the class FINAL instead of doing it to every virtual method.

:(

[Antti Koivisto]

http://trac.webkit.org/changeset/150897