Make some thread-unsafe functions thread-safe in Layout.cpp

This is 2nd attempt of I66d0dc4398d72a6

Nothing changes from previous CL. Just collected performance scores again.

This CL remvoes LayoutEngine singleton. The members will be updated as
follows:
- hbBuffer is removed. Always create new buffers.
- unicodeFunctions is now lazy-initialized global variable.
- layoutCache is now independent singleton.

This CL also changes getHbFontFuncs not to rely on global mutex lock.

Note that Layout is not yet ready to be free from global mutex lock
since LayoutCache still rely on it.

Here is a raw performance score:
StaticLayout creation time:
 MeasuredText Balanced Hyphenation  :    697,797 ->    701,636: (+0.6%)
 MeasuredText Balanced NoHyphenation:    520,123 ->    512,359: (-1.5%)
 MeasuredText Greedy Hyphenation    :    470,325 ->    460,290: (-2.1%)
 MeasuredText Greedy NoHyphenation  :    470,273 ->    463,154: (-1.5%)
 RandomText Balanced Hyphenation    : 17,416,368 -> 17,861,604: (+2.6%)
 RandomText Balanced NoHyphenation  :  7,225,669 ->  7,340,505: (+1.6%)
 RandomText Greedy Hyphenation      :  7,204,993 ->  7,309,539: (+1.5%)
 RandomText Greedy NoHyphenation    :  7,185,285 ->  7,262,509: (+1.1%)

MeasuredText creation time:
 NoStyled Hyphenation               : 17,284,701 -> 17,678,196: (+2.3%)
 NoStyled Hyphenation WidthOnly     : 16,912,733 -> 17,273,938: (+2.1%)
 NoStyled NoHyphenation             :  7,194,385 ->  7,281,984: (+1.2%)
 NoStyled NoHyphenation WidthOnly   :  6,762,311 ->  6,877,916: (+1.7%)
 Styled Hyphenation                 : 14,712,120 -> 14,718,770: (+0.0%)
 Styled Hyphenation WidthOnly       : 13,787,737 -> 13,836,961: (+0.4%)
 Styled NoHyphenation               : 14,152,498 -> 14,321,684: (+1.2%)
 Styled NoHyphenation WidthOnly     : 13,323,860 -> 13,474,873: (+1.1%)

StaticLayout draw time:
 MeasuredText NoStyled              :    651,421 ->    647,174: (-0.7%)
 MeasuredText NoStyled WithoutCache :    637,052 ->    636,286: (-0.1%)
 MeasuredText Styled                :    889,225 ->    861,159: (-3.2%)
 MeasuredText Styled WithoutCache   :    922,764 ->    902,421: (-2.2%)
 RandomText NoStyled                :    531,045 ->    508,908: (-4.2%)
 RandomText NoStyled WithoutCache   :  6,592,819 ->  6,698,095: (+1.6%)
 RandomText Styled                  :  2,876,176 ->  2,898,401: (+0.8%)
 RandomText Styled WithoutCache     :  3,248,887 ->  3,342,324: (+2.9%)

Bug: 37567215
Test: minikin_tests
Change-Id: If383176ab80026783440258bbbbd49a625b51121

2 files changed, 101 insertions, 102 deletions

diff --git a/include/minikin/HbUtils.h b/include/minikin/HbUtils.h
index 1cf375d..68e4ab8 100644
--- a/include/minikin/HbUtils.h
+++ b/include/minikin/HbUtils.h
@@ -33,9 +33,15 @@ struct HbFaceDeleter {
 struct HbFontDeleter {
     void operator()(hb_font_t* v) { hb_font_destroy(v); }
 };
+
+struct HbBufferDeleter {
+    void operator()(hb_buffer_t* v) { hb_buffer_destroy(v); }
+};
+
 using HbBlobUniquePtr = std::unique_ptr<hb_blob_t, HbBlobDeleter>;
 using HbFaceUniquePtr = std::unique_ptr<hb_face_t, HbFaceDeleter>;
 using HbFontUniquePtr = std::unique_ptr<hb_font_t, HbFontDeleter>;
+using HbBufferUniquePtr = std::unique_ptr<hb_buffer_t, HbBufferDeleter>;
 
 }  // namespace minikin
 
diff --git a/libs/minikin/Layout.cpp b/libs/minikin/Layout.cpp
index e4e6010..f5fe89f 100644
--- a/libs/minikin/Layout.cpp
+++ b/libs/minikin/Layout.cpp
@@ -30,7 +30,6 @@
 #include <unicode/utf16.h>
 #include <utils/JenkinsHash.h>
 #include <utils/LruCache.h>
