Bug 61875 – Fonts returned by FontCache::getFontDataForCharacters() are never released

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Michael Saboff]

From FontCache.h:

    // This method is implemented by the platform.
    // FIXME: Font data returned by this method never go inactive because callers don't track and release them.
    const SimpleFontData* getFontDataForCharacters(const Font&, const UChar* characters, int length);

The fonts returned by this method are used as system fall back fonts in the glyph page tree and are not properly cleaned up. In fact, fonts returned via this method never have their reference counts go down.  Therefore these fonts cannot be released and purged.

The issue isn't the method, but the caller of the method, currently there is only one, Font::glyphDataForCharacter in FontFastPath.cpp.

[Michael Saboff]

Created attachment 95641 [details]
Proposed patch

This patch adds cleanup code for system fall back fonts.  It also provides an auto release object to wrap around accesses to fonts via the FontCache::getFontDataForCharacters() method.  When that object is destructed, it calls FontCache::releaseFontData with the appropriate reference count.

The assureOneHold code is needed for the case where a view uses a system fallback font that was brought into the glyph page trees via another prior and currently open view.  The second view would still have one reference, even if the first view closed dereferenced the font.  This makes it so that every using view has at least one reference on a system fallback font that it uses.

NOTE: This patch does not add the appropriate code to platforms other than Mac to keep track of the number of references via those platform's FontCache::getFontDataForCharacters method.  That code will be added in a subsequent patch.

An alternative approach to this problem has been considered and that is using RefPtr<FontData> for most accesses to font data. The exception is the glyph page node tree, since that tree is purged of references when via the SimpleFontData destructor.  FontData returned from the glyph page tree code would need to increment the returned font's reference count.

This patch allows for almost all fonts to be released when all views are closed.  The only active fonts are those used by the rest of the application.

[Dave Hyatt]

Comment on attachment 95641 [details]
Proposed patch

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

> Source/WebCore/page/FrameView.cpp:118
> +    , m_fontCacheAutoRelease(new FontCacheAutoRelease)

OwnPtr!

> Source/WebCore/page/FrameView.cpp:204
> +    delete m_fontCacheAutoRelease;

OwnPtr!

> Source/WebCore/page/FrameView.h:367
> +    FontCacheAutoRelease* m_fontCacheAutoRelease;

Can't this be an OwnPtr?

> Source/WebCore/platform/mac/MemoryPressureHandlerMac.mm:73
> +//#ifndef NDEBUG

I assume you didn't mean to leave this commented out.

> Source/WebCore/platform/mac/MemoryPressureHandlerMac.mm:85
> +//#endif

Ditto.

[Adam Roben (:aroben)]

Comment on attachment 95641 [details]
Proposed patch

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

> Source/WebCore/platform/graphics/FontCache.cpp:303
> +    if (m_autoReleaseStack.size() > 0) {
> +        FontCacheAutoRelease* fontCacheAutoRelease = m_autoReleaseStack.last();
> +        if (!fontCacheAutoRelease->isReferenced(fontData)) {
> +            FontDataCache::iterator cacheEntry = gFontDataCache->find(fontData->platformData());
> +            ASSERT(cacheEntry != gFontDataCache->end());
> +            cacheEntry.get()->second.second++;
> +            fontCacheAutoRelease->addFontDataToRelease(fontData);
> +        }
> +    }
> +}
> +

Early returns would be nicer than all this nesting.

[Dave Hyatt]

As for approach, I would certainly favor moving to a reference counted model *if* it doesn't wreck performance to do so.

[Geoffrey Garen]

Comment on attachment 95641 [details]
Proposed patch

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

You mentioned that RefPtr<FontData> was considered. Why was it rejected? I think it was rejected because of performance concerns about refcount thrash. But isn't four hash lookups per font reference more expensive?

> Source/WebCore/page/FrameView.cpp:204
> +    delete m_fontCacheAutoRelease;

Please use OwnPtr instead, so we can verify by code inspection that ownership is correct.

> Source/WebCore/page/FrameView.cpp:2481
> +    m_fontCacheAutoRelease->pop();

I don't know how to verify by code inspection that you have pushes and pops in the right places.

Should assureOneHold or some similar function ASSERT that there is an autorelease pool on the stack?

> Source/WebCore/platform/graphics/FontCache.cpp:410
> +    (void)fontCache()->popAutoRelease();

No need for (void) here.

> Source/WebCore/platform/graphics/FontCache.cpp:432
> +    m_referencedFontDataMap.set(fontData, refCount);

You can reuse the iterator to avoid an extra hash lookup here.

> Source/WebCore/platform/graphics/FontCache.h:62
> +    FontCacheAutoRelease(bool autoPushPop = false);

Does anybody ever use the autoPushPop flag?

> Source/WebCore/platform/graphics/FontFastPath.cpp:82
> +                if (node->isSystemFallback() && data.fontData)
> +                    fontCache()->assureOneHold(data.fontData);

It's not really clear why assureOneHold should only be called on system fallback fonts. Maybe you can move the check for system fallback fonts inside assureOneHold, and add a comment there explaining why only system fallback fonts are considered.

