Revert "Don't send strike with no pending glyphs"

This reverts commit 9e081d164c.

Reason for revert: Spike in cache misses: chromium:996636

BUG=chromium:996636

Original change's description:
> Don't send strike with no pending glyphs
>
> This changes tracks the strikes to send as a set of pointers
> to SkGlyphCacheState instead of as descriptors. This removes a descriptor
> lookup allowing rapid calculation of the actual number of strikes to send.
> Knowing the number of strikes with changes allows us to remove sending a boolean
> if strike has no pending glyphs.
>
> Change-Id: Id173336e238daebc19564248a818eb9dbdbe6db6
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/235827
> Reviewed-by: Mike Klein <mtklein@google.com>
> Reviewed-by: Khushal Sagar <khushalsagar@chromium.org>
> Commit-Queue: Herb Derby <herb@google.com>

TBR=mtklein@google.com,herb@google.com,khushalsagar@chromium.org,khushalsagar@google.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Change-Id: I0e0c8877be8d08be4b62a029005889068b6e4cc3
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/236351
Commit-Queue: Herb Derby <herb@google.com>
Reviewed-by: Herb Derby <herb@google.com>

src/core/SkRemoteGlyphCache.cpp

@@ -77,7 +77,7 @@ size_t serialization_alignment() {
 
 class Serializer {
 public:
-    explicit Serializer(std::vector<uint8_t>* buffer) : fBuffer{buffer} { }
+    Serializer(std::vector<uint8_t>* buffer) : fBuffer{buffer} { }
 
     template <typename T, typename... Args>
     T* emplace(Args&&... args) {
@@ -229,16 +229,14 @@ public:
 
     void onAboutToExitScope() override {}
 
+private:
     bool hasPendingGlyphs() const {
         return !fPendingGlyphImages.empty() || !fPendingGlyphPaths.empty();
     }
-
-    void resetScalerContext();
-
-private:
     void writeGlyphPath(const SkPackedGlyphID& glyphID, Serializer* serializer) const;
 
     void ensureScalerContext();
+    void resetScalerContext();
 
     // The set of glyphs cached on the remote client.
     SkTHashSet<SkPackedGlyphID> fCachedGlyphImages;
@@ -360,6 +358,9 @@ private:
 };
 
 // -- SkTextBlobCacheDiffCanvas -------------------------------------------------------------------
+// DEPRECATED
+// TODO(herb): remove uses in Chrome
+
 SkTextBlobCacheDiffCanvas::SkTextBlobCacheDiffCanvas(int width, int height,
                                                      const SkSurfaceProps& props,
                                                      SkStrikeServer* strikeServer,
@@ -427,34 +428,22 @@ sk_sp<SkData> SkStrikeServer::serializeTypeface(SkTypeface* tf) {
 }
 
 void SkStrikeServer::writeStrikeData(std::vector<uint8_t>* memory) {
-    size_t countStrikesToSend = 0;
+    if (fLockedDescs.empty() && fTypefacesToSend.empty()) {
-    for (SkGlyphCacheState* strike : fStrikesToSend) {
-        if (strike->hasPendingGlyphs()) {
-            // Only send strikes with pending glyphs.
-            countStrikesToSend++;
-        } else {
-            strike->resetScalerContext();
-        }
-    }
-
-    if (fTypefacesToSend.empty() && countStrikesToSend == 0) {
         return;
     }
 
     Serializer serializer(memory);
     serializer.emplace<uint64_t>(fTypefacesToSend.size());
-    for (const WireTypeface& tf : fTypefacesToSend) {
+    for (const auto& tf : fTypefacesToSend) serializer.write<WireTypeface>(tf);
-        serializer.write<WireTypeface>(tf);
-    }
     fTypefacesToSend.clear();
 
-    serializer.emplace<uint64_t>(countStrikesToSend);
+    serializer.emplace<uint64_t>(fLockedDescs.size());
-    for (SkGlyphCacheState* strike : fStrikesToSend) {
+    for (const auto* desc : fLockedDescs) {
-        if (strike->hasPendingGlyphs()) {
+        auto it = fRemoteGlyphStateMap.find(desc);
-            strike->writePendingGlyphs(&serializer);
+        SkASSERT(it != fRemoteGlyphStateMap.end());
-        }
+        it->second->writePendingGlyphs(&serializer);
     }
-    fStrikesToSend.clear();
+    fLockedDescs.clear();
 }
 
 SkStrikeServer::SkGlyphCacheState* SkStrikeServer::getOrCreateCache(
@@ -510,26 +499,29 @@ SkStrikeServer::SkGlyphCacheState* SkStrikeServer::getOrCreateCache(
             )
     );
 
+    // Already locked.
+    if (fLockedDescs.find(&desc) != fLockedDescs.end()) {
+        auto it = fRemoteGlyphStateMap.find(&desc);
+        SkASSERT(it != fRemoteGlyphStateMap.end());
+        SkGlyphCacheState* cache = it->second.get();
+        cache->setTypefaceAndEffects(&typeface, effects);
+        return cache;
+    }
+
     // Try to lock.
     auto it = fRemoteGlyphStateMap.find(&desc);
     if (it != fRemoteGlyphStateMap.end()) {
         SkGlyphCacheState* cache = it->second.get();
-        cache->setTypefaceAndEffects(&typeface, effects);
+        bool locked = fDiscardableHandleManager->lockHandle(it->second->discardableHandleId());
-        bool inserted;
+        if (locked) {
-        std::tie(std::ignore, inserted) = fStrikesToSend.insert(cache);
+            fLockedDescs.insert(it->first);
-        if (!inserted) {
+            cache->setTypefaceAndEffects(&typeface, effects);
-            // Already existed in fStrikesToSend.
             return cache;
-        } else {
-            bool locked = fDiscardableHandleManager->lockHandle(cache->discardableHandleId());
-            if (locked) {
-                return cache;
-            }
-            // If the lock failed, the entry was deleted on the client. Remove our
-            // tracking.
-            fStrikesToSend.erase(cache);
-            fRemoteGlyphStateMap.erase(it);
         }
+
+        // If the lock failed, the entry was deleted on the client. Remove our
+        // tracking.
+        fRemoteGlyphStateMap.erase(it);
     }
 
     const SkFontID typefaceId = typeface.uniqueID();
@@ -548,7 +540,7 @@ SkStrikeServer::SkGlyphCacheState* SkStrikeServer::getOrCreateCache(
 
     auto* cacheStatePtr = cacheState.get();
 
-    fStrikesToSend.insert(cacheStatePtr);
+    fLockedDescs.insert(&cacheStatePtr->getDescriptor());
     fRemoteGlyphStateMap[&cacheStatePtr->getDescriptor()] = std::move(cacheState);
 
     checkForDeletedEntries();
@@ -571,6 +563,13 @@ static void writeGlyph(SkGlyph* glyph, Serializer* serializer) {
 }
 
 void SkStrikeServer::SkGlyphCacheState::writePendingGlyphs(Serializer* serializer) {
+    // TODO(khushalsagar): Write a strike only if it has any pending glyphs.
+    serializer->emplace<bool>(this->hasPendingGlyphs());
+    if (!this->hasPendingGlyphs()) {
+        this->resetScalerContext();
+        return;
+    }
+
     // Write the desc.
     serializer->emplace<StrikeSpec>(fContext->getTypeface()->uniqueID(), fDiscardableHandleId);
     serializer->writeDescriptor(*fDescriptor.getDesc());
@@ -782,6 +781,11 @@ bool SkStrikeClient::readStrikeData(const volatile void* memory, size_t memorySi
     if (!deserializer.read<uint64_t>(&strikeCount)) READ_FAILURE
 
     for (size_t i = 0; i < strikeCount; ++i) {
+        bool has_glyphs = false;
+        if (!deserializer.read<bool>(&has_glyphs)) READ_FAILURE
+
+        if (!has_glyphs) continue;
+
         StrikeSpec spec;
         if (!deserializer.read<StrikeSpec>(&spec)) READ_FAILURE
 

src/core/SkRemoteGlyphCache.h

@@ -47,6 +47,9 @@ template <typename T>
 using SkDescriptorMap = std::unordered_map<const SkDescriptor*, T, SkDescriptorMapOperators,
                                            SkDescriptorMapOperators>;
 
+using SkDescriptorSet =
+        std::unordered_set<const SkDescriptor*, SkDescriptorMapOperators, SkDescriptorMapOperators>;
+
 // A SkTextBlobCacheDiffCanvas is used to populate the SkStrikeServer with ops
 // which will be serialized and rendered using the SkStrikeClient.
 class SkTextBlobCacheDiffCanvas : public SkNoDrawCanvas {
@@ -152,7 +155,7 @@ private:
     SkTHashMap<SkFontID, sk_sp<SkData>> fSerializedTypefaces;
 
     // State cached until the next serialization.
-    std::unordered_set<SkGlyphCacheState*> fStrikesToSend;
+    SkDescriptorSet fLockedDescs;
     std::vector<WireTypeface> fTypefacesToSend;
 };