From 2ae0134b4c490b5b51106e9e7417e3b43dc228c3 Mon Sep 17 00:00:00 2001 From: Andrew Osmond Date: Fri, 29 Jul 2022 14:48:38 +0000 Subject: [PATCH] Bug 1780193 - Refactor gfxFontCache expiration behaviour to mitigate perf reductions. r=jfkthame In this patch, we make gfxFont use normal AddRef/Release semantics, and instead now hold a strong reference to it inside the cache. At all times a font should be in the expiration tracker, and whenever a lookup is performed, we mark it as used. When the tracker attempts to expire an entry, we check to see if the only strong reference is inside the cache itself. If so, it is deleted. If not, we mark it as used again instead of expiring it. This has the advantage of making gfxFont::Release cheaper when there is only one outstanding reference left outside the cache. It used to acquire the gfxFontCache mutex in order to reinsert the font into the expiration tracker when the last strong reference was cleared. This is a very common case since the typical use case is only the main thread is interacting with the cache, one font reference at a time. This does not completely restore us to the previous performance observed, but it does reclaim half of the original design benefit while remaining threadsafe. Differential Revision: https://phabricator.services.mozilla.com/D153110 --- gfx/thebes/gfxDWriteFonts.h | 3 +- gfx/thebes/gfxFT2FontBase.h | 2 +- gfx/thebes/gfxFT2Fonts.h | 5 +- gfx/thebes/gfxFcPlatformFontList.h | 2 +- gfx/thebes/gfxFont.cpp | 155 ++++++++++------------------- gfx/thebes/gfxFont.h | 83 +++++---------- gfx/thebes/gfxFontEntry.cpp | 23 ++--- gfx/thebes/gfxGDIFont.h | 6 +- gfx/thebes/gfxMacFont.h | 6 +- gfx/thebes/gfxPlatform.cpp | 2 +- gfx/thebes/gfxPlatformFontList.cpp | 2 +- 11 files changed, 103 insertions(+), 186 deletions(-) diff --git a/gfx/thebes/gfxDWriteFonts.h b/gfx/thebes/gfxDWriteFonts.h index 40a1e5e270db..a1b2390add32 100644 --- a/gfx/thebes/gfxDWriteFonts.h +++ b/gfx/thebes/gfxDWriteFonts.h @@ -28,7 +28,6 @@ class gfxDWriteFont final : public gfxFont { gfxFontEntry* aFontEntry, const gfxFontStyle* aFontStyle, RefPtr aFontFace = nullptr, AntialiasOption = kAntialiasDefault); - ~gfxDWriteFont(); static bool InitDWriteSupport(); @@ -74,6 +73,8 @@ class gfxDWriteFont final : public gfxFont { bool ShouldRoundXOffset(cairo_t* aCairo) const override; protected: + ~gfxDWriteFont() override; + const Metrics& GetHorizontalMetrics() const override { return mMetrics; } bool GetFakeMetricsForArialBlack(DWRITE_FONT_METRICS* aFontMetrics); diff --git a/gfx/thebes/gfxFT2FontBase.h b/gfx/thebes/gfxFT2FontBase.h index b04d4b6c6c3e..61c16b775085 100644 --- a/gfx/thebes/gfxFT2FontBase.h +++ b/gfx/thebes/gfxFT2FontBase.h @@ -45,7 +45,6 @@ class gfxFT2FontBase : public gfxFont { const RefPtr& aUnscaledFont, RefPtr&& aFTFace, gfxFontEntry* aFontEntry, const gfxFontStyle* aFontStyle, int aLoadFlags, bool aEmbolden); - virtual ~gfxFT2FontBase(); uint32_t GetGlyph(uint32_t aCharCode) { auto* entry = static_cast(mFontEntry.get()); @@ -85,6 +84,7 @@ class gfxFT2FontBase : public gfxFont { mozilla::gfx::IntRect* aBounds = nullptr) const; protected: + ~gfxFT2FontBase() override; void InitMetrics(); const Metrics& GetHorizontalMetrics() const override { return mMetrics; } FT_Vector GetEmboldenStrength(FT_Face aFace) const; diff --git a/gfx/thebes/gfxFT2Fonts.h b/gfx/thebes/gfxFT2Fonts.h index 9d546b18e88d..3b164c711ef5 100644 --- a/gfx/thebes/gfxFT2Fonts.h +++ b/gfx/thebes/gfxFT2Fonts.h @@ -16,13 +16,12 @@ class FT2FontEntry; -class gfxFT2Font : public gfxFT2FontBase { +class gfxFT2Font final : public gfxFT2FontBase { public: // new functions gfxFT2Font(const RefPtr& aUnscaledFont, RefPtr&& aFTFace, FT2FontEntry* aFontEntry, const gfxFontStyle* aFontStyle, int aLoadFlags); - virtual ~gfxFT2Font(); FT2FontEntry* GetFontEntry(); @@ -37,6 +36,8 @@ class gfxFT2Font : public gfxFT2FontBase { FontCacheSizes* aSizes) const override; protected: + ~gfxFT2Font() override; + struct CachedGlyphData { CachedGlyphData() : glyphIndex(0xffffffffU) {} diff --git a/gfx/thebes/gfxFcPlatformFontList.h b/gfx/thebes/gfxFcPlatformFontList.h index 90f62a47d6d9..f1cb705d3998 100644 --- a/gfx/thebes/gfxFcPlatformFontList.h +++ b/gfx/thebes/gfxFcPlatformFontList.h @@ -228,7 +228,7 @@ class gfxFontconfigFont final : public gfxFT2FontBase { bool ShouldHintMetrics() const override; private: - virtual ~gfxFontconfigFont(); + ~gfxFontconfigFont() override; RefPtr mPattern; }; diff --git a/gfx/thebes/gfxFont.cpp b/gfx/thebes/gfxFont.cpp index 7407635707f9..9990e0219e4a 100644 --- a/gfx/thebes/gfxFont.cpp +++ b/gfx/thebes/gfxFont.cpp @@ -212,14 +212,8 @@ gfxFontCache::~gfxFontCache() { mWordCacheExpirationTimer = nullptr; } - // Expire everything that has a zero refcount, so we don't leak them. - AgeAllGenerations(); - // All fonts should be gone. - NS_WARNING_ASSERTION(mFonts.Count() == 0, - "Fonts still alive while shutting down gfxFontCache"); - // Note that we have to delete everything through the expiration - // tracker, since there might be fonts not in the hashtable but in - // the tracker. + // Expire everything manually so we don't leak them. + Flush(); } bool gfxFontCache::HashEntry::KeyEquals(const KeyTypePointer aKey) const { @@ -246,84 +240,61 @@ already_AddRefed gfxFontCache::Lookup( } RefPtr font = entry->mFont; - if (font->GetExpirationState()->IsTracked()) { - RemoveObjectLocked(font, lock); - } + MarkUsedLocked(font, lock); return font.forget(); } -void gfxFontCache::AddNew(gfxFont* aFont) { - nsTArray discard; - { - MutexAutoLock lock(mMutex); +already_AddRefed gfxFontCache::MaybeInsert(RefPtr&& aFont) { + MOZ_ASSERT(aFont); + MutexAutoLock lock(mMutex); - Key key(aFont->GetFontEntry(), aFont->GetStyle(), - aFont->GetUnicodeRangeMap()); - HashEntry* entry = mFonts.PutEntry(key); - if (!entry) { - return; - } - gfxFont* oldFont = entry->mFont; - entry->mFont = aFont; + Key key(aFont->GetFontEntry(), aFont->GetStyle(), + aFont->GetUnicodeRangeMap()); + HashEntry* entry = mFonts.PutEntry(key); + if (!entry) { + return aFont.forget(); + } + + // If it is null, then we are inserting a new entry. Otherwise we are + // attempting to replace an existing font, probably due to a thread race, in + // which case stick with the original font. + if (!entry->mFont) { + entry->mFont = std::move(aFont); + AddObjectLocked(entry->mFont, lock); // Assert that we can find the entry we just put in (this fails if the key // has a NaN float value in it, e.g. 'sizeAdjust'). MOZ_ASSERT(entry == mFonts.GetEntry(key)); - - // If someone's asked us to replace an existing font entry, then that's a - // bit weird, but let it happen, and expire the old font if it's not used. - if (oldFont && oldFont->GetExpirationState()->IsTracked()) { - // if oldFont == aFont, recount should be > 0, - // so we shouldn't be here. - NS_ASSERTION(aFont != oldFont, "new font is tracked for expiry!"); - NotifyExpiredLocked(oldFont, lock); - discard = std::move(mTrackerDiscard); - } - } - DestroyDiscard(discard); -} - -void gfxFontCache::NotifyReleased(gfxFont* aFont) { - nsTArray discard; - { - MutexAutoLock lock(mMutex); - - // Ensure nothing pulled a strong reference out of the cache during the last - // release. The only raw reference remaining should be in this cache, so - // if it remains zero while we hold the lock, we know it is safe to add it - // to the tracker. - // - // We may also be racing to reinsert the font into the cache, given another - // thread could have not only done the lookup, but also released it before - // we were able to process the first zero count release. If it is already - // tracked, then we have nothing to do. - if (aFont->GetRefCount() > 0 || aFont->GetExpirationState()->IsTracked()) { - return; - } - - if (NS_SUCCEEDED(AddObjectLocked(aFont, lock))) { - return; - } - - // We couldn't track it for some reason. Kill it now. - DestroyFontLocked(aFont); - discard = std::move(mTrackerDiscard); + } else { + MarkUsedLocked(entry->mFont, lock); } - DestroyDiscard(discard); - // Note that we might have fonts that aren't in the hashtable, perhaps because - // of OOM adding to the hashtable or because someone did an AddNew where - // we already had a font. These fonts are added to the expiration tracker - // anyway, even though Lookup can't resurrect them. Eventually they will - // expire and be deleted. + return do_AddRef(entry->mFont); } void gfxFontCache::NotifyExpiredLocked(gfxFont* aFont, const AutoLock& aLock) { + // If there are outstanding references to the font outside the cache, we + // should just mark it as used. + if (aFont->GetRefCount() > 1) { + MarkUsedLocked(aFont, aLock); + return; + } + + // We should have the last reference to the font. RemoveObjectLocked(aFont, aLock); - DestroyFontLocked(aFont); + Key key(aFont->GetFontEntry(), aFont->GetStyle(), + aFont->GetUnicodeRangeMap()); + HashEntry* entry = mFonts.GetEntry(key); + if (!entry || entry->mFont != aFont) { + MOZ_ASSERT_UNREACHABLE("Invalid font?"); + return; + } + + mTrackerDiscard.AppendElement(std::move(entry->mFont)); + mFonts.RemoveEntry(entry); } void gfxFontCache::NotifyHandlerEnd() { - nsTArray discard; + nsTArray> discard; { MutexAutoLock lock(mMutex); discard = std::move(mTrackerDiscard); @@ -331,47 +302,27 @@ void gfxFontCache::NotifyHandlerEnd() { DestroyDiscard(discard); } -void gfxFontCache::DestroyFontLocked(gfxFont* aFont) { - Key key(aFont->GetFontEntry(), aFont->GetStyle(), - aFont->GetUnicodeRangeMap()); - HashEntry* entry = mFonts.GetEntry(key); - if (entry && entry->mFont == aFont) { - mFonts.RemoveEntry(entry); - } - NS_ASSERTION(aFont->GetRefCount() == 0, - "Destroying with non-zero ref count!"); - mTrackerDiscard.AppendElement(aFont); -} - -void gfxFontCache::DestroyDiscard(nsTArray& aDiscard) { - for (auto* font : aDiscard) { - NS_ASSERTION(font->GetRefCount() == 0, - "Destroying with non-zero ref count!"); +void gfxFontCache::DestroyDiscard(nsTArray>& aDiscard) { + for (auto& font : aDiscard) { + NS_ASSERTION(font->GetRefCount() == 1, + "Destroying with refs outside cache!"); font->ClearCachedWords(); - delete font; } aDiscard.Clear(); } void gfxFontCache::Flush() { - nsTArray discard; + nsTHashtable discard; { MutexAutoLock lock(mMutex); - mFonts.Clear(); - AgeAllGenerationsLocked(lock); - discard = std::move(mTrackerDiscard); + for (auto iter = mFonts.Iter(); !iter.Done(); iter.Next()) { + HashEntry* entry = static_cast(iter.Get()); + RemoveObjectLocked(entry->mFont, lock); + } + MOZ_ASSERT(IsEmptyLocked(lock), + "Cache tracker still has fonts after flush!"); + discard = std::move(mFonts); } - DestroyDiscard(discard); -} - -void gfxFontCache::AgeAllGenerations() { - nsTArray discard; - { - MutexAutoLock lock(mMutex); - AgeAllGenerationsLocked(lock); - discard = std::move(mTrackerDiscard); - } - DestroyDiscard(discard); } /*static*/ diff --git a/gfx/thebes/gfxFont.h b/gfx/thebes/gfxFont.h index 8e0190c125df..e3bef7710a43 100644 --- a/gfx/thebes/gfxFont.h +++ b/gfx/thebes/gfxFont.h @@ -256,11 +256,11 @@ struct gfxFontStyle { * Font cache design: * * The mFonts hashtable contains most fonts, indexed by (gfxFontEntry*, style). - * It does not add a reference to the fonts it contains. - * When a font's refcount decreases to zero, instead of deleting it we - * add it to our expiration tracker. - * The expiration tracker tracks fonts with zero refcount. After a certain - * period of time, such fonts expire and are deleted. + * It maintains a strong reference to the fonts it contains. + * Whenever a font is accessed, it is marked as used to move it to a new + * generation in the tracker to avoid expiration. + * The expiration tracker will only expire fonts with a single reference, the + * cache itself. Fonts with more than one reference are marked as used. * * We're using 3 generations with a ten-second generation interval, so * zero-refcount fonts will be deleted 20-30 seconds after their refcount @@ -324,13 +324,10 @@ class gfxFontCache final // We created a new font (presumably because Lookup returned null); // put it in the cache. The font's refcount should be nonzero. It is // allowable to add a new font even if there is one already in the - // cache with the same key; we'll forget about the old one. - void AddNew(gfxFont* aFont); - - // The font's refcount has gone to zero; give ownership of it to - // the cache. We delete it if it's not acquired again after a certain - // amount of time. - void NotifyReleased(gfxFont* aFont); + // cache with the same key, as we may race with other threads to do + // the insertion -- in that case we will return the original font, + // and destroy the new one. + already_AddRefed MaybeInsert(RefPtr&& aFont); // Cleans out the hashtable and removes expired fonts waiting for cleanup. // Other gfxFont objects may be still in use but they will be pushed @@ -369,8 +366,6 @@ class gfxFontCache final void AddSizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf, FontCacheSizes* aSizes) const; - void AgeAllGenerations(); - protected: class MemoryReporter final : public nsIMemoryReporter { ~MemoryReporter() = default; @@ -394,14 +389,13 @@ class gfxFontCache final return AddObjectLocked(aFont, lock); } - // This gets called when the timeout has expired on a zero-refcount + // This gets called when the timeout has expired on a single-refcount // font; we just delete it. void NotifyExpiredLocked(gfxFont* aFont, const AutoLock&) REQUIRES(mMutex) override; void NotifyHandlerEnd() override; - void DestroyFontLocked(gfxFont* aFont) REQUIRES(mMutex); - void DestroyDiscard(nsTArray& aDiscard); + void DestroyDiscard(nsTArray>& aDiscard); static gfxFontCache* gGlobalCache; @@ -425,24 +419,30 @@ class gfxFontCache final // blank. The caller of Put() will fill this in. explicit HashEntry(KeyTypePointer aStr) : mFont(nullptr) {} + HashEntry(const HashEntry&) = delete; + HashEntry& operator=(const HashEntry&) = delete; + + HashEntry(HashEntry&& aOther) noexcept : mFont(std::move(aOther.mFont)) {} + + HashEntry& operator=(HashEntry&& aOther) noexcept { + mFont = std::move(aOther.mFont); + return *this; + } + bool KeyEquals(const KeyTypePointer aKey) const; static KeyTypePointer KeyToPointer(KeyType aKey) { return &aKey; } static PLDHashNumber HashKey(const KeyTypePointer aKey) { return mozilla::HashGeneric(aKey->mStyle->Hash(), aKey->mFontEntry, aKey->mUnicodeRangeMap); } - enum { ALLOW_MEMMOVE = true }; + enum { ALLOW_MEMMOVE = false }; - // The cache tracks gfxFont objects whose refcount has dropped to zero, - // so they are not immediately deleted but may be "resurrected" if they - // have not yet expired from the tracker when they are needed again. - // See the custom AddRef/Release methods in gfxFont. - gfxFont* MOZ_UNSAFE_REF("tracking for deferred deletion") mFont; + RefPtr mFont; }; nsTHashtable mFonts GUARDED_BY(mMutex); - nsTArray mTrackerDiscard GUARDED_BY(mMutex); + nsTArray> mTrackerDiscard GUARDED_BY(mMutex); static void WordCacheExpirationTimerCallback(nsITimer* aTimer, void* aCache); @@ -1426,24 +1426,7 @@ class gfxFont { using FontSlantStyle = mozilla::FontSlantStyle; using FontSizeAdjust = mozilla::StyleFontSizeAdjust; - nsrefcnt AddRef(void) { - MOZ_ASSERT(int32_t(mRefCnt) >= 0, "illegal refcnt"); - nsrefcnt count = ++mRefCnt; - NS_LOG_ADDREF(this, count, "gfxFont", sizeof(*this)); - return count; - } - nsrefcnt Release(void) { - MOZ_ASSERT(0 != mRefCnt, "dup release"); - --mRefCnt; - NS_LOG_RELEASE(this, mRefCnt, "gfxFont"); - nsrefcnt rval = mRefCnt; - if (!rval) { - NotifyReleased(); - // |this| may have been deleted. - } - return rval; - } - + NS_INLINE_DECL_THREADSAFE_REFCOUNTING(gfxFont) int32_t GetRefCount() { return int32_t(mRefCnt); } // options to specify the kind of AA to be used when creating a font @@ -1455,27 +1438,13 @@ class gfxFont { } AntialiasOption; protected: - mozilla::ThreadSafeAutoRefCnt mRefCnt; - - void NotifyReleased() { - gfxFontCache* cache = gfxFontCache::GetCache(); - if (cache) { - // Don't delete just yet; return the object to the cache for - // possibly recycling within some time limit - cache->NotifyReleased(this); - } else { - // The cache may have already been shut down. - delete this; - } - } - gfxFont(const RefPtr& aUnscaledFont, gfxFontEntry* aFontEntry, const gfxFontStyle* aFontStyle, AntialiasOption anAAOption = kAntialiasDefault); - public: virtual ~gfxFont(); + public: bool Valid() const { return mIsValid; } // options for the kind of bounding box to return from measurement diff --git a/gfx/thebes/gfxFontEntry.cpp b/gfx/thebes/gfxFontEntry.cpp index 0e11adb6aa00..f51274bb27a2 100644 --- a/gfx/thebes/gfxFontEntry.cpp +++ b/gfx/thebes/gfxFontEntry.cpp @@ -270,21 +270,16 @@ already_AddRefed gfxFontEntry::FindOrMakeFont( const gfxFontStyle* aStyle, gfxCharacterMap* aUnicodeRangeMap) { RefPtr font = gfxFontCache::GetCache()->Lookup(this, aStyle, aUnicodeRangeMap); - - if (!font) { - gfxFont* newFont = CreateFontInstance(aStyle); - if (!newFont) { - return nullptr; - } - if (!newFont->Valid()) { - delete newFont; - return nullptr; - } - font = newFont; - font->SetUnicodeRangeMap(aUnicodeRangeMap); - gfxFontCache::GetCache()->AddNew(font); + if (font) { + return font.forget(); } - return font.forget(); + + font = CreateFontInstance(aStyle); + if (!font || !font->Valid()) { + return nullptr; + } + font->SetUnicodeRangeMap(aUnicodeRangeMap); + return gfxFontCache::GetCache()->MaybeInsert(std::move(font)); } uint16_t gfxFontEntry::UnitsPerEm() { diff --git a/gfx/thebes/gfxGDIFont.h b/gfx/thebes/gfxGDIFont.h index a731e06efa0b..a2f323b33414 100644 --- a/gfx/thebes/gfxGDIFont.h +++ b/gfx/thebes/gfxGDIFont.h @@ -15,13 +15,11 @@ #include "usp10.h" -class gfxGDIFont : public gfxFont { +class gfxGDIFont final : public gfxFont { public: gfxGDIFont(GDIFontEntry* aFontEntry, const gfxFontStyle* aFontStyle, AntialiasOption anAAOption = kAntialiasDefault); - virtual ~gfxGDIFont(); - HFONT GetHFONT() const { return mFont; } already_AddRefed GetScaledFont( @@ -59,6 +57,8 @@ class gfxGDIFont : public gfxFont { FontType GetType() const override { return FONT_TYPE_GDI; } protected: + ~gfxGDIFont() override; + const Metrics& GetHorizontalMetrics() const override { return *mMetrics; } bool ShapeText(DrawTarget* aDrawTarget, const char16_t* aText, diff --git a/gfx/thebes/gfxMacFont.h b/gfx/thebes/gfxMacFont.h index 7c5dd2a436a8..d739814afd3f 100644 --- a/gfx/thebes/gfxMacFont.h +++ b/gfx/thebes/gfxMacFont.h @@ -14,13 +14,11 @@ class MacOSFontEntry; -class gfxMacFont : public gfxFont { +class gfxMacFont final : public gfxFont { public: gfxMacFont(const RefPtr& aUnscaledFont, MacOSFontEntry* aFontEntry, const gfxFontStyle* aFontStyle); - virtual ~gfxMacFont(); - CGFontRef GetCGFontRef() const { return mCGFont; } /* override Measure to add padding for antialiasing */ @@ -59,6 +57,8 @@ class gfxMacFont : public gfxFont { CTFontDescriptorRef aFontDesc = nullptr); protected: + ~gfxMacFont() override; + const Metrics& GetHorizontalMetrics() const override { return mMetrics; } // override to prefer CoreText shaping with fonts that depend on AAT diff --git a/gfx/thebes/gfxPlatform.cpp b/gfx/thebes/gfxPlatform.cpp index 6ca0c6aba65f..bf3aac20511f 100644 --- a/gfx/thebes/gfxPlatform.cpp +++ b/gfx/thebes/gfxPlatform.cpp @@ -2230,7 +2230,7 @@ void gfxPlatform::FontsPrefsChanged(const char* aPref) { !strcmp("gfx.font_rendering.ahem_antialias_none", aPref)) { FlushFontAndWordCaches(); } else if (!strcmp(GFX_PREF_OPENTYPE_SVG, aPref)) { - gfxFontCache::GetCache()->AgeAllGenerations(); + gfxFontCache::GetCache()->Flush(); gfxFontCache::GetCache()->NotifyGlyphsChanged(); } } diff --git a/gfx/thebes/gfxPlatformFontList.cpp b/gfx/thebes/gfxPlatformFontList.cpp index bfd45c529984..d8b10c8b236b 100644 --- a/gfx/thebes/gfxPlatformFontList.cpp +++ b/gfx/thebes/gfxPlatformFontList.cpp @@ -164,7 +164,7 @@ static void FontListPrefChanged(const char* aPref, void* aData = nullptr) { // XXX this could be made to only clear out the cache for the prefs that were // changed but it probably isn't that big a deal. gfxPlatformFontList::PlatformFontList()->ClearLangGroupPrefFonts(); - gfxFontCache::GetCache()->AgeAllGenerations(); + gfxFontCache::GetCache()->Flush(); } static gfxFontListPrefObserver* gFontListPrefObserver = nullptr;