Bug 116863

Summary: Share FontGlyphs
Product: WebKit Reporter: Antti Koivisto <koivisto>
Component: TextAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, commit-queue, eflews.bot, esprehn+autocc, glenn, gyuyoung.kim, kling, macpherson, menard, rniwa, webkit-ews
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
WIP
webkit-ews: commit-queue-
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion
none
patch
webkit-ews: commit-queue-
patch 2
webkit-ews: commit-queue-
patch 3 kling: review+

Antti Koivisto
Reported 2013-05-28 05:48:01 PDT
Save memory and improve performance.
Attachments
WIP (17.13 KB, patch)
2013-05-28 05:49 PDT, Antti Koivisto
webkit-ews: commit-queue-
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (4.78 MB, application/zip)
2013-05-28 23:59 PDT, Build Bot
no flags
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (4.14 MB, application/zip)
2013-05-29 02:52 PDT, Build Bot
no flags
patch (23.84 KB, patch)
2013-05-29 03:39 PDT, Antti Koivisto
webkit-ews: commit-queue-
patch 2 (23.89 KB, patch)
2013-05-29 04:38 PDT, Antti Koivisto
webkit-ews: commit-queue-
patch 3 (23.92 KB, patch)
2013-05-29 04:56 PDT, Antti Koivisto
kling: review+
Antti Koivisto
Comment 1 2013-05-28 05:49:20 PDT
WebKit Commit Bot
Comment 2 2013-05-28 05:50:57 PDT
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 3 2013-05-28 05:57:38 PDT
EFL EWS Bot
Comment 4 2013-05-28 05:58:10 PDT
Early Warning System Bot
Comment 5 2013-05-28 05:59:03 PDT
EFL EWS Bot
Comment 6 2013-05-28 06:10:58 PDT
Build Bot
Comment 7 2013-05-28 06:15:21 PDT
Antti Koivisto
Comment 8 2013-05-28 09:52:01 PDT
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 9 2013-05-28 23:59:33 PDT
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
Comment 10 2013-05-28 23:59:36 PDT
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 11 2013-05-29 02:52:44 PDT
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
Comment 12 2013-05-29 02:52:47 PDT
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
Comment 13 2013-05-29 03:39:46 PDT
Early Warning System Bot
Comment 14 2013-05-29 03:45:57 PDT
Early Warning System Bot
Comment 15 2013-05-29 03:47:34 PDT
EFL EWS Bot
Comment 16 2013-05-29 03:54:01 PDT
Antti Koivisto
Comment 17 2013-05-29 04:38:35 PDT
Early Warning System Bot
Comment 18 2013-05-29 04:46:22 PDT
Early Warning System Bot
Comment 19 2013-05-29 04:47:07 PDT
EFL EWS Bot
Comment 20 2013-05-29 04:51:36 PDT
Antti Koivisto
Comment 21 2013-05-29 04:56:01 PDT
Andreas Kling
Comment 22 2013-05-29 07:13:53 PDT
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
Comment 23 2013-05-29 07:47:23 PDT
> > 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
Comment 24 2013-05-29 08:49:53 PDT
Note You need to log in before you can comment on or make changes to this bug.