-#include <utils/Singleton.h>
 
 #include "minikin/Emoji.h"
 #include "minikin/HbUtils.h"
@@ -49,6 +48,70 @@ struct SkiaArguments {
     FontFakery fakery;
 };
 
+static hb_position_t harfbuzzGetGlyphHorizontalAdvance(hb_font_t* /* hbFont */, void* fontData,
+                                                       hb_codepoint_t glyph, void* /* userData */) {
+    SkiaArguments* args = reinterpret_cast<SkiaArguments*>(fontData);
+    float advance = args->font->GetHorizontalAdvance(glyph, *args->paint, args->fakery);
+    return 256 * advance + 0.5;
+}
+
+static hb_bool_t harfbuzzGetGlyphHorizontalOrigin(hb_font_t* /* hbFont */, void* /* fontData */,
+                                                  hb_codepoint_t /* glyph */,
+                                                  hb_position_t* /* x */, hb_position_t* /* y */,
+                                                  void* /* userData */) {
+    // Just return true, following the way that Harfbuzz-FreeType implementation does.
+    return true;
+}
+
+// TODO: Initialize in Zygote if it helps.
+hb_unicode_funcs_t* getUnicodeFunctions() {
+    static hb_unicode_funcs_t* unicodeFunctions = nullptr;
+    static std::once_flag once;
+    std::call_once(once, [&]() {
+        unicodeFunctions = hb_unicode_funcs_create(hb_icu_get_unicode_funcs());
+        /* Disable the function used for compatibility decomposition */
+        hb_unicode_funcs_set_decompose_compatibility_func(
+                unicodeFunctions,
+                [](hb_unicode_funcs_t*, hb_codepoint_t, hb_codepoint_t*, void*) -> unsigned int {
+                    return 0;
+                },
+                nullptr, nullptr);
+    });
+    return unicodeFunctions;
+}
+
+// TODO: Initialize in Zygote if it helps.
+hb_font_funcs_t* getFontFuncs() {
+    static hb_font_funcs_t* fontFuncs = nullptr;
+    static std::once_flag once;
+    std::call_once(once, [&]() {
+        fontFuncs = hb_font_funcs_create();
+        // Override the h_advance function since we can't use HarfBuzz's implemenation. It may
+        // return the wrong value if the font uses hinting aggressively.
+        hb_font_funcs_set_glyph_h_advance_func(fontFuncs, harfbuzzGetGlyphHorizontalAdvance, 0, 0);
+        hb_font_funcs_set_glyph_h_origin_func(fontFuncs, harfbuzzGetGlyphHorizontalOrigin, 0, 0);
+        hb_font_funcs_make_immutable(fontFuncs);
+    });
+    return fontFuncs;
+}
+
+// TODO: Initialize in Zygote if it helps.
+hb_font_funcs_t* getFontFuncsForEmoji() {
+    static hb_font_funcs_t* fontFuncs = nullptr;
+    static std::once_flag once;
+    std::call_once(once, [&]() {
+        fontFuncs = hb_font_funcs_create();
+        // Don't override the h_advance function since we use HarfBuzz's implementation for emoji
+        // for performance reasons.
+        // Note that it is technically possible for a TrueType font to have outline and embedded
+        // bitmap at the same time. We ignore modified advances of hinted outline glyphs in that
+        // case.
+        hb_font_funcs_set_glyph_h_origin_func(fontFuncs, harfbuzzGetGlyphHorizontalOrigin, 0, 0);
+        hb_font_funcs_make_immutable(fontFuncs);
+    });
+    return fontFuncs;
+}
+
 }  // namespace
 
 static inline UBiDiLevel bidiToUBidiLevel(Bidi bidi) {
@@ -174,6 +237,11 @@ public:
         dprintf(fd, "  Hit ratio: %d/%d (%f)\n", mCacheHitCount, mRequestCount, ratio);
     }
 
+    static LayoutCache& getInstance() {
+        static LayoutCache cache;
+        return cache;
+    }
+
 private:
     // callback for OnEntryRemoved
     void operator()(LayoutCacheKey& key, Layout*& value) {
@@ -193,27 +261,6 @@ private:
     static const size_t kMaxEntries = 5000;
 };
 
