From c337c9e02057fc61ef22ac80fa2e5f3dae92b498 Mon Sep 17 00:00:00 2001 From: Florin Malita Date: Fri, 10 Mar 2017 18:02:29 +0000 Subject: Revert "Revert "Hold sk_sp refs in GrTextBlobCache instead of raw ptrs"" This reverts commit 3304c447b953dad79fe7f355184ac13ed7e302e0. Reason for revert: Fix for SkTHashMap issue landed Original change's description: > Revert "Hold sk_sp refs in GrTextBlobCache instead of raw ptrs" > > This reverts commit db3ceb86421fb9da86bb920e3a1f0957beec08d9. > > Reason for revert: observing some strange budget behavior w/ instrumented Chromium builds; need to investigate. > > Original change's description: > > Hold sk_sp refs in GrTextBlobCache instead of raw ptrs > > > > Refactor to store sk_sps, and minimize explicit ref manipulation. > > > > Change-Id: Ie3d18e5fe1cefbbc5c2f3c4941287a24038522a6 > > Reviewed-on: https://skia-review.googlesource.com/9490 > > Commit-Queue: Florin Malita > > Reviewed-by: Brian Salomon > > > > TBR=bsalomon@google.com,robertphillips@google.com,fmalita@chromium.org,reviews@skia.org > NOPRESUBMIT=true > NOTREECHECKS=true > NOTRY=true > > Change-Id: I8ca9862ad1519a9ec69ad1ce8e4d129b0dae7b0a > Reviewed-on: https://skia-review.googlesource.com/9524 > Reviewed-by: Florin Malita > Commit-Queue: Florin Malita > TBR=bsalomon@google.com,robertphillips@google.com,reviews@skia.org,fmalita@chromium.org,fmalita@google.com NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true Change-Id: I1ba50e3b574381717fbbf46b829d72aceff8f7fe Reviewed-on: https://skia-review.googlesource.com/9535 Reviewed-by: Florin Malita Commit-Queue: Florin Malita --- src/gpu/text/GrAtlasTextBlob.cpp | 7 +-- src/gpu/text/GrAtlasTextBlob.h | 2 +- src/gpu/text/GrAtlasTextContext.cpp | 96 ++++++++++++++++++------------------- src/gpu/text/GrAtlasTextContext.h | 36 +++++++------- src/gpu/text/GrTextBlobCache.cpp | 7 ++- src/gpu/text/GrTextBlobCache.h | 68 +++++++++++++------------- 6 files changed, 107 insertions(+), 109 deletions(-) (limited to 'src/gpu') diff --git a/src/gpu/text/GrAtlasTextBlob.cpp b/src/gpu/text/GrAtlasTextBlob.cpp index accdf6b693..5427855cfc 100644 --- a/src/gpu/text/GrAtlasTextBlob.cpp +++ b/src/gpu/text/GrAtlasTextBlob.cpp @@ -17,7 +17,7 @@ #include "SkTextBlobRunIterator.h" #include "ops/GrAtlasTextOp.h" -GrAtlasTextBlob* GrAtlasTextBlob::Create(GrMemoryPool* pool, int glyphCount, int runCount) { +sk_sp GrAtlasTextBlob::Make(GrMemoryPool* pool, int glyphCount, int runCount) { // We allocate size for the GrAtlasTextBlob itself, plus size for the vertices array, // and size for the glyphIds array. size_t verticesCount = glyphCount * kVerticesPerGlyph * kMaxVASize; @@ -31,11 +31,12 @@ GrAtlasTextBlob* GrAtlasTextBlob::Create(GrMemoryPool* pool, int glyphCount, int sk_bzero(allocation, size); } - GrAtlasTextBlob* cacheBlob = new (allocation) GrAtlasTextBlob; + sk_sp cacheBlob(new (allocation) GrAtlasTextBlob); cacheBlob->fSize = size; // setup offsets for vertices / glyphs - cacheBlob->fVertices = sizeof(GrAtlasTextBlob) + reinterpret_cast(cacheBlob); + cacheBlob->fVertices = sizeof(GrAtlasTextBlob) + + reinterpret_cast(cacheBlob.get()); cacheBlob->fGlyphs = reinterpret_cast(cacheBlob->fVertices + verticesCount); cacheBlob->fRuns = reinterpret_cast(cacheBlob->fGlyphs + glyphCount); diff --git a/src/gpu/text/GrAtlasTextBlob.h b/src/gpu/text/GrAtlasTextBlob.h index 7d686a93a6..47dd1db5c5 100644 --- a/src/gpu/text/GrAtlasTextBlob.h +++ b/src/gpu/text/GrAtlasTextBlob.h @@ -51,7 +51,7 @@ class GrAtlasTextBlob : public SkNVRefCnt { public: SK_DECLARE_INTERNAL_LLIST_INTERFACE(GrAtlasTextBlob); - static GrAtlasTextBlob* Create(GrMemoryPool* pool, int glyphCount, int runCount); + static sk_sp Make(GrMemoryPool* pool, int glyphCount, int runCount); struct Key { Key() { diff --git a/src/gpu/text/GrAtlasTextContext.cpp b/src/gpu/text/GrAtlasTextContext.cpp index e15281ef47..4e377c9595 100644 --- a/src/gpu/text/GrAtlasTextContext.cpp +++ b/src/gpu/text/GrAtlasTextContext.cpp @@ -115,7 +115,7 @@ void GrAtlasTextContext::drawTextBlob(GrContext* context, GrRenderTargetContext* key.fHasBlur = SkToBool(mf); key.fCanonicalColor = canonicalColor; key.fScalerContextFlags = scalerContextFlags; - cacheBlob.reset(SkSafeRef(cache->find(key))); + cacheBlob = cache->find(key); } GrTextUtils::Paint paint(&skPaint); @@ -125,7 +125,7 @@ void GrAtlasTextContext::drawTextBlob(GrContext* context, GrRenderTargetContext* // TODO we could probably get away reuse most of the time if the pointer is unique, // but we'd have to clear the subrun information cache->remove(cacheBlob.get()); - cacheBlob.reset(SkRef(cache->createCachedBlob(blob, key, blurRec, skPaint))); + cacheBlob = cache->makeCachedBlob(blob, key, blurRec, skPaint); RegenerateTextBlob(cacheBlob.get(), context->getAtlasGlyphCache(), *context->caps()->shaderCaps(), paint, scalerContextFlags, viewMatrix, props, blob, x, y, drawFilter); @@ -136,7 +136,7 @@ void GrAtlasTextContext::drawTextBlob(GrContext* context, GrRenderTargetContext* int glyphCount = 0; int runCount = 0; GrTextBlobCache::BlobGlyphCount(&glyphCount, &runCount, blob); - sk_sp sanityBlob(cache->createBlob(glyphCount, runCount)); + sk_sp sanityBlob(cache->makeBlob(glyphCount, runCount)); sanityBlob->setupKey(key, blurRec, skPaint); RegenerateTextBlob(sanityBlob.get(), context->getAtlasGlyphCache(), *context->caps()->shaderCaps(), paint, scalerContextFlags, @@ -146,9 +146,9 @@ void GrAtlasTextContext::drawTextBlob(GrContext* context, GrRenderTargetContext* } } else { if (canCache) { - cacheBlob.reset(SkRef(cache->createCachedBlob(blob, key, blurRec, skPaint))); + cacheBlob = cache->makeCachedBlob(blob, key, blurRec, skPaint); } else { - cacheBlob.reset(cache->createBlob(blob)); + cacheBlob = cache->makeBlob(blob); } RegenerateTextBlob(cacheBlob.get(), context->getAtlasGlyphCache(), *context->caps()->shaderCaps(), paint, scalerContextFlags, viewMatrix, @@ -230,52 +230,52 @@ void GrAtlasTextContext::RegenerateTextBlob(GrAtlasTextBlob* cacheBlob, } } -inline GrAtlasTextBlob* -GrAtlasTextContext::CreateDrawTextBlob(GrTextBlobCache* blobCache, - GrAtlasGlyphCache* fontCache, - const GrShaderCaps& shaderCaps, - const GrTextUtils::Paint& paint, - uint32_t scalerContextFlags, - const SkMatrix& viewMatrix, - const SkSurfaceProps& props, - const char text[], size_t byteLength, - SkScalar x, SkScalar y) { +inline sk_sp +GrAtlasTextContext::MakeDrawTextBlob(GrTextBlobCache* blobCache, + GrAtlasGlyphCache* fontCache, + const GrShaderCaps& shaderCaps, + const GrTextUtils::Paint& paint, + uint32_t scalerContextFlags, + const SkMatrix& viewMatrix, + const SkSurfaceProps& props, + const char text[], size_t byteLength, + SkScalar x, SkScalar y) { int glyphCount = paint.skPaint().countText(text, byteLength); - GrAtlasTextBlob* blob = blobCache->createBlob(glyphCount, 1); + sk_sp blob = blobCache->makeBlob(glyphCount, 1); blob->initThrowawayBlob(viewMatrix, x, y); if (GrTextUtils::CanDrawAsDistanceFields(paint, viewMatrix, props, shaderCaps)) { - GrTextUtils::DrawDFText(blob, 0, fontCache, props, paint, scalerContextFlags, viewMatrix, - text, byteLength, x, y); + GrTextUtils::DrawDFText(blob.get(), 0, fontCache, props, paint, scalerContextFlags, + viewMatrix, text, byteLength, x, y); } else { - GrTextUtils::DrawBmpText(blob, 0, fontCache, props, paint, scalerContextFlags, viewMatrix, - text, byteLength, x, y); + GrTextUtils::DrawBmpText(blob.get(), 0, fontCache, props, paint, scalerContextFlags, + viewMatrix, text, byteLength, x, y); } return blob; } -inline GrAtlasTextBlob* -GrAtlasTextContext::CreateDrawPosTextBlob(GrTextBlobCache* blobCache, - GrAtlasGlyphCache* fontCache, - const GrShaderCaps& shaderCaps, - const GrTextUtils::Paint& paint, - uint32_t scalerContextFlags, - const SkMatrix& viewMatrix, - const SkSurfaceProps& props, - const char text[], size_t byteLength, - const SkScalar pos[], int scalarsPerPosition, const - SkPoint& offset) { +inline sk_sp +GrAtlasTextContext::MakeDrawPosTextBlob(GrTextBlobCache* blobCache, + GrAtlasGlyphCache* fontCache, + const GrShaderCaps& shaderCaps, + const GrTextUtils::Paint& paint, + uint32_t scalerContextFlags, + const SkMatrix& viewMatrix, + const SkSurfaceProps& props, + const char text[], size_t byteLength, + const SkScalar pos[], int scalarsPerPosition, const + SkPoint& offset) { int glyphCount = paint.skPaint().countText(text, byteLength); - GrAtlasTextBlob* blob = blobCache->createBlob(glyphCount, 1); + sk_sp blob = blobCache->makeBlob(glyphCount, 1); blob->initThrowawayBlob(viewMatrix, offset.x(), offset.y()); if (GrTextUtils::CanDrawAsDistanceFields(paint, viewMatrix, props, shaderCaps)) { - GrTextUtils::DrawDFPosText(blob, 0, fontCache, props, paint, scalerContextFlags, viewMatrix, - text, byteLength, pos, scalarsPerPosition, offset); + GrTextUtils::DrawDFPosText(blob.get(), 0, fontCache, props, paint, scalerContextFlags, + viewMatrix, text, byteLength, pos, scalarsPerPosition, offset); } else { - GrTextUtils::DrawBmpPosText(blob, 0, fontCache, props, paint, scalerContextFlags, + GrTextUtils::DrawBmpPosText(blob.get(), 0, fontCache, props, paint, scalerContextFlags, viewMatrix, text, byteLength, pos, scalarsPerPosition, offset); } return blob; @@ -292,11 +292,11 @@ void GrAtlasTextContext::drawText(GrContext* context, GrRenderTargetContext* rtc GrTextUtils::Paint paint(&skPaint); if (this->canDraw(skPaint, viewMatrix, props, *context->caps()->shaderCaps())) { sk_sp blob( - CreateDrawTextBlob(context->getTextBlobCache(), context->getAtlasGlyphCache(), - *context->caps()->shaderCaps(), - paint, ComputeScalerContextFlags(rtc), - viewMatrix, props, - text, byteLength, x, y)); + MakeDrawTextBlob(context->getTextBlobCache(), context->getAtlasGlyphCache(), + *context->caps()->shaderCaps(), + paint, ComputeScalerContextFlags(rtc), + viewMatrix, props, + text, byteLength, x, y)); blob->flushThrowaway(context, rtc, props, fDistanceAdjustTable.get(), paint, clip, viewMatrix, regionClipBounds, x, y); return; @@ -318,13 +318,13 @@ void GrAtlasTextContext::drawPosText(GrContext* context, GrRenderTargetContext* return; } else if (this->canDraw(skPaint, viewMatrix, props, *context->caps()->shaderCaps())) { sk_sp blob( - CreateDrawPosTextBlob(context->getTextBlobCache(), context->getAtlasGlyphCache(), - *context->caps()->shaderCaps(), - paint, ComputeScalerContextFlags(rtc), - viewMatrix, props, - text, byteLength, - pos, scalarsPerPosition, - offset)); + MakeDrawPosTextBlob(context->getTextBlobCache(), context->getAtlasGlyphCache(), + *context->caps()->shaderCaps(), + paint, ComputeScalerContextFlags(rtc), + viewMatrix, props, + text, byteLength, + pos, scalarsPerPosition, + offset)); blob->flushThrowaway(context, rtc, props, fDistanceAdjustTable.get(), paint, clip, viewMatrix, regionClipBounds, offset.fX, offset.fY); return; @@ -377,7 +377,7 @@ DRAW_OP_TEST_DEFINE(TextBlobOp) { GrTextUtils::Paint paint(&skPaint); // right now we don't handle textblobs, nor do we handle drawPosText. Since we only intend to // test the text op with this unit test, that is okay. - sk_sp blob(GrAtlasTextContext::CreateDrawTextBlob( + sk_sp blob(GrAtlasTextContext::MakeDrawTextBlob( context->getTextBlobCache(), context->getAtlasGlyphCache(), *context->caps()->shaderCaps(), paint, GrAtlasTextContext::kTextBlobOpScalerContextFlags, viewMatrix, gSurfaceProps, text, diff --git a/src/gpu/text/GrAtlasTextContext.h b/src/gpu/text/GrAtlasTextContext.h index 20a736876b..7438b647b2 100644 --- a/src/gpu/text/GrAtlasTextContext.h +++ b/src/gpu/text/GrAtlasTextContext.h @@ -64,24 +64,24 @@ private: SkDrawFilter* drawFilter); inline static bool HasLCD(const SkTextBlob*); - static inline GrAtlasTextBlob* CreateDrawTextBlob(GrTextBlobCache*, GrAtlasGlyphCache*, - const GrShaderCaps&, - const GrTextUtils::Paint&, - uint32_t scalerContextFlags, - const SkMatrix& viewMatrix, - const SkSurfaceProps&, - const char text[], size_t byteLength, - SkScalar x, SkScalar y); - static inline GrAtlasTextBlob* CreateDrawPosTextBlob(GrTextBlobCache*, GrAtlasGlyphCache*, - const GrShaderCaps&, - const GrTextUtils::Paint&, - uint32_t scalerContextFlags, - const SkMatrix& viewMatrix, - const SkSurfaceProps&, - const char text[], size_t byteLength, - const SkScalar pos[], - int scalarsPerPosition, - const SkPoint& offset); + static inline sk_sp MakeDrawTextBlob(GrTextBlobCache*, GrAtlasGlyphCache*, + const GrShaderCaps&, + const GrTextUtils::Paint&, + uint32_t scalerContextFlags, + const SkMatrix& viewMatrix, + const SkSurfaceProps&, + const char text[], size_t byteLength, + SkScalar x, SkScalar y); + static inline sk_sp MakeDrawPosTextBlob(GrTextBlobCache*, GrAtlasGlyphCache*, + const GrShaderCaps&, + const GrTextUtils::Paint&, + uint32_t scalerContextFlags, + const SkMatrix& viewMatrix, + const SkSurfaceProps&, + const char text[], size_t byteLength, + const SkScalar pos[], + int scalarsPerPosition, + const SkPoint& offset); const GrDistanceFieldAdjustTable* dfAdjustTable() const { return fDistanceAdjustTable.get(); } sk_sp fDistanceAdjustTable; diff --git a/src/gpu/text/GrTextBlobCache.cpp b/src/gpu/text/GrTextBlobCache.cpp index f6dac691f4..c53fcb0492 100644 --- a/src/gpu/text/GrTextBlobCache.cpp +++ b/src/gpu/text/GrTextBlobCache.cpp @@ -8,14 +8,13 @@ #include "GrTextBlobCache.h" GrTextBlobCache::~GrTextBlobCache() { - this->freeAll(); + SkDEBUGCODE(this->freeAll();) } void GrTextBlobCache::freeAll() { fBlobIDCache.foreach([this](uint32_t, BlobIDCacheEntry* entry) { - for (auto* blob : entry->fBlobs) { - fBlobList.remove(blob); - blob->unref(); + for (const auto& blob : entry->fBlobs) { + fBlobList.remove(blob.get()); } }); diff --git a/src/gpu/text/GrTextBlobCache.h b/src/gpu/text/GrTextBlobCache.h index 886a0914ca..af6c7926fa 100644 --- a/src/gpu/text/GrTextBlobCache.h +++ b/src/gpu/text/GrTextBlobCache.h @@ -9,6 +9,7 @@ #define GrTextBlobCache_DEFINED #include "GrAtlasTextContext.h" +#include "SkRefCnt.h" #include "SkTArray.h" #include "SkTextBlobRunIterator.h" #include "SkTHash.h" @@ -31,31 +32,28 @@ public: ~GrTextBlobCache(); // creates an uncached blob - GrAtlasTextBlob* createBlob(int glyphCount, int runCount) { - return GrAtlasTextBlob::Create(&fPool, glyphCount, runCount); + sk_sp makeBlob(int glyphCount, int runCount) { + return GrAtlasTextBlob::Make(&fPool, glyphCount, runCount); } - GrAtlasTextBlob* createBlob(const SkTextBlob* blob) { + + sk_sp makeBlob(const SkTextBlob* blob) { int glyphCount = 0; int runCount = 0; BlobGlyphCount(&glyphCount, &runCount, blob); - GrAtlasTextBlob* cacheBlob = GrAtlasTextBlob::Create(&fPool, glyphCount, runCount); - return cacheBlob; + return GrAtlasTextBlob::Make(&fPool, glyphCount, runCount); } - GrAtlasTextBlob* createCachedBlob(const SkTextBlob* blob, - const GrAtlasTextBlob::Key& key, - const SkMaskFilter::BlurRec& blurRec, - const SkPaint& paint) { - int glyphCount = 0; - int runCount = 0; - BlobGlyphCount(&glyphCount, &runCount, blob); - GrAtlasTextBlob* cacheBlob = GrAtlasTextBlob::Create(&fPool, glyphCount, runCount); + sk_sp makeCachedBlob(const SkTextBlob* blob, + const GrAtlasTextBlob::Key& key, + const SkMaskFilter::BlurRec& blurRec, + const SkPaint& paint) { + sk_sp cacheBlob(this->makeBlob(blob)); cacheBlob->setupKey(key, blurRec, paint); this->add(cacheBlob); return cacheBlob; } - GrAtlasTextBlob* find(const GrAtlasTextBlob::Key& key) const { + sk_sp find(const GrAtlasTextBlob::Key& key) const { const auto* idEntry = fBlobIDCache.find(key.fUniqueID); return idEntry ? idEntry->find(key) : nullptr; } @@ -65,26 +63,11 @@ public: auto* idEntry = fBlobIDCache.find(id); SkASSERT(idEntry); + fBlobList.remove(blob); idEntry->removeBlob(blob); if (idEntry->fBlobs.empty()) { fBlobIDCache.remove(id); } - - fBlobList.remove(blob); - blob->unref(); - } - - void add(GrAtlasTextBlob* blob) { - auto id = GrAtlasTextBlob::GetKey(*blob).fUniqueID; - auto* idEntry = fBlobIDCache.find(id); - if (!idEntry) { - idEntry = fBlobIDCache.set(id, BlobIDCacheEntry(id)); - } - - idEntry->addBlob(blob); - fBlobList.addToHead(blob); - - this->checkPurge(blob); } void makeMRU(GrAtlasTextBlob* blob) { @@ -122,12 +105,12 @@ private: return entry.fID; } - void addBlob(GrAtlasTextBlob* blob) { + void addBlob(sk_sp blob) { SkASSERT(blob); SkASSERT(GrAtlasTextBlob::GetKey(*blob).fUniqueID == fID); SkASSERT(!this->find(GrAtlasTextBlob::GetKey(*blob))); - fBlobs.push_back(blob); + fBlobs.emplace_back(std::move(blob)); } void removeBlob(GrAtlasTextBlob* blob) { @@ -140,7 +123,7 @@ private: fBlobs.removeShuffle(index); } - GrAtlasTextBlob* find(const GrAtlasTextBlob::Key& key) const { + sk_sp find(const GrAtlasTextBlob::Key& key) const { auto index = this->findBlobIndex(key); return index < 0 ? nullptr : fBlobs[index]; } @@ -157,9 +140,24 @@ private: uint32_t fID; // Current clients don't generate multiple GrAtlasTextBlobs per SkTextBlob, so an array w/ // linear search is acceptable. If usage changes, we should re-evaluate this structure. - SkSTArray<1, GrAtlasTextBlob*, true> fBlobs; + SkSTArray<1, sk_sp, true> fBlobs; }; + void add(sk_sp blob) { + auto id = GrAtlasTextBlob::GetKey(*blob).fUniqueID; + auto* idEntry = fBlobIDCache.find(id); + if (!idEntry) { + idEntry = fBlobIDCache.set(id, BlobIDCacheEntry(id)); + } + + // Safe to retain a raw ptr temporarily here, because the cache will hold a ref. + GrAtlasTextBlob* rawBlobPtr = blob.get(); + fBlobList.addToHead(rawBlobPtr); + idEntry->addBlob(std::move(blob)); + + this->checkPurge(rawBlobPtr); + } + void checkPurge(GrAtlasTextBlob* blob = nullptr) { // If we are overbudget, then unref until we are below budget again if (fPool.size() > fBudget) { @@ -193,9 +191,9 @@ private: static const int kPreAllocSize = 1 << 17; static const int kMinGrowthSize = 1 << 17; static const int kDefaultBudget = 1 << 22; + GrMemoryPool fPool; BitmapBlobList fBlobList; SkTHashMap fBlobIDCache; - GrMemoryPool fPool; PFOverBudgetCB fCallback; void* fData; size_t fBudget; -- cgit v1.2.3