Add request cache to SkFontHost_fontconfig.

src/ports/SkFontHost_fontconfig.cpp

Index: src/ports/SkFontHost_fontconfig.cpp
diff --git a/src/ports/SkFontHost_fontconfig.cpp b/src/ports/SkFontHost_fontconfig.cpp
index 5c9e89661790be334309527212e2478aef5d92c9..7056438610a5b6d08ca82354428fb1dce84ab637 100644
--- a/src/ports/SkFontHost_fontconfig.cpp
+++ b/src/ports/SkFontHost_fontconfig.cpp
@@ -56,36 +56,81 @@ SkFontConfigInterface* SkFontHost_fontconfig_ref_global() {
 
 ///////////////////////////////////////////////////////////////////////////////
 
-struct NameStyle {
-    NameStyle(const char* name, const SkFontStyle& style)
-        : fFamilyName(name)  // don't need to make a deep copy
-        , fStyle(style) {}
-
-    const char* fFamilyName;
-    SkFontStyle fStyle;
-};
-
-static bool find_by_NameStyle(SkTypeface* cachedTypeface,
-                              const SkFontStyle& cachedStyle,
-                              void* ctx)
-{
-    FontConfigTypeface* cachedFCTypeface = static_cast<FontConfigTypeface*>(cachedTypeface);
-    const NameStyle* nameStyle = static_cast<const NameStyle*>(ctx);
-
-    return nameStyle->fStyle == cachedStyle &&
-           cachedFCTypeface->isFamilyName(nameStyle->fFamilyName);
-}
-
 static bool find_by_FontIdentity(SkTypeface* cachedTypeface, const SkFontStyle&, void* ctx) {
     typedef SkFontConfigInterface::FontIdentity FontIdentity;
     FontConfigTypeface* cachedFCTypeface = static_cast<FontConfigTypeface*>(cachedTypeface);
-    FontIdentity* indentity = static_cast<FontIdentity*>(ctx);
+    FontIdentity* identity = static_cast<FontIdentity*>(ctx);
 
-    return cachedFCTypeface->getIdentity() == *indentity;
+    return cachedFCTypeface->getIdentity() == *identity;
 }
 
-SkTypeface* FontConfigTypeface::LegacyCreateTypeface(const char familyName[],
-                                                     SkTypeface::Style style)
+SK_DECLARE_STATIC_MUTEX(gSkFontHostRequestCacheMutex);
+class SkFontHostRequestCache {
+public:
+    void add(SkTypeface* face, const char requestedFamilyName[], SkFontStyle requestedStyle) {
+        if (fCachedResults.count() < maxEntries) {
+            fCachedResults.emplace_back(face, Request(requestedFamilyName, requestedStyle));

[mtklein, 2016/02/10 16:14:10]

I found myself wondering whether or not these typefaces were refcounted when
reading through here, up until I read the Result struct.

It might be clearer that we're reffing the typefaces if we move the ref from the
constructor to each of these emplace_back calls explicitly:
    fCachedResults.emplace_back(SkSafeRef(face), Request(...));

[bungeman-skia, 2016/02/11 17:46:02]

Hmmm... so the reason things are written the way they are here is that normally
passing a bare pointer to a SkRefCnt means no transfer of the reference count.
(On the convention that if the callee needs to keep a reference it will
increment the reference count itself.) I also find it easier to reason about
that Result does all the refing and unrefing itself. If the Request and Result
types were defined first, does that make things a bit clearer?

There are a few odd places where we document passing the bare pointer does
transfer the reference count, but I would prefer that those eventually get
updated to some sort of move based system so that the types indicate this is
happening rather than a comment.

+        } else {
+            fCachedResults[fLRUIndex] = Result(face, Request(requestedFamilyName, requestedStyle));
+            fLRUIndex++;

[mtklein, 2016/02/10 16:14:10]

fLRUIndex seems like the wrong name here.  There's no LRU going on... we're not
tracking when things are used, only when they're added.  fRingNext, or just
fNext?

[bungeman-skia, 2016/02/11 17:46:02]

Yeah, it's actually the fLRAIndex (least recently added). The issue with just
fNextIndex is that it isn't the next index in the 'warm up'. It really is the
oldest added entry.

+            if (fLRUIndex == maxEntries) {
+                fLRUIndex = 0;
+            }
+        }
+    }
+    SkTypeface* findAndRef(const char requestedFamilyName[], SkFontStyle requestedStyle) const {
+        for (const auto& cachedResult : fCachedResults) {
+            if (cachedResult.fRequest.fStyle == requestedStyle &&
+                cachedResult.fRequest.fFamilyName.equals(requestedFamilyName))
+            {
+                return SkSafeRef(cachedResult.fFace.get());
+            }
+        }
+        return nullptr;
+    }
+
+    static void Add(SkTypeface* face, const char requestedFamilyName[], SkFontStyle requestedStyle){
+        SkAutoMutexAcquire ama(gSkFontHostRequestCacheMutex);
+        Get().add(face, requestedFamilyName, requestedStyle);
+    }
+
+    static SkTypeface* FindAndRef(const char requestedFamilyName[], SkFontStyle requestedStyle) {
+        SkAutoMutexAcquire ama(gSkFontHostRequestCacheMutex);
+        return Get().findAndRef(requestedFamilyName, requestedStyle);
+    }
+
+private:
+    static SkFontHostRequestCache& Get() {

[mtklein, 2016/02/10 16:14:10]

add 
    gSKFontHostRequestCacheMutex.assertHeld(); 

?  Gotta worry about racing on the static constructor, right?

[bungeman-skia, 2016/02/11 17:46:02]

Done.

+        static SkFontHostRequestCache gCache;
+        return gCache;
+    }
+
+    // Set the maxEntries to a small value in debug to ensure testing of wrap around.

[mtklein, 2016/02/10 16:14:10]

// 64 for Release builds is... (arbitrary?)  (pulled from a histogram of web
content?) (a nice round number?)

[bungeman-skia, 2016/02/11 17:46:02]

A totally magic number.

+    static const int maxEntries = 64 SkDEBUGCODE( - 62);
+
+    struct Request {
+        Request(const char* name, const SkFontStyle& style) : fFamilyName(name), fStyle(style) {}
+        Request(Request&&) = default;
+        Request& operator=(Request&&) = default;
+        SkString fFamilyName;
+        SkFontStyle fStyle;
+    };
+    struct Result {
+        Result(SkTypeface* typeface, Request&& request)

[mtklein, 2016/02/10 16:14:10]

I'd slightly prefer passing (Request&&, SkTypeface*) to make it read more like a
key->value mapping.

[bungeman-skia, 2016/02/11 17:46:02]

Done.

+            : fFace(SkSafeRef(typeface))
+            , fRequest(std::move(request)) {}
+        Result(Result&&) = default;
+        Result& operator=(Result&&) = default;
+        SkAutoTUnref<SkTypeface> fFace;
+        Request fRequest;
+    };
+
+    SkTArray<Result> fCachedResults;
+    int fLRUIndex = 0;
+};
+
+SkTypeface* FontConfigTypeface::LegacyCreateTypeface(const char requestedFamilyName[],
+                                                     SkTypeface::Style requestedOldStyle)
 {
     SkAutoTUnref<SkFontConfigInterface> fci(RefFCI());
     if (nullptr == fci.get()) {
@@ -93,35 +138,31 @@ SkTypeface* FontConfigTypeface::LegacyCreateTypeface(const char familyName[],
     }
 
     // Check if requested NameStyle is in the NameStyle cache.

[mtklein, 2016/02/10 16:14:10]

Needs an update / deletion?

[bungeman-skia, 2016/02/11 17:46:02]

Done.

-    SkFontStyle requestedStyle(style);
-    NameStyle nameStyle(familyName, requestedStyle);
-    SkTypeface* face = SkTypefaceCache::FindByProcAndRef(find_by_NameStyle, &nameStyle);

[mtklein, 2016/02/10 16:14:10]

Is this new request-caching approach something we should spread over to
SkFontHost_mac too?  I see it's the only user of find_by_NameStyle now.

[bungeman-skia, 2016/02/11 17:46:02]

Yeah, SkFontHost_mac has its own find_by_NameStyle which is somewhat suspicious.
I would prefer not to do any of this request caching, or if we do to provide it
as a separable wrapper (a SkFontMgr_request_caching). This is a somewhat special
case just for Chromium/Blink. It's been a long term goal to fully separate the
request cache from the typeface cache, and this is a nice step in that
direction.

+    SkFontStyle requestedStyle(requestedOldStyle);
+    SkTypeface* face = SkFontHostRequestCache::FindAndRef(requestedFamilyName, requestedStyle);
     if (face) {
-        //SkDebugf("found cached face <%s> <%s> %p [%d]\n",
-        //         familyName, ((FontConfigTypeface*)face)->getFamilyName(),
-        //         face, face->getRefCnt());
         return face;
     }
 
-    SkFontConfigInterface::FontIdentity indentity;
+    SkFontConfigInterface::FontIdentity identity;
     SkString outFamilyName;
-    SkTypeface::Style outStyle;
-    if (!fci->matchFamilyName(familyName, style, &indentity, &outFamilyName, &outStyle)) {
+    SkTypeface::Style outOldStyle;
+    if (!fci->matchFamilyName(requestedFamilyName, requestedOldStyle,
+                              &identity, &outFamilyName, &outOldStyle))
+    {
         return nullptr;
     }
 
     // Check if a typeface with this FontIdentity is already in the FontIdentity cache.
-    face = SkTypefaceCache::FindByProcAndRef(find_by_FontIdentity, &indentity);
+    face = SkTypefaceCache::FindByProcAndRef(find_by_FontIdentity, &identity);
     if (!face) {
-        face = FontConfigTypeface::Create(SkFontStyle(outStyle), indentity, outFamilyName);
+        face = FontConfigTypeface::Create(SkFontStyle(outOldStyle), identity, outFamilyName);
         // Add this FontIdentity to the FontIdentity cache.
-        SkTypefaceCache::Add(face, requestedStyle);
+        SkTypefaceCache::Add(face, SkFontStyle(outOldStyle));
     }
-    // TODO: Ensure requested NameStyle and resolved NameStyle are both in the NameStyle cache.
+    // Add this NameStyle to the NameStyle cache.

[mtklein, 2016/02/10 16:14:10]

Update?  Seems weird when there's no NameStyle object here anymore.

[bungeman-skia, 2016/02/11 17:46:02]

Done.

+    SkFontHostRequestCache::Add(face, requestedFamilyName, requestedStyle);
 
-    //SkDebugf("add face <%s> <%s> %p [%d]\n",
-    //         familyName, outFamilyName.c_str(),
-    //         face, face->getRefCnt());
     return face;
 }