-static unsigned int disabledDecomposeCompatibility(hb_unicode_funcs_t*, hb_codepoint_t,
-                                                   hb_codepoint_t*, void*) {
-    return 0;
-}
-
-class LayoutEngine : public ::android::Singleton<LayoutEngine> {
-public:
-    LayoutEngine() {
-        unicodeFunctions = hb_unicode_funcs_create(hb_icu_get_unicode_funcs());
-        /* Disable the function used for compatibility decomposition */
-        hb_unicode_funcs_set_decompose_compatibility_func(
-                unicodeFunctions, disabledDecomposeCompatibility, NULL, NULL);
-        hbBuffer = hb_buffer_create();
-        hb_buffer_set_unicode_funcs(hbBuffer, unicodeFunctions);
-    }
-
-    hb_buffer_t* hbBuffer;
-    hb_unicode_funcs_t* unicodeFunctions;
-    LayoutCache layoutCache;
-};
-
 bool LayoutCacheKey::operator==(const LayoutCacheKey& other) const {
     return mId == other.mId && mStart == other.mStart && mCount == other.mCount &&
            mStyle == other.mStyle && mSize == other.mSize && mScaleX == other.mScaleX &&
@@ -268,48 +315,6 @@ void Layout::reset() {
     mAdvance = 0;
 }
 
-static hb_position_t harfbuzzGetGlyphHorizontalAdvance(hb_font_t* /* hbFont */, void* fontData,
-                                                       hb_codepoint_t glyph, void* /* userData */) {
-    SkiaArguments* args = reinterpret_cast<SkiaArguments*>(fontData);
-    float advance = args->font->GetHorizontalAdvance(glyph, *args->paint, args->fakery);
-    return 256 * advance + 0.5;
-}
-
-static hb_bool_t harfbuzzGetGlyphHorizontalOrigin(hb_font_t* /* hbFont */, void* /* fontData */,
-                                                  hb_codepoint_t /* glyph */,
-                                                  hb_position_t* /* x */, hb_position_t* /* y */,
-                                                  void* /* userData */) {
-    // Just return true, following the way that Harfbuzz-FreeType
-    // implementation does.
-    return true;
-}
-
-hb_font_funcs_t* getHbFontFuncs(bool forColorBitmapFont) {
-    assertMinikinLocked();
-
-    static hb_font_funcs_t* hbFuncs = nullptr;
-    static hb_font_funcs_t* hbFuncsForColorBitmap = nullptr;
-
-    hb_font_funcs_t** funcs = forColorBitmapFont ? &hbFuncs : &hbFuncsForColorBitmap;
-    if (*funcs == nullptr) {
-        *funcs = hb_font_funcs_create();
-        if (forColorBitmapFont) {
-            // Don't override the h_advance function since we use HarfBuzz's implementation for
-            // emoji for performance reasons.
-            // Note that it is technically possible for a TrueType font to have outline and embedded
-            // bitmap at the same time. We ignore modified advances of hinted outline glyphs in that
-            // case.
-        } else {
-            // Override the h_advance function since we can't use HarfBuzz's implemenation. It may
-            // return the wrong value if the font uses hinting aggressively.
-            hb_font_funcs_set_glyph_h_advance_func(*funcs, harfbuzzGetGlyphHorizontalAdvance, 0, 0);
-        }
-        hb_font_funcs_set_glyph_h_origin_func(*funcs, harfbuzzGetGlyphHorizontalOrigin, 0, 0);
-        hb_font_funcs_make_immutable(*funcs);
-    }
-    return *funcs;
-}
-
 static bool isColorBitmapFont(const HbFontUniquePtr& font) {
     HbBlob cbdt(font, HB_TAG('C', 'B', 'D', 'T'));
     return cbdt;
@@ -346,7 +351,7 @@ int Layout::findOrPushBackFace(const FakedFont& face, LayoutContext* ctx) {
         // Create new hb_font_t from base font.
         HbFontUniquePtr font(hb_font_create_sub_font(face.font->baseFont().get()));
         hb_font_set_funcs(
-                font.get(), getHbFontFuncs(isColorBitmapFont(font)),
+                font.get(), isColorBitmapFont(font) ? getFontFuncsForEmoji() : getFontFuncs(),
                 new SkiaArguments({face.font->typeface().get(), &ctx->paint, face.fakery}),
                 [](void* data) { delete reinterpret_cast<SkiaArguments*>(data); });
         ctx->hbFonts.push_back(std::move(font));
@@ -354,14 +359,6 @@ int Layout::findOrPushBackFace(const FakedFont& face, LayoutContext* ctx) {
     return ix;
 }
 
-static hb_script_t codePointToScript(hb_codepoint_t codepoint) {
-    static hb_unicode_funcs_t* u = 0;
-    if (!u) {
-        u = LayoutEngine::getInstance().unicodeFunctions;
-    }
-    return hb_unicode_script(u, codepoint);
-}
-
 static hb_codepoint_t decodeUtf16(const uint16_t* chars, size_t len, ssize_t* iter) {
     UChar32 result;
     U16_NEXT(chars, *iter, (ssize_t)len, result);
@@ -376,12 +373,12 @@ static hb_script_t getScriptRun(const uint16_t* chars, size_t len, ssize_t* iter
         return HB_SCRIPT_UNKNOWN;
     }
     uint32_t cp = decodeUtf16(chars, len, iter);
-    hb_script_t current_script = codePointToScript(cp);
+    hb_script_t current_script = hb_unicode_script(getUnicodeFunctions(), cp);
     for (;;) {
         if (size_t(*iter) == len) break;
         const ssize_t prev_iter = *iter;
         cp = decodeUtf16(chars, len, iter);
-        const hb_script_t script = codePointToScript(cp);
+        const hb_script_t script = hb_unicode_script(getUnicodeFunctions(), cp);
         if (script != current_script) {
             if (current_script == HB_SCRIPT_INHERITED || current_script == HB_SCRIPT_COMMON) {
                 current_script = script;
@@ -682,7 +679,7 @@ float Layout::doLayoutWord(const uint16_t* buf, size_t start, size_t count, size
                            bool isRtl, LayoutContext* ctx, size_t bufStart,
                            StartHyphenEdit startHyphen, EndHyphenEdit endHyphen, Layout* layout,
                            float* advances, MinikinExtent* extents, LayoutPieces* lpOut) {
-    LayoutCache& cache = LayoutEngine::getInstance().layoutCache;
+    LayoutCache& cache = LayoutCache::getInstance();
     LayoutCacheKey key(ctx->paint, buf, start, count, bufSize, isRtl, startHyphen, endHyphen);
 
     float wordSpacing = count == 1 && isWordSpace(buf[start]) ? ctx->paint.wordSpacing : 0;
@@ -769,22 +766,23 @@ static inline hb_codepoint_t determineHyphenChar(hb_codepoint_t preferredHyphen,
 }
 
 template <typename HyphenEdit>
-static inline void addHyphenToHbBuffer(hb_buffer_t* buffer, const HbFontUniquePtr& font,
+static inline void addHyphenToHbBuffer(const HbBufferUniquePtr& buffer, const HbFontUniquePtr& font,
                                        HyphenEdit hyphen, uint32_t cluster) {
     const uint32_t* chars;
     size_t size;
     std::tie(chars, size) = getHyphenString(hyphen);
     for (size_t i = 0; i < size; i++) {
-        hb_buffer_add(buffer, determineHyphenChar(chars[i], font.get()), cluster);
+        hb_buffer_add(buffer.get(), determineHyphenChar(chars[i], font.get()), cluster);
     }
 }
 
 // Returns the cluster value assigned to the first codepoint added to the buffer, which can be used
 // to translate cluster values returned by HarfBuzz to input indices.
-static inline uint32_t addToHbBuffer(hb_buffer_t* buffer, const uint16_t* buf, size_t start,
-                                     size_t count, size_t bufSize, ssize_t scriptRunStart,
-                                     ssize_t scriptRunEnd, StartHyphenEdit inStartHyphen,
-                                     EndHyphenEdit inEndHyphen, const HbFontUniquePtr& hbFont) {
+static inline uint32_t addToHbBuffer(const HbBufferUniquePtr& buffer, const uint16_t* buf,
+                                     size_t start, size_t count, size_t bufSize,
+                                     ssize_t scriptRunStart, ssize_t scriptRunEnd,
+                                     StartHyphenEdit inStartHyphen, EndHyphenEdit inEndHyphen,
+                                     const HbFontUniquePtr& hbFont) {
     // Only hyphenate the very first script run for starting hyphens.
     const StartHyphenEdit startHyphen =
             (scriptRunStart == 0) ? inStartHyphen : StartHyphenEdit::NO_EDIT;
@@ -845,10 +843,10 @@ static inline uint32_t addToHbBuffer(hb_buffer_t* buffer, const uint16_t* buf, s
         hbTextLength = hbItemOffset + hbItemLength;
     }
 
-    hb_buffer_add_utf16(buffer, hbText, hbTextLength, hbItemOffset, hbItemLength);
+    hb_buffer_add_utf16(buffer.get(), hbText, hbTextLength, hbItemOffset, hbItemLength);
 
     unsigned int numCodepoints;
-    hb_glyph_info_t* cpInfo = hb_buffer_get_glyph_infos(buffer, &numCodepoints);
+    hb_glyph_info_t* cpInfo = hb_buffer_get_glyph_infos(buffer.get(), &numCodepoints);
 
     // Add the hyphen at the end, if there's any.
     if (hasEndInsertion || hasEndReplacement) {
@@ -872,7 +870,7 @@ static inline uint32_t addToHbBuffer(hb_buffer_t* buffer, const uint16_t* buf, s
         addHyphenToHbBuffer(buffer, hbFont, endHyphen, hyphenCluster);
         // Since we have just added to the buffer, cpInfo no longer necessarily points to
         // the right place. Refresh it.
-        cpInfo = hb_buffer_get_glyph_infos(buffer, nullptr /* we don't need the size */);
+        cpInfo = hb_buffer_get_glyph_infos(buffer.get(), nullptr /* we don't need the size */);
     }
     return cpInfo[0].cluster;
 }
@@ -880,7 +878,8 @@ static inline uint32_t addToHbBuffer(hb_buffer_t* buffer, const uint16_t* buf, s
 void Layout::doLayoutRun(const uint16_t* buf, size_t start, size_t count, size_t bufSize,
                          bool isRtl, LayoutContext* ctx, StartHyphenEdit startHyphen,
                          EndHyphenEdit endHyphen) {
-    hb_buffer_t* buffer = LayoutEngine::getInstance().hbBuffer;
+    HbBufferUniquePtr buffer(hb_buffer_create());
+    hb_buffer_set_unicode_funcs(buffer.get(), getUnicodeFunctions());
     std::vector<FontCollection::Run> items;
     ctx->paint.font->itemize(buf + start, count, ctx->paint, &items);
 
@@ -953,9 +952,9 @@ void Layout::doLayoutRun(const uint16_t* buf, size_t start, size_t count, size_t
                 letterSpaceHalfRight = letterSpace - letterSpaceHalfLeft;
             }
 
-            hb_buffer_clear_contents(buffer);
-            hb_buffer_set_script(buffer, script);
-            hb_buffer_set_direction(buffer, isRtl ? HB_DIRECTION_RTL : HB_DIRECTION_LTR);
+            hb_buffer_clear_contents(buffer.get());
+            hb_buffer_set_script(buffer.get(), script);
+            hb_buffer_set_direction(buffer.get(), isRtl ? HB_DIRECTION_RTL : HB_DIRECTION_LTR);
             const LocaleList& localeList = LocaleListCache::getById(ctx->paint.localeListId);
             if (localeList.size() != 0) {
                 hb_language_t hbLanguage = localeList.getHbLanguage(0);
@@ -965,17 +964,18 @@ void Layout::doLayoutRun(const uint16_t* buf, size_t start, size_t count, size_t
                         break;
                     }
                 }
-                hb_buffer_set_language(buffer, hbLanguage);
+                hb_buffer_set_language(buffer.get(), hbLanguage);
             }
 
             const uint32_t clusterStart =
                     addToHbBuffer(buffer, buf, start, count, bufSize, scriptRunStart, scriptRunEnd,
                                   startHyphen, endHyphen, hbFont);
 
-            hb_shape(hbFont.get(), buffer, features.empty() ? NULL : &features[0], features.size());
+            hb_shape(hbFont.get(), buffer.get(), features.empty() ? NULL : &features[0],
+                     features.size());
             unsigned int numGlyphs;
-            hb_glyph_info_t* info = hb_buffer_get_glyph_infos(buffer, &numGlyphs);
-            hb_glyph_position_t* positions = hb_buffer_get_glyph_positions(buffer, NULL);
+            hb_glyph_info_t* info = hb_buffer_get_glyph_infos(buffer.get(), &numGlyphs);
+            hb_glyph_position_t* positions = hb_buffer_get_glyph_positions(buffer.get(), NULL);
 
             // At this point in the code, the cluster values in the info buffer correspond to the
             // input characters with some shift. The cluster value clusterStart corresponds to the
@@ -1130,19 +1130,12 @@ void Layout::getBounds(MinikinRect* bounds) const {
 
 void Layout::purgeCaches() {
     android::AutoMutex _l(gMinikinLock);
-    LayoutCache& layoutCache = LayoutEngine::getInstance().layoutCache;
-    layoutCache.clear();
+    LayoutCache::getInstance().clear();
 }
 
 void Layout::dumpMinikinStats(int fd) {
     android::AutoMutex _l(gMinikinLock);
-    LayoutEngine::getInstance().layoutCache.dumpStats(fd);
+    LayoutCache::getInstance().dumpStats(fd);
 }
 
 }  // namespace minikin
-
-// Unable to define the static data member outside of android.
-// TODO: introduce our own Singleton to drop android namespace.
-namespace android {
-ANDROID_SINGLETON_STATIC_INSTANCE(minikin::LayoutEngine);
-}  // namespace android