> Source/WebCore/platform/mac/MemoryPressureHandlerMac.mm:76
> +//#ifndef NDEBUG
>      if (!_cache_event_source2) {
>          dispatch_async(dispatch_get_main_queue(), ^{
>              _cache_event_source2 = dispatch_source_create(DISPATCH_SOURCE_TYPE_SIGNAL, SIGUSR2, 0, dispatch_get_main_queue());

Did you mean to include this?

[Michael Saboff]

Created attachment 95679 [details]
Patch with changes to address comments

Made changes to address most of the comments.

Added speculative changes to FontCache::getFontDataForCharacters for other platforms.

I did reduce the number of hash lookups in the FontCache::assureOneHold case by keeping the top of m_autoReleaseStack value.  

Concerning the comments and questions about moving to a reference counting algorithm. That is my desire as well, but I have some lingering concerns due to performance.  I instrumented the current code to compare the calls to FontCache::assureOneHold, which only happen for system fallback fonts, with the likely number of ref() calls in a reference counting implementation.  On one memory tests, the ratio was ~13 likely ref() calls to 1 assureOneHold call.  For english only web pages, there were 0 or very few FontCache::assureOneHold calls and thousands to tens of thousands of likely ref() calls.

I will be modifying this patch to replace the reference counting currently implemented in the FontCache class with RefPtr<FontData> for the holders of font data down stream of the glyph page node tree.  I will then compare this patch's performance to the reference counting version.  If the reference counting version performs the same or better than this patch, I will post it for review.

Comment on attachment 95679 [details]
Patch with changes to address comments

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

This looks very good! Here are some low-level comments. It would have been easier to review the patch if it included function-level comments in the change log. I would like to see those still. I am going to add some comments on naming and maybe some higher-level notes.

> Source/WebCore/ChangeLog:64
> +        * platform/graphics/chromium/FontCacheChromiumWin.cpp:
> +        (WebCore::FontCache::getFontDataForCharacters):
> +        * platform/graphics/freetype/FontCacheFreeType.cpp:
> +        (WebCore::FontCache::getFontDataForCharacters):
> +        * platform/graphics/haiku/FontCacheHaiku.cpp:
> +        (WebCore::FontCache::getFontDataForCharacters):
> +        * platform/graphics/mac/FontCacheMac.mm:
> +        (WebCore::FontCache::getFontDataForCharacters):
> +        * platform/graphics/qt/FontCacheQt.cpp:
> +        (WebCore::FontCache::getFontDataForCharacters):
> +        * platform/graphics/win/FontCacheWin.cpp:
> +        (WebCore::FontCache::getFontDataForCharacters):
> +        * platform/graphics/wince/FontCacheWinCE.cpp:
> +        * platform/graphics/wx/FontCacheWx.cpp:
> +        (WebCore::FontCache::getFontDataForCharacters):

All of these platform-specific implementations of getFontDataForCharacters() were changes in the exact same platform-agnostic way. A better way to make this change would have been:
1. Declare a new private member function in FontCache called platformGetFontDataForCharacters()
2. Rename all of the platform-specific implementations of getFontDataForCharacters to platformGetFontDataForCharacters
3. Implement getFontDataForCharacters in FontCache.cpp which calls the platform-specific function, then does the platform-agnostic part.

> Source/WebCore/page/FrameView.cpp:118
> +    , m_fontCacheAutoRelease(adoptPtr(new FontCacheAutoRelease))

Is there a reason to allocate this object separately? Its lifetime seems to coincide with that of the FrameView.

> Source/WebCore/page/FrameView.h:41
> +class FontCacheAutoRelease;

Maybe that’s the reason: not wanting to include another header here :)

> Source/WebCore/platform/graphics/FontCache.cpp:385
> +FontCacheAutoRelease::FontCacheAutoRelease()
> +{
> +}
> +

I think there’s not need to write a default constructor. The compiler generates one on its own.

> Source/WebCore/platform/graphics/FontCache.cpp:398
> +#ifndef NDEBUG

This should be #if !ASSERT_DISABLED

> Source/WebCore/platform/graphics/FontCache.cpp:411
> +    ReferencedFontDataMap::iterator mapIt = m_referencedFontDataMap.begin();
> +
> +    for (; mapIt != mapEnd; ++mapIt)

The convention is to declare the loop variable in side the for statement, and call it “it”.

> Source/WebCore/platform/graphics/FontCache.cpp:425
> +    ReferencedFontDataMap::iterator result = m_referencedFontDataMap.find(fontData);
> +    if (result != m_referencedFontDataMap.end()) {
> +        ++result->second;
> +        return;
> +    }
> +
> +    m_referencedFontDataMap.set(fontData, 1);

find() and set() entail two hash table lookups. A more efficient idiom is to use a single add() and check the boolean member of the result, which tells you if you got a new entry or an existing one.

> Source/WebCore/platform/graphics/GlyphPageTreeNode.cpp:380
> +    // Prune fall back child (if any) of this font.
> +    if (m_systemFallbackChild && m_systemFallbackChild->m_page)
> +        m_systemFallbackChild->m_page->clearForFontData(fontData);
> +

I am worried that the stopping condition further down in this function (level > fontData->maxGlyphPageTreeLevel()) isn’t accounting for system fallback pages, i.e. maxGlyphPageTreeLevel() is not set when a fontData is used in a fallback page. Am I wrong?

> Source/WebCore/platform/graphics/SimpleFontData.cpp:225
> +void SimpleFontData::DerivedFontData::pruneFromGlyphPageTree()
> +{
> +    if (!forCustomFont) {
> +        if (smallCaps)
> +            GlyphPageTreeNode::pruneTreeFontData(smallCaps.get());
> +        if (emphasisMark)
> +            GlyphPageTreeNode::pruneTreeFontData(emphasisMark.get());
> +        if (brokenIdeograph)
> +            GlyphPageTreeNode::pruneTreeFontData(brokenIdeograph.get());
> +        if (verticalRightOrientation)
> +            GlyphPageTreeNode::pruneTreeFontData(verticalRightOrientation.get());
> +        if (uprightOrientation)
> +            GlyphPageTreeNode::pruneTreeFontData(uprightOrientation.get());
> +
>          return;
> +    }

I don’t understand this part.

> Source/WebCore/platform/mac/MemoryPressureHandlerMac.mm:101
> +    fontCache()->purgeInactiveFontData();
> +

I think this change is not an integral part of fixing this bug. It should go in a separate patch.

Comment on attachment 95679 [details]
Patch with changes to address comments

While the general approach of this patch is good, I have two ideas for simplifying it. The first idea is: avoid multiple nested autorelease pools. In your design, nested pools correspond to nested frame. The theoretical advantage of nested pools over a single pool at the top-level frame is that it allows you to release fonts that are used only by a subframe before you exit the layout or paint operation for the parent frame. I think this advantage is purely theoretical, since the concern here is not instantaneous size of the font cache but rather long-term growth. So the first suggestion is: have push() only create a pool if there is none, otherwise have it increment a counter; have pop() decrement the counter and only destroy the pool when you hit 0.

But once you only one pool, maybe you don’t need a pool at all. The pool serves to protect the system fallback font data from being deleted prematurely. Here’s another, simpler way to do it: have a flag (or actually a counter, to allow nesting) on the font cache, forbidding pruning. When starting layout or painting, set the flag (or increment the counter), and when finishing, reset it (or decrement). When you get system fallback font data, release it *immediately*, and count on the font cache to not actually prune it until the end of the operation.

[Michael Saboff]

(In reply to comment #7)
> (From update of attachment 95679 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95679&action=review
> 
> This looks very good! Here are some low-level comments. It would have been easier to review the patch if it included function-level comments in the change log. I would like to see those still. I am going to add some comments on naming and maybe some higher-level notes.
> 
> > Source/WebCore/ChangeLog:64
> > +        * platform/graphics/chromium/FontCacheChromiumWin.cpp:
> > +        (WebCore::FontCache::getFontDataForCharacters):
> > +        * platform/graphics/freetype/FontCacheFreeType.cpp:
> > +        (WebCore::FontCache::getFontDataForCharacters):
> > +        * platform/graphics/haiku/FontCacheHaiku.cpp:
> > +        (WebCore::FontCache::getFontDataForCharacters):
> > +        * platform/graphics/mac/FontCacheMac.mm:
> > +        (WebCore::FontCache::getFontDataForCharacters):
> > +        * platform/graphics/qt/FontCacheQt.cpp:
> > +        (WebCore::FontCache::getFontDataForCharacters):
> > +        * platform/graphics/win/FontCacheWin.cpp:
> > +        (WebCore::FontCache::getFontDataForCharacters):
> > +        * platform/graphics/wince/FontCacheWinCE.cpp:
> > +        * platform/graphics/wx/FontCacheWx.cpp:
> > +        (WebCore::FontCache::getFontDataForCharacters):
> 
> All of these platform-specific implementations of getFontDataForCharacters() were changes in the exact same platform-agnostic way. A better way to make this change would have been:
> 1. Declare a new private member function in FontCache called platformGetFontDataForCharacters()
> 2. Rename all of the platform-specific implementations of getFontDataForCharacters to platformGetFontDataForCharacters
> 3. Implement getFontDataForCharacters in FontCache.cpp which calls the platform-specific function, then does the platform-agnostic part.

Your suggested approach would need some modifications.  Some of the platform specific implementations return fonts that don't come from the font cache.  We would need to handle and ignore the release of a font that wasn't in the cache for your suggestion to work.  At least one platform specific getFontDataForCharacters() returns 0, which requires some handling as well.

> > Source/WebCore/page/FrameView.cpp:118
> > +    , m_fontCacheAutoRelease(adoptPtr(new FontCacheAutoRelease))
> 
> Is there a reason to allocate this object separately? Its lifetime seems to coincide with that of the FrameView.
> 
> > Source/WebCore/page/FrameView.h:41
> > +class FontCacheAutoRelease;
> 
> Maybe that’s the reason: not wanting to include another header here :)

Not wanting to create a new header dependency and not wanting to bloat the FrameView object where my reasons for a pointer.
 
... <comments deleted as I agree with them> ...

> > Source/WebCore/platform/graphics/GlyphPageTreeNode.cpp:380
> > +    // Prune fall back child (if any) of this font.
> > +    if (m_systemFallbackChild && m_systemFallbackChild->m_page)
> > +        m_systemFallbackChild->m_page->clearForFontData(fontData);
> > +
> 
> I am worried that the stopping condition further down in this function (level > fontData->maxGlyphPageTreeLevel()) isn’t accounting for system fallback pages, i.e. maxGlyphPageTreeLevel() is not set when a fontData is used in a fallback page. Am I wrong?

I'll look into this.  You may be right.

> > Source/WebCore/platform/graphics/SimpleFontData.cpp:225
> > +void SimpleFontData::DerivedFontData::pruneFromGlyphPageTree()
> > +{
> > +    if (!forCustomFont) {
> > +        if (smallCaps)
> > +            GlyphPageTreeNode::pruneTreeFontData(smallCaps.get());
> > +        if (emphasisMark)
> > +            GlyphPageTreeNode::pruneTreeFontData(emphasisMark.get());
> > +        if (brokenIdeograph)
> > +            GlyphPageTreeNode::pruneTreeFontData(brokenIdeograph.get());
> > +        if (verticalRightOrientation)
> > +            GlyphPageTreeNode::pruneTreeFontData(verticalRightOrientation.get());
> > +        if (uprightOrientation)
> > +            GlyphPageTreeNode::pruneTreeFontData(uprightOrientation.get());
> > +
> >          return;
> > +    }
> 
> I don’t understand this part.

We can use system fallback fonts for variants.  Therefore we need to prune the tree of those variants.  The prior code only handled the custom font case.  I added the non-custom case.

> > Source/WebCore/platform/mac/MemoryPressureHandlerMac.mm:101
> > +    fontCache()->purgeInactiveFontData();
> > +
> 
> I think this change is not an integral part of fixing this bug. It should go in a separate patch.

This is the main reason for the change, but I can move it to another bug & patch.

(In reply to comment #8)
> (From update of attachment 95679 [details])
> While the general approach of this patch is good, I have two ideas for simplifying it. The first idea is: avoid multiple nested autorelease pools. In your design, nested pools correspond to nested frame. The theoretical advantage of nested pools over a single pool at the top-level frame is that it allows you to release fonts that are used only by a subframe before you exit the layout or paint operation for the parent frame. I think this advantage is purely theoretical, since the concern here is not instantaneous size of the font cache but rather long-term growth. So the first suggestion is: have push() only create a pool if there is none, otherwise have it increment a counter; have pop() decrement the counter and only destroy the pool when you hit 0.
> 
> But once you only one pool, maybe you don’t need a pool at all. The pool serves to protect the system fallback font data from being deleted prematurely. Here’s another, simpler way to do it: have a flag (or actually a counter, to allow nesting) on the font cache, forbidding pruning. When starting layout or painting, set the flag (or increment the counter), and when finishing, reset it (or decrement). When you get system fallback font data, release it *immediately*, and count on the font cache to not actually prune it until the end of the operation.

Although the current use doesn't use the nesting, it may be useful.  As I was looking for the appropriate level in the layout / paint call graph to insert FontCacheAutoRelease objects, I did nest. 

I don't think immediately releasing a ref on a system fallback font will work as you describe.  The current code allows for pruning fonts as soon as the last autorelease pool (i.e. FrameView) using the font is destructed.  I think that your approach would require that all FrameViews are closed since you don't have a ref count for each using FrameView.

[Michael Saboff]

Created attachment 95968 [details]
Patch with rework suggested by reviewers

Changed to use the single pool suggested in comment #8.  Added the appropriate changes for the other platform specific FontCache::getFontDataForCharacters.  Moved the purgeFontCache check to when the global auto release count goes to 0.  Lowered the threshold for purging.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/FontFastPath.cpp:40:  Use 'using namespace std;' instead of 'using std::max;'.  [build/using_std] [4]
Total errors found: 1 in 25 files


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

[Simon Fraser (smfr)]

Comment on attachment 95968 [details]
Patch with rework suggested by reviewers

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

> Source/WebCore/ChangeLog:8
> +        This change allows fonts allocated as system fallback fonts to be release.

released

> Source/WebCore/ChangeLog:10
> +        Previously, the reference counts for these fonts grew without bound.  This
> +        is implemented primarily an auto release class that wraps accesses to the

Sentence doesn't parse.

> Source/WebCore/ChangeLog:11
> +        cache for system fallback fonts. All such access are via the method

accesses

> Source/WebCore/ChangeLog:13
> +        FontCacheAutoRelease. When such an object exists, it protects these font from 

these fonts

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1878
> +    // Protects and automatically releases system fallback fonts from font cache

I don't think the comment is necessary.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1908
> +    // Protects and automatically releases system fallback fonts from font cache

Ditto

> Source/WebCore/platform/graphics/FontCache.h:103
> +    inline void incDontPurgeCount() { m_dontPurgeCount++; }
> +    inline void decDontPurgeCount()

These methods are too easily misread as "increment/decrement but don't purge the count"

> Source/WebCore/platform/graphics/SimpleFontData.cpp:213
> +#if 0

Why the #if 0?

[Alexey Proskuryakov]

Comment on attachment 95968 [details]
Patch with rework suggested by reviewers

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

> Source/WebKit/mac/Misc/WebStringTruncator.mm:98
> +    // Protects and automatically releases system fallback fonts from font cache
> +    FontCacheAutoRelease fontCacheAutoRelease;

I don't have any useful comments, but this class name positively needs be good enough to avoid commenting each time it's used.

[Michael Saboff]

Created attachment 96000 [details]
Updated patch to address comments #12 & #13

Comment on attachment 96000 [details]
Updated patch to address comments #12 & #13

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

> Source/WebCore/ChangeLog:73
> +        * WebCore.exp.in:
> +        * html/canvas/CanvasRenderingContext2D.cpp:
> +        (WebCore::CanvasRenderingContext2D::measureText):
> +        (WebCore::CanvasRenderingContext2D::drawTextInternal):
> +        * page/FrameView.cpp:
> +        (WebCore::FrameView::layout):
> +        (WebCore::FrameView::paintContents):
> +        * platform/graphics/FontCache.cpp:
> +        (WebCore::FontCache::FontCache):
> +        (WebCore::FontCache::getCachedFontData):
> +        (WebCore::FontCache::releaseFontData):
> +        (WebCore::FontCache::checkForFontsToPurge):
> +        (WebCore::FontCache::purgeInactiveFontData):
> +        (WebCore::FontCache::derefForPurgeProtectedFont):
> +        * platform/graphics/FontCache.h:
> +        (WebCore::FontCache::incPurgePreventCount):
> +        (WebCore::FontCache::decPurgePreventCount):
> +        (WebCore::FontCachePurgePreventer::FontCachePurgePreventer):
> +        (WebCore::FontCachePurgePreventer::~FontCachePurgePreventer):
> +        * platform/graphics/FontFastPath.cpp:
> +        (WebCore::Font::glyphDataForCharacter):
> +        * platform/graphics/GlyphPage.h:
> +        (WebCore::GlyphPage::clearForFontData):
> +        * platform/graphics/GlyphPageTreeNode.cpp:
> +        (WebCore::GlyphPageTreeNode::pruneFontData):
> +        * platform/graphics/SimpleFontData.cpp:
> +        (WebCore::SimpleFontData::~SimpleFontData):
> +        (WebCore::SimpleFontData::DerivedFontData::~DerivedFontData):
> +        (WebCore::SimpleFontData::DerivedFontData::pruneFromGlyphPageTree):
> +        * platform/graphics/SimpleFontData.h:
> +        * platform/graphics/chromium/FontCacheChromiumWin.cpp:
> +        (WebCore::FontCache::getFontDataForCharacters):
> +        * platform/graphics/freetype/FontCacheFreeType.cpp:
> +        (WebCore::FontCache::getFontDataForCharacters):
> +        * platform/graphics/haiku/FontCacheHaiku.cpp:
> +        (WebCore::FontCache::getFontDataForCharacters):
> +        * platform/graphics/mac/FontCacheMac.mm:
> +        (WebCore::FontCache::getFontDataForCharacters):
> +        * platform/graphics/qt/FontCacheQt.cpp:
> +        (WebCore::FontCache::getFontDataForCharacters):
> +        * platform/graphics/win/FontCacheWin.cpp:
> +        (WebCore::FontCache::getFontDataForCharacters):
> +        * platform/graphics/wince/FontCacheWinCE.cpp:
> +        * platform/graphics/wx/FontCacheWx.cpp:
> +        (WebCore::FontCache::getFontDataForCharacters):
> +        * rendering/RenderImage.cpp:
> +        (WebCore::RenderImage::setImageSizeForAltText):
> +        * rendering/RenderListBox.cpp:
> +        (WebCore::RenderListBox::updateFromElement):
> +        (WebCore::RenderListBox::paintItemForeground):

Function-level change log comments are encouraged.

> Source/WebCore/platform/graphics/FontCache.cpp:-300
> -        if (gInactiveFontData->size() > cMaxInactiveFontData)
> -            purgeInactiveFontData(gInactiveFontData->size() - cTargetInactiveFontData);

Without a change log comment, I have no idea why you didn’t replace this with a call to checkForFontsToPurge()

> Source/WebCore/platform/graphics/FontCache.cpp:304
> +void FontCache::checkForFontsToPurge()

I would call this purgeInactiveFontDataIfNeeded(), because it doesn’t only check, it also purges if needed.

> Source/WebCore/platform/graphics/FontCache.cpp:373
> +void FontCache::derefForPurgeProtectedFont(const SimpleFontData* fontData)

A better name for this function would use the word “release” instead of the non-word “deref”. There’s no reason to introduce a new word to mean the same thing here.

> Source/WebCore/platform/graphics/FontCache.h:52
> +class FontCache;

Please keep this list sorted.

> Source/WebCore/platform/graphics/FontCache.h:63
> -    
> + 

As long as you’re removing whitespace, remove all of it.

> Source/WebCore/platform/graphics/FontCache.h:103
> +    inline void incPurgePreventCount() { m_purgePreventCount++; }
> +    inline void decPurgePreventCount()

We prefer complete words over abbreviations like “inc” and “dec”. It is also common to just call such methods “disable” and “enable” respectively (e.g. disablePurging() and enablePurging() or something similar).

We typically don’t use “inline” for member functions defined in the header. Is there a reason to do so here?

> Source/WebCore/platform/graphics/FontCache.h:104
> +    { 

You can (and should) assert here that the count is greater than zero so that no one is over-enabling.

> Source/WebCore/platform/graphics/FontCache.h:138
> +    inline FontCachePurgePreventer() { fontCache()->incPurgePreventCount(); }
> +    inline ~FontCachePurgePreventer() { fontCache()->decPurgePreventCount(); }

Don’t think “inline” does anything here.

> Source/WebCore/platform/graphics/FontFastPath.cpp:40
> +using std::max;

WebKit style is to add “using namespace std” instead of an individual member.

> Source/WebCore/platform/graphics/GlyphPage.h:127
> +        for (size_t i = 0; i < size; i++) {

The convention is to pre-decrement the loop variable (even though it doesn’t matter for POD types).

> Source/WebCore/platform/graphics/GlyphPageTreeNode.cpp:383
> -    if (child == m_children.end()) {
> -        // If there is no level-1 node for fontData, then there is no deeper node for it in this tree.
> -        if (!level)
> -            return;
> -    } else {
> +    if (child != m_children.end()) {

It is sad to lose this optimization, which still applies to non-fallback fonts. If this function took a parameter telling it whether the pruning is for a system fallback font or not, it could still apply the optimization in the common case.

> Source/WebCore/platform/graphics/SimpleFontData.cpp:211
>  SimpleFontData::DerivedFontData::~DerivedFontData()
>  {
> +}
> +
> +void SimpleFontData::DerivedFontData::pruneFromGlyphPageTree()
> +{

Without a change log comment explaining this, I’m still failing to see the case where you want to destroy a DerivedFontData for a non-custom font and don’t want to prune it.

> Source/WebCore/platform/graphics/wx/FontCacheWx.cpp:50
> -    
> + 

Whitespace removal 75% complete.

> Source/WebCore/rendering/RenderListBox.cpp:365
> +    FontCachePurgePreventer fontCachePurgePreventer;
> +    

It’s not clear to me when this can get called outside of FrameView::paintContents()

Comment on attachment 96000 [details]
Updated patch to address comments #12 & #13

What about WebKit/win/WebKitGraphics.cpp ?

[Michael Saboff]

Created attachment 96057 [details]
Updated patch to address comments 15 & 16

(In reply to comment #15)

Made changes in response to ell comments except:

> > Source/WebCore/platform/graphics/FontCache.cpp:-300
> > -        if (gInactiveFontData->size() > cMaxInactiveFontData)
> > -            purgeInactiveFontData(gInactiveFontData->size() - cTargetInactiveFontData);
> 
> Without a change log comment, I have no idea why you didn’t replace this with a call to checkForFontsToPurge()

Check needed to be moved outside of getFontData since since almost all calls to getCachedFontData() now happen when purging is not allowed.  Comment to this effect added to ChangeLog.

> > Source/WebCore/platform/graphics/GlyphPageTreeNode.cpp:383
> > -    if (child == m_children.end()) {
> > -        // If there is no level-1 node for fontData, then there is no deeper node for it in this tree.
> > -        if (!level)
> > -            return;
> > -    } else {
> > +    if (child != m_children.end()) {
> 
> It is sad to lose this optimization, which still applies to non-fallback fonts. If this function took a parameter telling it whether the pruning is for a system fallback font or not, it could still apply the optimization in the common case.

Unfortunately we don't have data keeping track of this. From what I saw during testing, a font often serve as a non-fallback and fallback font at the same time.  Given that font purging is relatively rare, I don't think there is much benefit to this.

> > Source/WebCore/rendering/RenderListBox.cpp:365
> > +    FontCachePurgePreventer fontCachePurgePreventer;
> > +    
> 
> It’s not clear to me when this can get called outside of FrameView::paintContents()

This and other paths that are protected became apparent when running the webkit tests with a debug build via the ASSERT in FontCache::releasePurgeProtectedFont.  Most of these non-frameView cases are needed for editing code paths.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/FontCache.cpp:307:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 1 in 26 files


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

[Michael Saboff]

Created attachment 96060 [details]
Prior patch with fixed debug code caught by stylebot

[Geoffrey Garen]

Dan is still your best reviewer here, but I noticed these inline declarations he mentioned, which you can remove:

> +    inline FontCachePurgePreventer() { fontCache()->disablePurging(); }
> +    inline ~FontCachePurgePreventer() { fontCache()->enablePurging(); }



> +    const SimpleFontData* fontData = getCachedFontData(&data);
> +    releasePurgeProtectedFont(fontData);

I'm very unclear on when a call to releasePurgeProtectedFont() is required or, if not required, desirable. The "retain and immediately release" idiom seen here is very odd to me. If I don't need to retain the object, why am I calling a retaining API? If I do need to retain the object, why is releasing it OK?

Perhaps the default cache behavior should be to ASSERT that purging is prohibited, and return a font without retaining, while the few clients that do explicitly retain their fonts can call a special function that does not ASSERT that purging is prohibited, and does a retain instead:

getCachedFontData(FontPlatformData*); // ASSERTs that purging is prohibited, does not retain &data. Returned SimpleFontData* is implicitly owned by the current FontCachePurgePreventer scope. 

retainCachedFontData(FontPlatformData*); // Retains returned SimpleFontData*, does not ASSERT that purging is prohibited. Returned SimpleFontData* is explicitly owned by the caller.
releaseCachedFontData(SimpleFontData*); // Releases SimpleFontData* obtained by retainCachedFontData.

I think this default would be congruent with the code's intention, given your comment:

> almost all calls to getCachedFontData() now happen when purging is not allowed.

In such a design a "FontCachePurgePreventer" is really a "CachedFontDataOwner" or "CachedFontDataScope", since it implicitly owns all cached font data accessed within its scope.

[Michael Saboff]

Created attachment 96254 [details]
Patch with updates in response to comment #20

(In reply to comment #20)
> Dan is still your best reviewer here, but I noticed these inline declarations he mentioned, which you can remove:
> 
> > +    inline FontCachePurgePreventer() { fontCache()->disablePurging(); }
> > +    inline ~FontCachePurgePreventer() { fontCache()->enablePurging(); }

This was an oversight and has been fixed.

> > +    const SimpleFontData* fontData = getCachedFontData(&data);
> > +    releasePurgeProtectedFont(fontData);
> 
> I'm very unclear on when a call to releasePurgeProtectedFont() is required or, if not required, desirable. The "retain and immediately release" idiom seen here is very odd to me. If I don't need to retain the object, why am I calling a retaining API? If I do need to retain the object, why is releasing it OK?

It was being called only by platform specific FontCache::getFontDataForCharacters(), the method used to access system fallback fonts.

To make it less confusing, I removed releasePurgeProtectedFont() and added a "shouldRetain" parameter to getCachedFontData() with a default value of true.  This change also eliminated one hash lookup (find()) for the system fallback font case.

> Perhaps the default cache behavior should be to ASSERT that purging is prohibited, and return a font without retaining, while the few clients that do explicitly retain their fonts can call a special function that does not ASSERT that purging is prohibited, and does a retain instead:
> 
> getCachedFontData(FontPlatformData*); // ASSERTs that purging is prohibited, does not retain &data. Returned SimpleFontData* is implicitly owned by the current FontCachePurgePreventer scope. 
> 
> retainCachedFontData(FontPlatformData*); // Retains returned SimpleFontData*, does not ASSERT that purging is prohibited. Returned SimpleFontData* is explicitly owned by the caller.
> releaseCachedFontData(SimpleFontData*); // Releases SimpleFontData* obtained by retainCachedFontData.
> 
> I think this default would be congruent with the code's intention, given your comment:
> 
> > almost all calls to getCachedFontData() now happen when purging is not allowed.
> 
> In such a design a "FontCachePurgePreventer" is really a "CachedFontDataOwner" or "CachedFontDataScope", since it implicitly owns all cached font data accessed within its scope.

Most font requests to the cache rely on the reference counting.  It is only the system fallback font accesses from the cache that needs the purge prevention handling and no change to reference count. It is the case though that these two cache requests are intermingled and therefore purging is disallowed even if we are requesting the font for the normal reference counted paths.

Although FontCachePurgePreventer can be seen as an "owner", a better description is that a font can only be released if its reference count is zero AND there isn't an active FontCachePurgePreventer.  It is likely the case that a font needs both types of protection, as a font can be accessed and actively used via a reference counted path as well as via the system fallback access path for the same time period.

[Geoffrey Garen]

> bool shouldRetain

WebKit style prefers enum over bool for function arguments that are not typically named, so you can tell just by reading at the call site what the meaning of the argument is. For example, here you could use "enum { Retain, DoNotRetain }".

Otherwise, this patch is looking good to me. Alexey, Simon, Dan, do you have further comments?

(In reply to comment #22)
> > bool shouldRetain
> 
> WebKit style prefers enum over bool for function arguments that are not typically named, so you can tell just by reading at the call site what the meaning of the argument is. For example, here you could use "enum { Retain, DoNotRetain }".

I like this suggestion.

> 
> Otherwise, this patch is looking good to me. Alexey, Simon, Dan, do you have further comments?

I have nothing further.

[Geoffrey Garen]

Comment on attachment 96254 [details]
Patch with updates in response to comment #20

OK, r=me with the change for "enum { Retain, DoNotRetain }".

[Michael Saboff]

Committed r88260: <http://trac.webkit.org/changeset/88260>

[Ryosuke Niwa]

It seems like this patch broke Windows builds:
http://build.webkit.org/builders/Windows%20Release%20%28Build%29/builds/17281/steps/compile-webkit/logs/stdio

3>..\platform\graphics\win\FontCacheWin.cpp(288) : error C2664: 'WebCore::SimpleFontData *WebCore::FontCache::getCachedFontData(const WebCore::FontDescription &,const WTF::AtomicString &,bool,WebCore::FontCache::ShouldRetain)' : cannot convert parameter 1 from 'WebCore::FontPlatformData *' to 'const WebCore::FontDescription &'
3>        Reason: cannot convert from 'WebCore::FontPlatformData *' to 'const WebCore::FontDescription'
3>        No constructor could take the source type, or constructor overload resolution was ambiguous
3>SVGCSSStyleSelector.cpp

[James Robinson]

This is in the regression range for a significant hit on many of chromium's page cyclers on windows, linux, and mac.  The full regression range is http://trac.webkit.org/log/?rev=88266&stop_rev=88259&verbose=on, but the other changes look fairly harmless.

You can see the jump on one of our graphs here: http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/moz/report.html?history=50&rev=88279 - the X axis here is sadly chromium checkins, but the jump from "r88196 - r88210" is actually the WebKit range mentioned above.  The magnitude of the perf hit varies depending on the bot configuration, etc, but is at least 10% in some of our tests.

[Michael Saboff]

(In reply to comment #27)
> This is in the regression range for a significant hit on many of chromium's page cyclers on windows, linux, and mac.  The full regression range is http://trac.webkit.org/log/?rev=88266&stop_rev=88259&verbose=on, but the other changes look fairly harmless.
> 
> You can see the jump on one of our graphs here: http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/moz/report.html?history=50&rev=88279 - the X axis here is sadly chromium checkins, but the jump from "r88196 - r88210" is actually the WebKit range mentioned above.  The magnitude of the perf hit varies depending on the bot configuration, etc, but is at least 10% in some of our tests.

You can try adjusting the two constants cMaxInactiveFontData and cTargetInactiveFontData in FontCache.cpp and rerun your tests.  The change in the patch included lowering those from 120 and 100 respectively to 50 and 30.  This was done to reduce the memory footprint.  If your tests have a large number of pages, especially those using non-latin fonts, I could see how this would impact performance especially if it takes a long time for the platform code and OS to bring a font in to webcore.

These constants govern inactive fonts, not total fonts. With this change, system fallback fonts are now likely to be inactive where they previously stayed active and could be abandoned. I would be reluctant in general to increase these constants as 50 and 30 appear reasonable in our tests and seem to work well in real world browsing.  Keeping up to 119 inactive fonts can waste quite a bit of memory.

[James Robinson]

(In reply to comment #28)
> (In reply to comment #27)
> > This is in the regression range for a significant hit on many of chromium's page cyclers on windows, linux, and mac.  The full regression range is http://trac.webkit.org/log/?rev=88266&stop_rev=88259&verbose=on, but the other changes look fairly harmless.
> > 
> > You can see the jump on one of our graphs here: http://build.chromium.org/f/chromium/perf/linux-release-webkit-latest/moz/report.html?history=50&rev=88279 - the X axis here is sadly chromium checkins, but the jump from "r88196 - r88210" is actually the WebKit range mentioned above.  The magnitude of the perf hit varies depending on the bot configuration, etc, but is at least 10% in some of our tests.
> 
> You can try adjusting the two constants cMaxInactiveFontData and cTargetInactiveFontData in FontCache.cpp and rerun your tests.  The change in the patch included lowering those from 120 and 100 respectively to 50 and 30.  This was done to reduce the memory footprint.  If your tests have a large number of pages, especially those using non-latin fonts, I could see how this would impact performance especially if it takes a long time for the platform code and OS to bring a font in to webcore.
> 
> These constants govern inactive fonts, not total fonts. With this change, system fallback fonts are now likely to be inactive where they previously stayed active and could be abandoned. I would be reluctant in general to increase these constants as 50 and 30 appear reasonable in our tests and seem to work well in real world browsing.  Keeping up to 119 inactive fonts can waste quite a bit of memory.

There's a significant regression even on tests that don't use a significant number of fonts.  Did you measure the performance impact of the change from 120/100 to 50/30 independently of the other changes in this patch?

[Michael Saboff]

> There's a significant regression even on tests that don't use a significant number of fonts.  Did you measure the performance impact of the change from 120/100 to 50/30 independently of the other changes in this patch?

Do you know if the simpler tests end up using fallback fonts or how many fonts are actually end up in the FontCache during the tests?

I changed the constants from 120/100 to 50/30 late in the development of the patch.  I measured page load times as well as memory usage with the changes before and after making the constant changes.  I didn't see any performance changes in our tests with the constant changes.  For the whole patch, I did see a slight slowdown in some tests, on the order of a 1-3% IIRC.  I saw no impact on other tests.

Remember that this fixes a memory abandonment issue, fonts get added to the cache and their reference counts grow without bound.

[Michael Saboff]

Just reran the page loading tests that I earlier saw the 1-3% regression.  I compared webkit r88258 (two changes before) with webkit r88273 (13 changes after).  There is a regression of 0.2% comparing the average of 25 runs of the test. I consider this to be in the noise.   I cannot replicate the performance degradation you are seeing.

[James Robinson]

The results from page cyclers will depend entirely on the test data, of course.  Over the page cycler data sets we use in chromium the inactive font data cache grows to between 60 and 105.  None of the page cyclers we use will complete without eviction with a threshhold of 50.  Are the values 50/30 based off of user data?

I've confirmed that restoring the previous values of 120/100 restores performance to previous levels.  Since this regression is well over the magnitude that we'd consider acceptable in chromium, I'm inclined to restore the old values, behind #if PLATFORM(CHROMIUM) if you would like to keep the 50/30 values for other platforms.  I can also try to gather some data from users in the wild about how many fonts people have in the cache in general.  It would be useful if you have any other data to share about how those values were arrived at, or about how significant the resource use delta is on average between 120/100 and 50/30 on various platforms.