Revert "Revert "Hold sk_sp<GrAtlasTextBlob> 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<GrAtlasTextBlob> 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<GrAtlasTextBlob> 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 <fmalita@chromium.org>
> > Reviewed-by: Brian Salomon <bsalomon@google.com>
> >
>
> 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 <fmalita@google.com>
> Commit-Queue: Florin Malita <fmalita@google.com>
>
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 <fmalita@chromium.org>
Commit-Queue: Florin Malita <fmalita@chromium.org>
diff --git a/src/gpu/text/GrAtlasTextBlob.cpp b/src/gpu/text/GrAtlasTextBlob.cpp
index accdf6b..5427855 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> 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 @@
sk_bzero(allocation, size);
}
- GrAtlasTextBlob* cacheBlob = new (allocation) GrAtlasTextBlob;
+ sk_sp<GrAtlasTextBlob> cacheBlob(new (allocation) GrAtlasTextBlob);
cacheBlob->fSize = size;
// setup offsets for vertices / glyphs
- cacheBlob->fVertices = sizeof(GrAtlasTextBlob) + reinterpret_cast<unsigned char*>(cacheBlob);
+ cacheBlob->fVertices = sizeof(GrAtlasTextBlob) +
+ reinterpret_cast<unsigned char*>(cacheBlob.get());
cacheBlob->fGlyphs = reinterpret_cast<GrGlyph**>(cacheBlob->fVertices + verticesCount);
cacheBlob->fRuns = reinterpret_cast<GrAtlasTextBlob::Run*>(cacheBlob->fGlyphs + glyphCount);
diff --git a/src/gpu/text/GrAtlasTextBlob.h b/src/gpu/text/GrAtlasTextBlob.h
index 7d686a9..47dd1db 100644
--- a/src/gpu/text/GrAtlasTextBlob.h
+++ b/src/gpu/text/GrAtlasTextBlob.h
@@ -51,7 +51,7 @@
public:
SK_DECLARE_INTERNAL_LLIST_INTERFACE(GrAtlasTextBlob);
- static GrAtlasTextBlob* Create(GrMemoryPool* pool, int glyphCount, int runCount);
+ static sk_sp<GrAtlasTextBlob> 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 e15281e..4e377c9 100644
--- a/src/gpu/text/GrAtlasTextContext.cpp
+++ b/src/gpu/text/GrAtlasTextContext.cpp
@@ -115,7 +115,7 @@
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 @@
// 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 @@
int glyphCount = 0;
int runCount = 0;
GrTextBlobCache::BlobGlyphCount(&glyphCount, &runCount, blob);
- sk_sp<GrAtlasTextBlob> sanityBlob(cache->createBlob(glyphCount, runCount));
+ sk_sp<GrAtlasTextBlob> sanityBlob(cache->makeBlob(glyphCount, runCount));
sanityBlob->setupKey(key, blurRec, skPaint);
RegenerateTextBlob(sanityBlob.get(), context->getAtlasGlyphCache(),
*context->caps()->shaderCaps(), paint, scalerContextFlags,
@@ -146,9 +146,9 @@
}
} 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 @@
}
}
-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<GrAtlasTextBlob>
+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<GrAtlasTextBlob> 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<GrAtlasTextBlob>
+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<GrAtlasTextBlob> 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 @@
GrTextUtils::Paint paint(&skPaint);
if (this->canDraw(skPaint, viewMatrix, props, *context->caps()->shaderCaps())) {
sk_sp<GrAtlasTextBlob> 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 @@
return;
} else if (this->canDraw(skPaint, viewMatrix, props, *context->caps()->shaderCaps())) {
sk_sp<GrAtlasTextBlob> 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 @@
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<GrAtlasTextBlob> blob(GrAtlasTextContext::CreateDrawTextBlob(
+ sk_sp<GrAtlasTextBlob> 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 20a7368..7438b64 100644
--- a/src/gpu/text/GrAtlasTextContext.h
+++ b/src/gpu/text/GrAtlasTextContext.h
@@ -64,24 +64,24 @@
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<GrAtlasTextBlob> 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<GrAtlasTextBlob> 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<const GrDistanceFieldAdjustTable> fDistanceAdjustTable;
diff --git a/src/gpu/text/GrTextBlobCache.cpp b/src/gpu/text/GrTextBlobCache.cpp
index f6dac69..c53fcb0 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 886a091..af6c792 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 @@
~GrTextBlobCache();
// creates an uncached blob
- GrAtlasTextBlob* createBlob(int glyphCount, int runCount) {
- return GrAtlasTextBlob::Create(&fPool, glyphCount, runCount);
- }
- GrAtlasTextBlob* createBlob(const SkTextBlob* blob) {
- int glyphCount = 0;
- int runCount = 0;
- BlobGlyphCount(&glyphCount, &runCount, blob);
- GrAtlasTextBlob* cacheBlob = GrAtlasTextBlob::Create(&fPool, glyphCount, runCount);
- return cacheBlob;
+ sk_sp<GrAtlasTextBlob> makeBlob(int glyphCount, int runCount) {
+ return GrAtlasTextBlob::Make(&fPool, glyphCount, runCount);
}
- GrAtlasTextBlob* createCachedBlob(const SkTextBlob* blob,
- const GrAtlasTextBlob::Key& key,
- const SkMaskFilter::BlurRec& blurRec,
- const SkPaint& paint) {
+ sk_sp<GrAtlasTextBlob> makeBlob(const SkTextBlob* blob) {
int glyphCount = 0;
int runCount = 0;
BlobGlyphCount(&glyphCount, &runCount, blob);
- GrAtlasTextBlob* cacheBlob = GrAtlasTextBlob::Create(&fPool, glyphCount, runCount);
+ return GrAtlasTextBlob::Make(&fPool, glyphCount, runCount);
+ }
+
+ sk_sp<GrAtlasTextBlob> makeCachedBlob(const SkTextBlob* blob,
+ const GrAtlasTextBlob::Key& key,
+ const SkMaskFilter::BlurRec& blurRec,
+ const SkPaint& paint) {
+ sk_sp<GrAtlasTextBlob> cacheBlob(this->makeBlob(blob));
cacheBlob->setupKey(key, blurRec, paint);
this->add(cacheBlob);
return cacheBlob;
}
- GrAtlasTextBlob* find(const GrAtlasTextBlob::Key& key) const {
+ sk_sp<GrAtlasTextBlob> find(const GrAtlasTextBlob::Key& key) const {
const auto* idEntry = fBlobIDCache.find(key.fUniqueID);
return idEntry ? idEntry->find(key) : nullptr;
}
@@ -65,26 +63,11 @@
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 @@
return entry.fID;
}
- void addBlob(GrAtlasTextBlob* blob) {
+ void addBlob(sk_sp<GrAtlasTextBlob> 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 @@
fBlobs.removeShuffle(index);
}
- GrAtlasTextBlob* find(const GrAtlasTextBlob::Key& key) const {
+ sk_sp<GrAtlasTextBlob> find(const GrAtlasTextBlob::Key& key) const {
auto index = this->findBlobIndex(key);
return index < 0 ? nullptr : fBlobs[index];
}
@@ -157,9 +140,24 @@
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<GrAtlasTextBlob>, true> fBlobs;
};
+ void add(sk_sp<GrAtlasTextBlob> 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 @@
static const int kPreAllocSize = 1 << 17;
static const int kMinGrowthSize = 1 << 17;
static const int kDefaultBudget = 1 << 22;
+ GrMemoryPool fPool;
BitmapBlobList fBlobList;
SkTHashMap<uint32_t, BlobIDCacheEntry> fBlobIDCache;
- GrMemoryPool fPool;
PFOverBudgetCB fCallback;
void* fData;
size_t fBudget;