From b2e71274fe1f5a66c9c4befd2e86b6cf39c783b5 Mon Sep 17 00:00:00 2001 From: Khushal Date: Tue, 15 May 2018 12:59:48 -0700 Subject: [PATCH] fonts: Fix memory accounting for deserialized glyphs. When deserializing glyphs in the SkRemoteGlyphCache, we allocate from the arena for the SkGlyphCache but don't account for it in the total memory used by the cache. Fix that and avoid exposing the SkArenaAlloc from SkGlyphCache, since that can result in such brittle use. R=herb@google.com Bug: 829622 Change-Id: Iecff9ce6e0ed2c641957535363edec3e3fad178d Reviewed-on: https://skia-review.googlesource.com/128112 Reviewed-by: Herb Derby Commit-Queue: Khusal Sagar --- src/core/SkGlyphCache.cpp | 42 +++++++++++++++++++++++++------- src/core/SkGlyphCache.h | 10 ++++---- src/core/SkRemoteGlyphCache.cpp | 3 +-- src/core/SkStrikeCache.cpp | 8 ++++++ src/core/SkStrikeCache.h | 2 +- tests/SkRemoteGlyphCacheTest.cpp | 26 ++++++++++++++++++++ 6 files changed, 74 insertions(+), 17 deletions(-) diff --git a/src/core/SkGlyphCache.cpp b/src/core/SkGlyphCache.cpp index 6930b37e6a..d9d8d41eb1 100644 --- a/src/core/SkGlyphCache.cpp +++ b/src/core/SkGlyphCache.cpp @@ -15,6 +15,12 @@ #include "SkTypeface.h" #include +namespace { +size_t compute_path_size(const SkPath& path) { + return sizeof(SkPath) + path.countPoints() * sizeof(SkPoint); +} +} // namespace + SkGlyphCache::SkGlyphCache( const SkDescriptor& desc, std::unique_ptr scaler, @@ -188,6 +194,18 @@ const void* SkGlyphCache::findImage(const SkGlyph& glyph) { return glyph.fImage; } +void SkGlyphCache::initializeImage(const volatile void* data, size_t size, SkGlyph* glyph) { + if (glyph->fWidth > 0 && glyph->fWidth < kMaxGlyphWidth) { + size_t allocSize = glyph->allocImage(&fAlloc); + // check that alloc() actually succeeded + if (glyph->fImage) { + SkAssertResult(size == allocSize); + memcpy(glyph->fImage, const_cast(data), size); + fMemoryUsed += size; + } + } +} + const SkPath* SkGlyphCache::findPath(const SkGlyph& glyph) { if (glyph.fWidth) { if (glyph.fPathData == nullptr) { @@ -197,7 +215,7 @@ const SkPath* SkGlyphCache::findPath(const SkGlyph& glyph) { SkPath* path = new SkPath; if (fScalerContext->getPath(glyph.getPackedID(), path)) { pathData->fPath = path; - fMemoryUsed += sizeof(SkPath) + path->countPoints() * sizeof(SkPoint); + fMemoryUsed += compute_path_size(*path); } else { pathData->fPath = nullptr; delete path; @@ -392,17 +410,23 @@ void SkGlyphCache::dump() const { } #ifdef SK_DEBUG +void SkGlyphCache::forceValidate() const { + size_t memoryUsed = sizeof(*this); + fGlyphMap.foreach ([&memoryUsed](const SkGlyph& glyph) { + memoryUsed += sizeof(SkGlyph); + if (glyph.fImage) { + memoryUsed += glyph.computeImageSize(); + } + if (glyph.fPathData && glyph.fPathData->fPath) { + memoryUsed += compute_path_size(*glyph.fPathData->fPath); + } + }); + SkASSERT(fMemoryUsed == memoryUsed); +} void SkGlyphCache::validate() const { #ifdef SK_DEBUG_GLYPH_CACHE - int count = fGlyphArray.count(); - for (int i = 0; i < count; i++) { - const SkGlyph* glyph = &fGlyphArray[i]; - SkASSERT(glyph); - if (glyph->fImage) { - SkASSERT(fGlyphAlloc.contains(glyph->fImage)); - } - } + forceValidate(); #endif } diff --git a/src/core/SkGlyphCache.h b/src/core/SkGlyphCache.h index b9234df74f..e069efd905 100644 --- a/src/core/SkGlyphCache.h +++ b/src/core/SkGlyphCache.h @@ -44,11 +44,6 @@ public: /** Return a glyph that has no information if it is not already filled out. */ SkGlyph* getRawGlyphByID(SkPackedGlyphID); - /** Return the Strike's SkArenaAlloc. */ - SkArenaAlloc* getAlloc() { - return &fAlloc; - } - /** Returns a glyph with valid fAdvance and fDevKern fields. The remaining fields may be valid, but that is not guaranteed. If you require those, call getUnicharMetrics or getGlyphIDMetrics instead. @@ -93,6 +88,10 @@ public: */ const void* findImage(const SkGlyph&); + /** Initializes the image associated with the glyph with |data|. + */ + void initializeImage(const volatile void* data, size_t size, SkGlyph*); + /** If the advance axis intersects the glyph's path, append the positions scaled and offset to the array (if non-null), and set the count to the updated array length. */ @@ -126,6 +125,7 @@ public: SkScalerContext* getScalerContext() const { return fScalerContext.get(); } #ifdef SK_DEBUG + void forceValidate() const; void validate() const; #else void validate() const {} diff --git a/src/core/SkRemoteGlyphCache.cpp b/src/core/SkRemoteGlyphCache.cpp index 0643aff2b3..5e21f8f292 100644 --- a/src/core/SkRemoteGlyphCache.cpp +++ b/src/core/SkRemoteGlyphCache.cpp @@ -668,8 +668,7 @@ bool SkStrikeClient::readStrikeData(const volatile void* memory, size_t memorySi if (imageSize != 0) { image = deserializer.readArray(imageSize); if (!image.data()) READ_FAILURE - allocatedGlyph->allocImage(strike->getAlloc()); - memcpy(allocatedGlyph->fImage, image.data(), image.size()); + strike->initializeImage(image.data(), image.size(), allocatedGlyph); } } } diff --git a/src/core/SkStrikeCache.cpp b/src/core/SkStrikeCache.cpp index 04a1a80030..2e5bd89b79 100644 --- a/src/core/SkStrikeCache.cpp +++ b/src/core/SkStrikeCache.cpp @@ -155,6 +155,14 @@ void SkStrikeCache::PurgeAll() { get_globals().purgeAll(); } +void SkStrikeCache::Validate() { +#ifdef SK_DEBUG + auto visitor = [](const SkGlyphCache& cache) { cache.forceValidate(); }; + + get_globals().forEachStrike(visitor); +#endif +} + void SkStrikeCache::Dump() { SkDebugf("GlyphCache [ used budget ]\n"); SkDebugf(" bytes [ %8zu %8zu ]\n", diff --git a/src/core/SkStrikeCache.h b/src/core/SkStrikeCache.h index df048e33be..f2c9820799 100644 --- a/src/core/SkStrikeCache.h +++ b/src/core/SkStrikeCache.h @@ -92,7 +92,7 @@ public: const SkDescriptor&, const SkScalerContextEffects&, const SkTypeface&); static void PurgeAll(); - + static void Validate(); static void Dump(); // Dump memory usage statistics of all the attaches caches in the process using the diff --git a/tests/SkRemoteGlyphCacheTest.cpp b/tests/SkRemoteGlyphCacheTest.cpp index e653b7d2c2..dbe36d1e6a 100644 --- a/tests/SkRemoteGlyphCacheTest.cpp +++ b/tests/SkRemoteGlyphCacheTest.cpp @@ -220,3 +220,29 @@ DEF_TEST(SkRemoteGlyphCache_StrikePinningClient, reporter) { SkGraphics::PurgeFontCache(); REPORTER_ASSERT(reporter, clientTf->unique()); } + +DEF_TEST(SkRemoteGlyphCache_ClientMemoryAccounting, reporter) { + sk_sp discardableManager = sk_make_sp(); + SkStrikeServer server(discardableManager.get()); + SkStrikeClient client(discardableManager); + + // Server. + auto serverTf = SkTypeface::MakeFromName("monospace", SkFontStyle()); + auto serverTfData = server.serializeTypeface(serverTf.get()); + + 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; + cache_diff_canvas.drawTextBlob(serverBlob.get(), 0, 0, paint); + + std::vector serverStrikeData; + server.writeStrikeData(&serverStrikeData); + + // Client. + REPORTER_ASSERT(reporter, + client.readStrikeData(serverStrikeData.data(), serverStrikeData.size())); + SkStrikeCache::Validate(); +}