Revert "Multi-threaded strike cache"
This reverts commit a6cd7c0b1f989840efe203979943cbb59d4d131b.
Reason for revert: Introduced nullptr de-ref in last patch
Original change's description:
> Multi-threaded strike cache
>
> Allow multiple threads to share the same strike. The old
> system of removing the cache from the linked list is no longer.
> The strikes stay in the list and can be found by other threads.
>
> * Removed strike size verification. There was no way to get the
> locks to work out. The whole point of the change was to have multiple
> threads muting the structure at the same time.
>
> * Strikes are now refed instead of being checked out. Therefore,
> ExclusiveStrikePtr is now just wraps an sk_sp, and should be renamed
> in a future CL.
>
> Change-Id: I832642332a3106e30745f9cdd3156ae72d41fd0b
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/272057
> Reviewed-by: Ben Wagner <bungeman@google.com>
> Commit-Queue: Herb Derby <herb@google.com>
TBR=bungeman@google.com,herb@google.com
Change-Id: Ie1acd9b42e614c17cb63bda1b3fd4dd9827a8fa6
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/272378
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Herb Derby <herb@google.com>
diff --git a/src/core/SkScalerCache.cpp b/src/core/SkScalerCache.cpp
index 5cc69cc..7e8741a 100644
--- a/src/core/SkScalerCache.cpp
+++ b/src/core/SkScalerCache.cpp
@@ -237,7 +237,6 @@
void SkScalerCache::findIntercepts(const SkScalar bounds[2], SkScalar scale, SkScalar xPos,
SkGlyph* glyph, SkScalar* array, int* count) {
- SkAutoMutexExclusive lock{fMu};
glyph->ensureIntercepts(bounds, scale, xPos, array, count, &fAlloc);
}
@@ -259,3 +258,21 @@
SkDebugf("%s\n", msg.c_str());
}
+#ifdef SK_DEBUG
+size_t SkScalerCache::recalculateMemoryUsed() const {
+ SkAutoMutexExclusive lock{fMu};
+ size_t memoryUsed = sizeof(*this);
+ fGlyphMap.foreach ([&memoryUsed](const SkGlyph* glyphPtr) {
+ memoryUsed += sizeof(SkGlyph);
+ if (glyphPtr->setImageHasBeenCalled()) {
+ memoryUsed += glyphPtr->imageSize();
+ }
+ if (glyphPtr->setPathHasBeenCalled() && glyphPtr->path() != nullptr) {
+ memoryUsed += glyphPtr->path()->approximateBytesUsed();
+ }
+ });
+ return memoryUsed;
+}
+#endif // SK_DEBUG
+
+
diff --git a/src/core/SkScalerCache.h b/src/core/SkScalerCache.h
index 4d4496c..68b202b 100644
--- a/src/core/SkScalerCache.h
+++ b/src/core/SkScalerCache.h
@@ -85,6 +85,10 @@
SkScalerContext* getScalerContext() const { return fScalerContext.get(); }
+#ifdef SK_DEBUG
+ size_t recalculateMemoryUsed() const SK_EXCLUDES(fMu);
+#endif
+
private:
class GlyphMapHashTraits {
public:
diff --git a/src/core/SkStrikeCache.cpp b/src/core/SkStrikeCache.cpp
index fe40da2..51d40c2 100644
--- a/src/core/SkStrikeCache.cpp
+++ b/src/core/SkStrikeCache.cpp
@@ -31,25 +31,35 @@
return cache;
}
-SkStrikeCache::ExclusiveStrikePtr::ExclusiveStrikePtr(sk_sp<Strike> strike)
- : fStrike{std::move(strike)} {}
+SkStrikeCache::ExclusiveStrikePtr::ExclusiveStrikePtr(SkStrikeCache::Strike* strike)
+ : fStrike{strike} {}
SkStrikeCache::ExclusiveStrikePtr::ExclusiveStrikePtr()
: fStrike{nullptr} {}
SkStrikeCache::ExclusiveStrikePtr::ExclusiveStrikePtr(ExclusiveStrikePtr&& o)
- : fStrike{std::move(o.fStrike)} {
+ : fStrike{o.fStrike} {
o.fStrike = nullptr;
}
SkStrikeCache::ExclusiveStrikePtr&
-SkStrikeCache::ExclusiveStrikePtr::operator = (ExclusiveStrikePtr&& that) {
- fStrike = std::move(that.fStrike);
+SkStrikeCache::ExclusiveStrikePtr::operator = (ExclusiveStrikePtr&& o) {
+ if (fStrike != nullptr) {
+ fStrike->fStrikeCache->attachStrike(fStrike);
+ }
+ fStrike = o.fStrike;
+ o.fStrike = nullptr;
return *this;
}
+SkStrikeCache::ExclusiveStrikePtr::~ExclusiveStrikePtr() {
+ if (fStrike != nullptr) {
+ fStrike->fStrikeCache->attachStrike(fStrike);
+ }
+}
+
SkStrike* SkStrikeCache::ExclusiveStrikePtr::get() const {
- return fStrike.get();
+ return fStrike;
}
SkStrike* SkStrikeCache::ExclusiveStrikePtr::operator -> () const {
@@ -94,8 +104,8 @@
auto SkStrikeCache::findOrCreateStrike(const SkDescriptor& desc,
const SkScalerContextEffects& effects,
- const SkTypeface& typeface) -> sk_sp<Strike> {
- sk_sp<Strike> strike = this->findStrikeOrNull(desc);
+ const SkTypeface& typeface) -> Strike* {
+ Strike* strike = this->findAndDetachStrike(desc);
if (strike == nullptr) {
auto scaler = typeface.createScalerContext(effects, &desc);
strike = this->createStrike(desc, std::move(scaler));
@@ -106,7 +116,7 @@
SkScopedStrikeForGPU SkStrikeCache::findOrCreateScopedStrike(const SkDescriptor& desc,
const SkScalerContextEffects& effects,
const SkTypeface& typeface) {
- return SkScopedStrikeForGPU{this->findOrCreateStrike(desc, effects, typeface).release()};
+ return SkScopedStrikeForGPU{this->findOrCreateStrike(desc, effects, typeface)};
}
void SkStrikeCache::PurgeAll() {
@@ -178,30 +188,30 @@
GlobalStrikeCache()->forEachStrike(visitor);
}
-SkExclusiveStrikePtr SkStrikeCache::findStrikeExclusive(const SkDescriptor& desc) {
- return SkExclusiveStrikePtr(this->findStrikeOrNull(desc));
+
+void SkStrikeCache::attachStrike(Strike* strike) {
+ if (strike == nullptr) {
+ return;
+ }
+ SkAutoSpinlock ac(fLock);
+
+ this->validate();
+
+ this->internalAttachToHead(strike);
+ this->internalPurge();
}
-auto SkStrikeCache::findStrikeOrNull(const SkDescriptor& desc) -> sk_sp<Strike> {
+SkExclusiveStrikePtr SkStrikeCache::findStrikeExclusive(const SkDescriptor& desc) {
+ return SkExclusiveStrikePtr(this->findAndDetachStrike(desc));
+}
+
+auto SkStrikeCache::findAndDetachStrike(const SkDescriptor& desc) -> Strike* {
SkAutoSpinlock ac(fLock);
for (Strike* strike = fHead; strike != nullptr; strike = strike->fNext) {
if (strike->fScalerCache.getDescriptor() == desc) {
- if (fHead != strike) {
- // Make most recently used
- strike->fPrev->fNext = strike->fNext;
- if (strike->fNext != nullptr) {
- strike->fNext->fPrev = strike->fPrev;
- } else {
- fTail = strike->fPrev;
- }
- fHead->fPrev = strike;
- strike->fNext = fHead;
- strike->fPrev = nullptr;
- fHead = strike;
- }
-
- return sk_ref_sp(strike);
+ this->internalDetachStrike(strike);
+ return strike;
}
}
@@ -222,12 +232,8 @@
const SkDescriptor& desc,
std::unique_ptr<SkScalerContext> scaler,
SkFontMetrics* maybeMetrics,
- std::unique_ptr<SkStrikePinner> pinner) -> sk_sp<Strike> {
- auto strike =
- sk_make_sp<Strike>(this, desc, std::move(scaler), maybeMetrics, std::move(pinner));
- SkAutoSpinlock lock{fLock};
- this->internalAttachToHead(strike);
- return strike;
+ std::unique_ptr<SkStrikePinner> pinner) -> Strike* {
+ return new Strike{this, desc, std::move(scaler), maybeMetrics, std::move(pinner)};
}
void SkStrikeCache::purgeAll() {
@@ -347,7 +353,8 @@
if (strike->fPinner == nullptr || strike->fPinner->canDelete()) {
bytesFreed += strike->fMemoryUsed;
countFreed += 1;
- this->internalRemoveStrike(strike);
+ this->internalDetachStrike(strike);
+ strike->unref();
}
strike = prev;
}
@@ -364,25 +371,23 @@
return bytesFreed;
}
-void SkStrikeCache::internalAttachToHead(sk_sp<Strike> strike) {
+void SkStrikeCache::internalAttachToHead(Strike* strike) {
SkASSERT(nullptr == strike->fPrev && nullptr == strike->fNext);
+ if (fHead) {
+ fHead->fPrev = strike;
+ strike->fNext = fHead;
+ }
+ fHead = strike;
+
+ if (fTail == nullptr) {
+ fTail = strike;
+ }
fCacheCount += 1;
fTotalMemoryUsed += strike->fMemoryUsed;
-
- if (fHead) {
- fHead->fPrev = strike.get();
- strike->fNext = fHead;
- }
-
- if (fTail == nullptr) {
- fTail = strike.get();
- }
-
- fHead = strike.release(); // Transfer ownership of strike to the cache list.
}
-void SkStrikeCache::internalRemoveStrike(Strike* strike) {
+void SkStrikeCache::internalDetachStrike(Strike* strike) {
SkASSERT(fCacheCount > 0);
fCacheCount -= 1;
fTotalMemoryUsed -= strike->fMemoryUsed;
@@ -398,10 +403,23 @@
fTail = strike->fPrev;
}
strike->fPrev = strike->fNext = nullptr;
- strike->fStrikeCache = nullptr;
- strike->unref();
}
+void SkStrikeCache::ValidateGlyphCacheDataSize() {
+#ifdef SK_DEBUG
+ GlobalStrikeCache()->validateGlyphCacheDataSize();
+#endif
+}
+
+#ifdef SK_DEBUG
+void SkStrikeCache::validateGlyphCacheDataSize() const {
+ this->forEachStrike(
+ [](const Strike& strike) {
+ SkASSERT(strike.fMemoryUsed == strike.fScalerCache.recalculateMemoryUsed());
+ });
+}
+#endif
+
#ifdef SK_DEBUG
void SkStrikeCache::validate() const {
size_t computedBytes = 0;
diff --git a/src/core/SkStrikeCache.h b/src/core/SkStrikeCache.h
index 089e164..8a273a5 100644
--- a/src/core/SkStrikeCache.h
+++ b/src/core/SkStrikeCache.h
@@ -55,14 +55,16 @@
, fPinner{std::move(pinner)} {}
SkGlyph* mergeGlyphAndImage(SkPackedGlyphID toID, const SkGlyph& from) {
- auto [glyph, increase] = fScalerCache.mergeGlyphAndImage(toID, from);
- this->updateDelta(increase);
+ auto [glyph, delta] = fScalerCache.mergeGlyphAndImage(toID, from);
+ fMemoryUsed += delta;
+ SkASSERT(fScalerCache.recalculateMemoryUsed() == fMemoryUsed);
return glyph;
}
const SkPath* mergePath(SkGlyph* glyph, const SkPath* path) {
- auto [glyphPath, increase] = fScalerCache.mergePath(glyph, path);
- this->updateDelta(increase);
+ auto [glyphPath, delta] = fScalerCache.mergePath(glyph, path);
+ fMemoryUsed += delta;
+ SkASSERT(fScalerCache.recalculateMemoryUsed() == fMemoryUsed);
return glyphPath;
}
@@ -81,28 +83,32 @@
SkSpan<const SkGlyph*> metrics(SkSpan<const SkGlyphID> glyphIDs,
const SkGlyph* results[]) {
- auto [glyphs, increase] = fScalerCache.metrics(glyphIDs, results);
- this->updateDelta(increase);
+ auto [glyphs, delta] = fScalerCache.metrics(glyphIDs, results);
+ fMemoryUsed += delta;
+ SkASSERT(fScalerCache.recalculateMemoryUsed() == fMemoryUsed);
return glyphs;
}
SkSpan<const SkGlyph*> preparePaths(SkSpan<const SkGlyphID> glyphIDs,
const SkGlyph* results[]) {
- auto [glyphs, increase] = fScalerCache.preparePaths(glyphIDs, results);
- this->updateDelta(increase);
+ auto [glyphs, delta] = fScalerCache.preparePaths(glyphIDs, results);
+ fMemoryUsed += delta;
+ SkASSERT(fScalerCache.recalculateMemoryUsed() == fMemoryUsed);
return glyphs;
}
SkSpan<const SkGlyph*> prepareImages(SkSpan<const SkPackedGlyphID> glyphIDs,
const SkGlyph* results[]) {
- auto [glyphs, increase] = fScalerCache.prepareImages(glyphIDs, results);
- this->updateDelta(increase);
+ auto [glyphs, delta] = fScalerCache.prepareImages(glyphIDs, results);
+ fMemoryUsed += delta;
+ SkASSERT(fScalerCache.recalculateMemoryUsed() == fMemoryUsed);
return glyphs;
}
void prepareForDrawingMasksCPU(SkDrawableGlyphBuffer* drawables) {
- size_t increase = fScalerCache.prepareForDrawingMasksCPU(drawables);
- this->updateDelta(increase);
+ size_t delta = fScalerCache.prepareForDrawingMasksCPU(drawables);
+ fMemoryUsed += delta;
+ SkASSERT(fScalerCache.recalculateMemoryUsed() == fMemoryUsed);
}
const SkGlyphPositionRoundingSpec& roundingSpec() const override {
@@ -115,37 +121,30 @@
void prepareForMaskDrawing(
SkDrawableGlyphBuffer* drawbles, SkSourceGlyphBuffer* rejects) override {
- size_t increase = fScalerCache.prepareForMaskDrawing(drawbles, rejects);
- this->updateDelta(increase);
+ size_t delta = fScalerCache.prepareForMaskDrawing(drawbles, rejects);
+ fMemoryUsed += delta;
+ SkASSERT(fScalerCache.recalculateMemoryUsed() == fMemoryUsed);
}
void prepareForSDFTDrawing(
SkDrawableGlyphBuffer* drawbles, SkSourceGlyphBuffer* rejects) override {
- size_t increase = fScalerCache.prepareForSDFTDrawing(drawbles, rejects);
- this->updateDelta(increase);
+ size_t delta = fScalerCache.prepareForSDFTDrawing(drawbles, rejects);
+ fMemoryUsed += delta;
+ SkASSERT(fScalerCache.recalculateMemoryUsed() == fMemoryUsed);
}
void prepareForPathDrawing(
SkDrawableGlyphBuffer* drawbles, SkSourceGlyphBuffer* rejects) override {
- size_t increase = fScalerCache.prepareForPathDrawing(drawbles, rejects);
- this->updateDelta(increase);
+ size_t delta = fScalerCache.prepareForPathDrawing(drawbles, rejects);
+ fMemoryUsed += delta;
+ SkASSERT(fScalerCache.recalculateMemoryUsed() == fMemoryUsed);
}
void onAboutToExitScope() override {
- this->unref();
+ fStrikeCache->attachStrike(this);
}
- void updateDelta(size_t increase) {
- if (increase != 0) {
- SkAutoSpinlock lock{fStrikeCache->fLock};
- fMemoryUsed += increase;
- if (fStrikeCache != nullptr) {
- fStrikeCache->fTotalMemoryUsed += increase;
- }
- }
- }
-
- SkStrikeCache* fStrikeCache{nullptr};
+ SkStrikeCache* const fStrikeCache;
Strike* fNext{nullptr};
Strike* fPrev{nullptr};
SkScalerCache fScalerCache;
@@ -155,12 +154,13 @@
class ExclusiveStrikePtr {
public:
- explicit ExclusiveStrikePtr(sk_sp<Strike> strike);
+ explicit ExclusiveStrikePtr(Strike*);
ExclusiveStrikePtr();
ExclusiveStrikePtr(const ExclusiveStrikePtr&) = delete;
ExclusiveStrikePtr& operator = (const ExclusiveStrikePtr&) = delete;
ExclusiveStrikePtr(ExclusiveStrikePtr&&);
ExclusiveStrikePtr& operator = (ExclusiveStrikePtr&&);
+ ~ExclusiveStrikePtr();
Strike* get() const;
Strike* operator -> () const;
@@ -171,7 +171,7 @@
friend bool operator == (decltype(nullptr), const ExclusiveStrikePtr&);
private:
- sk_sp<Strike> fStrike;
+ Strike* fStrike;
};
static SkStrikeCache* GlobalStrikeCache();
@@ -194,6 +194,7 @@
const SkTypeface& typeface) override;
static void PurgeAll();
+ static void ValidateGlyphCacheDataSize();
static void Dump();
// Dump memory usage statistics of all the attaches caches in the process using the
@@ -212,6 +213,12 @@
int getCachePointSizeLimit() const;
int setCachePointSizeLimit(int limit);
+#ifdef SK_DEBUG
+ // Make sure that each glyph cache's memory tracking and actual memory used are in sync.
+ void validateGlyphCacheDataSize() const;
+#else
+ void validateGlyphCacheDataSize() const {}
+#endif
private:
#ifdef SK_DEBUG
@@ -221,20 +228,21 @@
void validate() const {}
#endif
- sk_sp<Strike> findStrikeOrNull(const SkDescriptor& desc) SK_EXCLUDES(fLock);
- sk_sp<Strike> createStrike(
+ Strike* findAndDetachStrike(const SkDescriptor&) SK_EXCLUDES(fLock);
+ Strike* createStrike(
const SkDescriptor& desc,
std::unique_ptr<SkScalerContext> scaler,
SkFontMetrics* maybeMetrics = nullptr,
- std::unique_ptr<SkStrikePinner> = nullptr) SK_EXCLUDES(fLock);
- sk_sp<Strike> findOrCreateStrike(
+ std::unique_ptr<SkStrikePinner> = nullptr);
+ Strike* findOrCreateStrike(
const SkDescriptor& desc,
const SkScalerContextEffects& effects,
const SkTypeface& typeface) SK_EXCLUDES(fLock);
+ void attachStrike(Strike* strike) SK_EXCLUDES(fLock);
// The following methods can only be called when mutex is already held.
- void internalRemoveStrike(Strike* strike) SK_REQUIRES(fLock);
- void internalAttachToHead(sk_sp<Strike> strike) SK_REQUIRES(fLock);
+ void internalDetachStrike(Strike* strike) SK_REQUIRES(fLock);
+ void internalAttachToHead(Strike* strike) SK_REQUIRES(fLock);
// Checkout budgets, modulated by the specified min-bytes-needed-to-purge,
// and attempt to purge caches to match.
diff --git a/tests/SkRemoteGlyphCacheTest.cpp b/tests/SkRemoteGlyphCacheTest.cpp
index 1b870aa..96f94d7 100644
--- a/tests/SkRemoteGlyphCacheTest.cpp
+++ b/tests/SkRemoteGlyphCacheTest.cpp
@@ -403,6 +403,7 @@
// Client.
REPORTER_ASSERT(reporter,
client.readStrikeData(serverStrikeData.data(), serverStrikeData.size()));
+ SkStrikeCache::ValidateGlyphCacheDataSize();
// Must unlock everything on termination, otherwise valgrind complains about memory leaks.
discardableManager->unlockAndDeleteAll();
@@ -486,6 +487,7 @@
SkBitmap actual = RasterBlob(clientBlob, 10, 10, paint, ctxInfo.grContext());
compare_blobs(expected, actual, reporter, 1);
REPORTER_ASSERT(reporter, !discardableManager->hasCacheMiss());
+ SkStrikeCache::ValidateGlyphCacheDataSize();
// Must unlock everything on termination, otherwise valgrind complains about memory leaks.
discardableManager->unlockAndDeleteAll();
@@ -562,6 +564,7 @@
SkBitmap actual = RasterBlob(clientBlob, 10, 10, paint, ctxInfo.grContext());
compare_blobs(expected, actual, reporter);
REPORTER_ASSERT(reporter, !discardableManager->hasCacheMiss());
+ SkStrikeCache::ValidateGlyphCacheDataSize();
// Must unlock everything on termination, otherwise valgrind complains about memory leaks.
discardableManager->unlockAndDeleteAll();
@@ -636,6 +639,7 @@
// interpolation.
compare_blobs(expected, actual, reporter, 36);
REPORTER_ASSERT(reporter, !discardableManager->hasCacheMiss());
+ SkStrikeCache::ValidateGlyphCacheDataSize();
// Must unlock everything on termination, otherwise valgrind complains about memory leaks.
discardableManager->unlockAndDeleteAll();
@@ -673,6 +677,7 @@
SkBitmap actual = RasterBlob(clientBlob, 10, 10, paint, ctxInfo.grContext(), nullptr, 0.5);
compare_blobs(expected, actual, reporter);
REPORTER_ASSERT(reporter, !discardableManager->hasCacheMiss());
+ SkStrikeCache::ValidateGlyphCacheDataSize();
// Must unlock everything on termination, otherwise valgrind complains about memory leaks.
discardableManager->unlockAndDeleteAll();
@@ -718,6 +723,7 @@
SkBitmap actual = RasterBlob(clientBlob, 10, 10, paint, ctxInfo.grContext(), &matrix);
compare_blobs(expected, actual, reporter);
REPORTER_ASSERT(reporter, !discardableManager->hasCacheMiss());
+ SkStrikeCache::ValidateGlyphCacheDataSize();
// Must unlock everything on termination, otherwise valgrind complains about memory leaks.
discardableManager->unlockAndDeleteAll();
@@ -807,6 +813,7 @@
RasterBlob(clientBlob, 500, 500, paint, ctxInfo.grContext());
REPORTER_ASSERT(reporter, !discardableManager->hasCacheMiss());
+ SkStrikeCache::ValidateGlyphCacheDataSize();
discardableManager->resetCacheMissCounts();
}
