diff options
author | Seigo Nonaka <nona@google.com> | 2018-02-08 16:48:33 -0800 |
---|---|---|
committer | Seigo Nonaka <nona@google.com> | 2018-02-09 13:40:12 -0800 |
commit | 7d8c4ed5b7683e16e6205a6b52b1910f8ff6b51b (patch) | |
tree | bb501c01f38f75b86b9741012a6074a33450b20c | |
parent | 48fa54111e68986bfa675cc4005486f9d336db1d (diff) | |
download | minikin-7d8c4ed5b7683e16e6205a6b52b1910f8ff6b51b.tar.gz |
Remove global mutex lock from minikin
All modules are now free from gMinikinLock.
This increases the performance of parallel text layout in background thread.
Here is a raw performance score on walleye-userdebug
StaticLayout creation time in parallel:
RandomText Thread 1 : 7,348,118 -> 7,327,542: (-0.3%)
RandomText Thread 2 : 13,872,214 -> 7,402,398: (-46.6%)
RandomText Thread 4 : 24,999,986 -> 8,185,263: (-67.3%)
StaticLayout creation time:
MeasuredText Balanced Hyphenation : 774,245 -> 781,534: (+0.9%)
MeasuredText Balanced NoHyphenation: 587,157 -> 574,882: (-2.1%)
MeasuredText Greedy Hyphenation : 535,843 -> 523,630: (-2.3%)
MeasuredText Greedy NoHyphenation : 534,965 -> 522,259: (-2.4%)
RandomText Balanced Hyphenation : 17,886,651 -> 17,814,108: (-0.4%)
RandomText Balanced NoHyphenation : 7,385,288 -> 7,410,588: (+0.3%)
RandomText Greedy Hyphenation : 7,380,350 -> 7,296,407: (-1.1%)
RandomText Greedy NoHyphenation : 7,358,680 -> 7,320,040: (-0.5%)
MeasuredText creation time
NoStyled Hyphenation : 17,587,451 -> 17,544,350: (-0.2%)
NoStyled Hyphenation WidthOnly : 17,112,258 -> 17,092,507: (-0.1%)
NoStyled NoHyphenation : 7,307,064 -> 7,296,519: (-0.1%)
NoStyled NoHyphenation WidthOnly : 6,880,721 -> 6,848,543: (-0.5%)
Styled Hyphenation : 14,753,919 -> 14,659,049: (-0.6%)
Styled Hyphenation WidthOnly : 13,767,908 -> 13,755,623: (-0.1%)
Styled NoHyphenation : 14,368,118 -> 14,337,125: (-0.2%)
Styled NoHyphenation WidthOnly : 13,414,942 -> 13,375,858: (-0.3%)
StaticLayout draw time:
MeasuredText NoStyled : 676,438 -> 678,795: (+0.3%)
MeasuredText NoStyled WithoutCache : 663,041 -> 654,687: (-1.3%)
MeasuredText Styled : 885,574 -> 890,281: (+0.5%)
MeasuredText Styled WithoutCache : 944,259 -> 928,675: (-1.7%)
RandomText NoStyled : 562,361 -> 553,651: (-1.5%)
RandomText NoStyled WithoutCache : 6,678,968 -> 6,696,637: (+0.3%)
RandomText Styled : 2,970,369 -> 2,933,900: (-1.2%)
RandomText Styled WithoutCache : 3,408,554 -> 3,387,260: (-0.6%)
Bug: 37567215
Test: minikin_tests
Test: minikin_stress_tests
Test: atest CtsWidgetTestCases:EditTextTest
CtsWidgetTestCases:TextViewFadingEdgeTest
FrameworksCoreTests:TextViewFallbackLineSpacingTest
FrameworksCoreTests:TextViewTest FrameworksCoreTests:TypefaceTest
CtsGraphicsTestCases:TypefaceTest CtsWidgetTestCases:TextViewTest
CtsTextTestCases CtsGraphicsTestCases:PaintTest
Test: bit FrameworksCoreTests:android.text.
Change-Id: I6aab090be6bfa65f1205b26b77c6c65823a3d94e
-rw-r--r-- | include/minikin/FontCollection.h | 3 | ||||
-rw-r--r-- | libs/minikin/Android.bp | 4 | ||||
-rw-r--r-- | libs/minikin/FontCollection.cpp | 8 | ||||
-rw-r--r-- | libs/minikin/Layout.cpp | 9 | ||||
-rw-r--r-- | libs/minikin/LineBreakerUtil.h | 1 | ||||
-rw-r--r-- | libs/minikin/Locale.cpp | 1 | ||||
-rw-r--r-- | libs/minikin/LocaleListCache.h | 2 | ||||
-rw-r--r-- | libs/minikin/MinikinInternal.cpp | 8 | ||||
-rw-r--r-- | libs/minikin/MinikinInternal.h | 9 | ||||
-rw-r--r-- | tests/perftests/FontCollection.cpp | 1 | ||||
-rw-r--r-- | tests/stresstest/FontFamilyTest.cpp | 1 | ||||
-rw-r--r-- | tests/unittest/FontCollectionItemizeTest.cpp | 2 | ||||
-rw-r--r-- | tests/unittest/FontFamilyTest.cpp | 4 | ||||
-rw-r--r-- | tests/unittest/FontLanguageListCacheTest.cpp | 2 | ||||
-rw-r--r-- | tests/unittest/HyphenatorMapTest.cpp | 1 | ||||
-rw-r--r-- | tests/unittest/LineBreakerTestHelper.h | 1 | ||||
-rw-r--r-- | tests/unittest/LocaleListTest.cpp | 1 |
17 files changed, 4 insertions, 54 deletions
diff --git a/include/minikin/FontCollection.h b/include/minikin/FontCollection.h index 288ed0d..61b4231 100644 --- a/include/minikin/FontCollection.h +++ b/include/minikin/FontCollection.h @@ -91,9 +91,6 @@ private: static uint32_t calcVariantMatchingScore(FontFamily::Variant variant, const FontFamily& fontFamily); - // static for allocating unique id's - static uint32_t sNextId; - // unique id for this font collection (suitable for cache key) uint32_t mId; diff --git a/libs/minikin/Android.bp b/libs/minikin/Android.bp index 0f9d003..4dc36b7 100644 --- a/libs/minikin/Android.bp +++ b/libs/minikin/Android.bp @@ -75,8 +75,8 @@ cc_library { ], product_variables: { debuggable: { - // Enable race detection and assertion on eng and userdebug build. - cppflags: ["-DENABLE_RACE_DETECTION", "-DENABLE_ASSERTION"], + // Enable assertion on eng and userdebug build. + cppflags: ["-DENABLE_ASSERTION"], }, }, shared_libs: [ diff --git a/libs/minikin/FontCollection.cpp b/libs/minikin/FontCollection.cpp index d9b200a..b20d779 100644 --- a/libs/minikin/FontCollection.cpp +++ b/libs/minikin/FontCollection.cpp @@ -42,7 +42,7 @@ static inline T max(T a, T b) { const uint32_t EMOJI_STYLE_VS = 0xFE0F; const uint32_t TEXT_STYLE_VS = 0xFE0E; -uint32_t FontCollection::sNextId = 0; +static std::atomic<uint32_t> gNextCollectionId = {0}; FontCollection::FontCollection(std::shared_ptr<FontFamily>&& typeface) : mMaxChar(0) { std::vector<std::shared_ptr<FontFamily>> typefaces; @@ -55,8 +55,7 @@ FontCollection::FontCollection(const vector<std::shared_ptr<FontFamily>>& typefa } void FontCollection::init(const vector<std::shared_ptr<FontFamily>>& typefaces) { - android::AutoMutex _l(gMinikinLock); - mId = sNextId++; + mId = gNextCollectionId++; vector<uint32_t> lastChar; size_t nTypefaces = typefaces.size(); const FontStyle defaultStyle; @@ -350,9 +349,6 @@ bool FontCollection::hasVariationSelector(uint32_t baseCodepoint, } } - // TODO: We can remove this lock by precomputing color emoji information. - android::AutoMutex _l(gMinikinLock); - // Even if there is no cmap format 14 subtable entry for the given sequence, should return true // for <char, text presentation selector> case since we have special fallback rule for the // sequence. Note that we don't need to restrict this to already standardized variation diff --git a/libs/minikin/Layout.cpp b/libs/minikin/Layout.cpp index a641fae..824e3a1 100644 --- a/libs/minikin/Layout.cpp +++ b/libs/minikin/Layout.cpp @@ -310,8 +310,6 @@ private: // number of strings static const size_t kMaxEntries = 5000; - // To avoid dead-locking, need to acquire gMinikinLock before acquiring mMutex. - // TODO: Remove gMinikinLock std::mutex mMutex; }; @@ -626,8 +624,6 @@ BidiText::BidiText(const uint16_t* buf, size_t start, size_t count, size_t bufSi void Layout::doLayout(const U16StringPiece& textBuf, const Range& range, Bidi bidiFlags, const MinikinPaint& paint, StartHyphenEdit startHyphen, EndHyphenEdit endHyphen) { - android::AutoMutex _l(gMinikinLock); - LayoutContext ctx(paint); const uint32_t count = range.getLength(); @@ -646,7 +642,6 @@ void Layout::doLayout(const U16StringPiece& textBuf, const Range& range, Bidi bi void Layout::addToLayoutPieces(const U16StringPiece& textBuf, const Range& range, Bidi bidiFlag, const MinikinPaint& paint, LayoutPieces* out) { - android::AutoMutex _l(gMinikinLock); LayoutContext ctx(paint); float advance = 0; @@ -666,8 +661,6 @@ void Layout::addToLayoutPieces(const U16StringPiece& textBuf, const Range& range float Layout::measureText(const U16StringPiece& textBuf, const Range& range, Bidi bidiFlags, const MinikinPaint& paint, StartHyphenEdit startHyphen, EndHyphenEdit endHyphen, float* advances, MinikinExtent* extents) { - android::AutoMutex _l(gMinikinLock); - LayoutContext ctx(paint); float advance = 0; @@ -1149,12 +1142,10 @@ void Layout::getBounds(MinikinRect* bounds) const { } void Layout::purgeCaches() { - android::AutoMutex _l(gMinikinLock); LayoutCache::getInstance().clear(); } void Layout::dumpMinikinStats(int fd) { - android::AutoMutex _l(gMinikinLock); LayoutCache::getInstance().dumpStats(fd); } diff --git a/libs/minikin/LineBreakerUtil.h b/libs/minikin/LineBreakerUtil.h index 5e51138..4c86604 100644 --- a/libs/minikin/LineBreakerUtil.h +++ b/libs/minikin/LineBreakerUtil.h @@ -56,7 +56,6 @@ inline bool isLineEndSpace(uint16_t c) { } inline Locale getEffectiveLocale(uint32_t localeListId) { - android::AutoMutex _l(gMinikinLock); const LocaleList& localeList = LocaleListCache::getById(localeListId); return localeList.empty() ? Locale() : localeList[0]; } diff --git a/libs/minikin/Locale.cpp b/libs/minikin/Locale.cpp index aad8cfd..346e5e4 100644 --- a/libs/minikin/Locale.cpp +++ b/libs/minikin/Locale.cpp @@ -31,7 +31,6 @@ namespace minikin { constexpr uint32_t FIVE_BITS = 0x1f; uint32_t registerLocaleList(const std::string& locales) { - android::AutoMutex _l(gMinikinLock); return LocaleListCache::getId(locales); } diff --git a/libs/minikin/LocaleListCache.h b/libs/minikin/LocaleListCache.h index 2a007af..32b5f6d 100644 --- a/libs/minikin/LocaleListCache.h +++ b/libs/minikin/LocaleListCache.h @@ -63,8 +63,6 @@ private: // A map from the string representation of the font locale list to the ID. std::unordered_map<std::string, uint32_t> mLocaleListLookupTable; - // To avoid dead-locking, need to acquire gMinikinLock before acquiring mMutex. - // TODO: Remove gMinikinLock std::mutex mMutex; }; diff --git a/libs/minikin/MinikinInternal.cpp b/libs/minikin/MinikinInternal.cpp index 0ed667d..d02f71f 100644 --- a/libs/minikin/MinikinInternal.cpp +++ b/libs/minikin/MinikinInternal.cpp @@ -23,14 +23,6 @@ namespace minikin { -android::Mutex gMinikinLock; - -void assertMinikinLocked() { -#ifdef ENABLE_RACE_DETECTION - LOG_ALWAYS_FATAL_IF(gMinikinLock.tryLock() == 0); -#endif -} - inline static bool isBMPVariationSelector(uint32_t codePoint) { return VS1 <= codePoint && codePoint <= VS16; } diff --git a/libs/minikin/MinikinInternal.h b/libs/minikin/MinikinInternal.h index 05dcb9c..d90f099 100644 --- a/libs/minikin/MinikinInternal.h +++ b/libs/minikin/MinikinInternal.h @@ -28,15 +28,6 @@ namespace minikin { -// All external Minikin interfaces are designed to be thread-safe. -// Presently, that's implemented by through a global lock, and having -// all external interfaces take that lock. - -extern android::Mutex gMinikinLock; - -// Aborts if gMinikinLock is not acquired. Do nothing on the release build. -void assertMinikinLocked(); - #ifdef ENABLE_ASSERTION #define MINIKIN_ASSERT(cond, ...) LOG_ALWAYS_FATAL_IF(!(cond), __VA_ARGS__) #else diff --git a/tests/perftests/FontCollection.cpp b/tests/perftests/FontCollection.cpp index 24fc665..7d7af6d 100644 --- a/tests/perftests/FontCollection.cpp +++ b/tests/perftests/FontCollection.cpp @@ -93,7 +93,6 @@ static void BM_FontCollection_itemize(benchmark::State& state) { MinikinPaint paint(collection); paint.localeListId = registerLocaleList(ITEMIZE_TEST_CASES[testIndex].languageTag); - android::AutoMutex _l(gMinikinLock); while (state.KeepRunning()) { result.clear(); collection->itemize(buffer, utf16_length, paint, &result); diff --git a/tests/stresstest/FontFamilyTest.cpp b/tests/stresstest/FontFamilyTest.cpp index f7d8b8e..77371cb 100644 --- a/tests/stresstest/FontFamilyTest.cpp +++ b/tests/stresstest/FontFamilyTest.cpp @@ -36,7 +36,6 @@ TEST_P(FontFamilyHarfBuzzCompatibilityTest, CoverageTest) { std::shared_ptr<FontFamily> family = buildFontFamily(fontPath); - android::AutoMutex _l(gMinikinLock); hb_font_t* hbFont = family->getFont(0)->baseFont().get(); for (uint32_t codePoint = 0; codePoint < MAX_UNICODE_CODE_POINT; ++codePoint) { diff --git a/tests/unittest/FontCollectionItemizeTest.cpp b/tests/unittest/FontCollectionItemizeTest.cpp index c130f6d..d1ff460 100644 --- a/tests/unittest/FontCollectionItemizeTest.cpp +++ b/tests/unittest/FontCollectionItemizeTest.cpp @@ -66,7 +66,6 @@ void itemize(const std::shared_ptr<FontCollection>& collection, const char* str, MinikinPaint paint(collection); paint.fontStyle = style; paint.localeListId = localeListId; - android::AutoMutex _l(gMinikinLock); collection->itemize(buf, len, paint, result); } @@ -96,7 +95,6 @@ const std::string& getFontPath(const FontCollection::Run& run) { // Utility function to obtain LocaleList from string. const LocaleList& registerAndGetLocaleList(const std::string& locale_string) { - android::AutoMutex _l(gMinikinLock); return LocaleListCache::getById(LocaleListCache::getId(locale_string)); } diff --git a/tests/unittest/FontFamilyTest.cpp b/tests/unittest/FontFamilyTest.cpp index 402f26b..7cdc6ad 100644 --- a/tests/unittest/FontFamilyTest.cpp +++ b/tests/unittest/FontFamilyTest.cpp @@ -28,13 +28,11 @@ namespace minikin { static const LocaleList& createLocaleList(const std::string& input) { - android::AutoMutex _l(gMinikinLock); uint32_t localeListId = LocaleListCache::getId(input); return LocaleListCache::getById(localeListId); } static Locale createLocale(const std::string& input) { - android::AutoMutex _l(gMinikinLock); uint32_t localeListId = LocaleListCache::getId(input); return LocaleListCache::getById(localeListId)[0]; } @@ -627,8 +625,6 @@ TEST_F(FontFamilyTest, coverageTableSelectionTest) { std::shared_ptr<FontFamily> unicodeEnc3Font = buildFontFamily(kUnicodeEncoding3Font); std::shared_ptr<FontFamily> unicodeEnc4Font = buildFontFamily(kUnicodeEncoding4Font); - android::AutoMutex _l(gMinikinLock); - EXPECT_TRUE(unicodeEnc1Font->hasGlyph(0x0061, 0)); EXPECT_TRUE(unicodeEnc3Font->hasGlyph(0x0061, 0)); EXPECT_TRUE(unicodeEnc4Font->hasGlyph(0x0061, 0)); diff --git a/tests/unittest/FontLanguageListCacheTest.cpp b/tests/unittest/FontLanguageListCacheTest.cpp index e15cc24..e957cfc 100644 --- a/tests/unittest/FontLanguageListCacheTest.cpp +++ b/tests/unittest/FontLanguageListCacheTest.cpp @@ -30,7 +30,6 @@ TEST(LocaleListCacheTest, getId) { EXPECT_NE(0UL, registerLocaleList("jp")); EXPECT_NE(0UL, registerLocaleList("en,zh-Hans")); - android::AutoMutex _l(gMinikinLock); EXPECT_EQ(0UL, LocaleListCache::getId("")); EXPECT_EQ(LocaleListCache::getId("en"), LocaleListCache::getId("en")); @@ -44,7 +43,6 @@ TEST(LocaleListCacheTest, getId) { } TEST(LocaleListCacheTest, getById) { - android::AutoMutex _l(gMinikinLock); uint32_t enLangId = LocaleListCache::getId("en"); uint32_t jpLangId = LocaleListCache::getId("jp"); Locale english = LocaleListCache::getById(enLangId)[0]; diff --git a/tests/unittest/HyphenatorMapTest.cpp b/tests/unittest/HyphenatorMapTest.cpp index fb8d536..b793fe0 100644 --- a/tests/unittest/HyphenatorMapTest.cpp +++ b/tests/unittest/HyphenatorMapTest.cpp @@ -133,7 +133,6 @@ protected: const Locale& getLocale(const std::string& localeStr) { // In production, we reconstruct the LocaleList from the locale list ID. // So, do it here too. - android::AutoMutex _l(gMinikinLock); const uint32_t id = LocaleListCache::getId(localeStr); const LocaleList& locales = LocaleListCache::getById(id); MINIKIN_ASSERT(locales.size() == 1, "The input must be a single locale"); diff --git a/tests/unittest/LineBreakerTestHelper.h b/tests/unittest/LineBreakerTestHelper.h index 0083f43..9eb73af 100644 --- a/tests/unittest/LineBreakerTestHelper.h +++ b/tests/unittest/LineBreakerTestHelper.h @@ -45,7 +45,6 @@ class ConstantRun : public Run { public: ConstantRun(const Range& range, const std::string& lang, float width) : Run(range), mPaint(nullptr /* font collection */), mWidth(width) { - android::AutoMutex _l(gMinikinLock); mLocaleListId = LocaleListCache::getId(lang); } diff --git a/tests/unittest/LocaleListTest.cpp b/tests/unittest/LocaleListTest.cpp index 36d8048..8056185 100644 --- a/tests/unittest/LocaleListTest.cpp +++ b/tests/unittest/LocaleListTest.cpp @@ -26,7 +26,6 @@ namespace minikin { namespace { const LocaleList& getLocaleList(const std::string& localeStr) { - android::AutoMutex _l(gMinikinLock); return LocaleListCache::getById(LocaleListCache::getId(localeStr)); } |