From cf33b1b1cff343288c592e42f3154db780b02c4e Mon Sep 17 00:00:00 2001 From: Khushal Date: Wed, 29 Aug 2018 16:16:25 -0700 Subject: [PATCH] fonts: Cap the max number of entries in server side glyph cache tracking The SkStrikeServer maintains a map of SkGlyphCacheState to track the cache for a strike on the client. This entry is evicted only on a failure to lock, which means that the map can potentially grow unbounded if entries are not reused after eviction. Make sure we check that they are deleted after some limit. R=herb@google.com Bug:878966 Change-Id: I4adb06c35661049328f6e0bde52fb1c774d0c29b Reviewed-on: https://skia-review.googlesource.com/150443 Reviewed-by: Herb Derby Commit-Queue: Khusal Sagar --- src/core/SkRemoteGlyphCache.cpp | 14 ++++++++++ src/core/SkRemoteGlyphCache.h | 17 +++++++++--- tests/SkRemoteGlyphCacheTest.cpp | 45 ++++++++++++++++++++++++++++++++ 3 files changed, 73 insertions(+), 3 deletions(-) diff --git a/src/core/SkRemoteGlyphCache.cpp b/src/core/SkRemoteGlyphCache.cpp index 4fe2f428c4..4a8d917be9 100644 --- a/src/core/SkRemoteGlyphCache.cpp +++ b/src/core/SkRemoteGlyphCache.cpp @@ -515,9 +515,23 @@ SkStrikeServer::SkGlyphCacheState* SkStrikeServer::getOrCreateCache( fLockedDescs.insert(keyDescPtr); fRemoteGlyphStateMap[keyDescPtr] = std::move(cacheState); cacheStatePtr->ensureScalerContext(paint, props, matrix, flags, effects); + + checkForDeletedEntries(); return cacheStatePtr; } +void SkStrikeServer::checkForDeletedEntries() { + auto it = fRemoteGlyphStateMap.begin(); + while (fRemoteGlyphStateMap.size() > fMaxEntriesInDescriptorMap && + it != fRemoteGlyphStateMap.end()) { + if (fDiscardableHandleManager->isHandleDeleted(it->second->discardableHandleId())) { + it = fRemoteGlyphStateMap.erase(it); + } else { + ++it; + } + } +} + // -- SkGlyphCacheState ---------------------------------------------------------------------------- SkStrikeServer::SkGlyphCacheState::SkGlyphCacheState( std::unique_ptr keyDescriptor, uint32_t discardableHandleId) diff --git a/src/core/SkRemoteGlyphCache.h b/src/core/SkRemoteGlyphCache.h index d9abb7ca78..2faf64da39 100644 --- a/src/core/SkRemoteGlyphCache.h +++ b/src/core/SkRemoteGlyphCache.h @@ -105,9 +105,10 @@ public: // allowed. virtual bool lockHandle(SkDiscardableHandleId) = 0; - // TODO(khushalsagar): Add an API which checks whether a handle is still - // valid without locking, so we can avoid tracking stale handles once they - // have been purged on the remote side. + // Returns true if a handle has been deleted on the remote client. It is + // invalid to use a handle id again with this manager once this returns true. + // TODO(khushalsagar): Make pure virtual once chrome implementation lands. + virtual bool isHandleDeleted(SkDiscardableHandleId) { return false; } }; SkStrikeServer(DiscardableHandleManager* discardableHandleManager); @@ -128,10 +129,20 @@ public: SkScalerContextFlags flags, SkScalerContextEffects* effects); + void setMaxEntriesInDescriptorMapForTesting(size_t count) { + fMaxEntriesInDescriptorMap = count; + } + size_t remoteGlyphStateMapSizeForTesting() const { return fRemoteGlyphStateMap.size(); } + private: + static constexpr size_t kMaxEntriesInDescriptorMap = 2000u; + + void checkForDeletedEntries(); + SkDescriptorMap> fRemoteGlyphStateMap; DiscardableHandleManager* const fDiscardableHandleManager; SkTHashSet fCachedTypefaces; + size_t fMaxEntriesInDescriptorMap = kMaxEntriesInDescriptorMap; // State cached until the next serialization. SkDescriptorSet fLockedDescs; diff --git a/tests/SkRemoteGlyphCacheTest.cpp b/tests/SkRemoteGlyphCacheTest.cpp index 24ede91c07..1493f2ba63 100644 --- a/tests/SkRemoteGlyphCacheTest.cpp +++ b/tests/SkRemoteGlyphCacheTest.cpp @@ -41,6 +41,7 @@ public: // Client implementation. bool deleteHandle(SkDiscardableHandleId id) override { return id <= fLastDeletedHandleId; } void notifyCacheMiss(SkStrikeClient::CacheMissType type) override { fCacheMissCount[type]++; } + bool isHandleDeleted(SkDiscardableHandleId id) override { return id <= fLastDeletedHandleId; } void unlockAll() { fLockedHandles.reset(); } void unlockAndDeleteAll() { @@ -342,6 +343,50 @@ DEF_TEST(SkRemoteGlyphCache_ClientMemoryAccounting, reporter) { discardableManager->unlockAndDeleteAll(); } +DEF_TEST(SkRemoteGlyphCache_PurgesServerEntries, reporter) { + sk_sp discardableManager = sk_make_sp(); + SkStrikeServer server(discardableManager.get()); + server.setMaxEntriesInDescriptorMapForTesting(1u); + SkStrikeClient client(discardableManager, false); + + { + auto serverTf = SkTypeface::MakeFromName("monospace", SkFontStyle()); + int glyphCount = 10; + auto serverBlob = buildTextBlob(serverTf, glyphCount); + + const SkSurfaceProps props(SkSurfaceProps::kLegacyFontHost_InitType); + SkTextBlobCacheDiffCanvas cache_diff_canvas(10, 10, SkMatrix::I(), props, &server); + SkPaint paint; + REPORTER_ASSERT(reporter, server.remoteGlyphStateMapSizeForTesting() == 0u); + cache_diff_canvas.drawTextBlob(serverBlob.get(), 0, 0, paint); + REPORTER_ASSERT(reporter, server.remoteGlyphStateMapSizeForTesting() == 1u); + } + + // Serialize to release the lock from the strike server and delete all current + // handles. + std::vector fontData; + server.writeStrikeData(&fontData); + discardableManager->unlockAndDeleteAll(); + + // Use a different typeface. Creating a new strike should evict the previous + // one. + { + auto serverTf = SkTypeface::MakeFromName("Georgia", SkFontStyle()); + int glyphCount = 10; + auto serverBlob = buildTextBlob(serverTf, glyphCount); + + const SkSurfaceProps props(SkSurfaceProps::kLegacyFontHost_InitType); + SkTextBlobCacheDiffCanvas cache_diff_canvas(10, 10, SkMatrix::I(), props, &server); + SkPaint paint; + REPORTER_ASSERT(reporter, server.remoteGlyphStateMapSizeForTesting() == 1u); + cache_diff_canvas.drawTextBlob(serverBlob.get(), 0, 0, paint); + REPORTER_ASSERT(reporter, server.remoteGlyphStateMapSizeForTesting() == 1u); + } + + // Must unlock everything on termination, otherwise valgrind complains about memory leaks. + discardableManager->unlockAndDeleteAll(); +} + DEF_GPUTEST_FOR_RENDERING_CONTEXTS(SkRemoteGlyphCache_DrawTextAsPath, reporter, ctxInfo) { sk_sp discardableManager = sk_make_sp(); SkStrikeServer server(discardableManager.get());