Add storage on the surface for its last render task

Let's land this and see if it gets us back to baseline on the
lastRenderTask regression from the bug. If it doesn't, we'll revert it
since the extra complexity won't have been worth it.

Bug: skia:10372
Change-Id: I9d5ae93435b833d575afdc7f219dc8e7c453c92b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/297836
Reviewed-by: Robert Phillips <robertphillips@google.com>
Commit-Queue: Adlai Holler <adlai@google.com>

gn/gpu.gni

@@ -31,6 +31,7 @@ skia_gpu_sources = [
   "$_src/gpu/GrAHardwareBufferImageGenerator.h",
   "$_src/gpu/GrAHardwareBufferUtils.cpp",
   "$_src/gpu/GrAHardwareBufferUtils.h",
+  "$_src/gpu/GrAffinedStorage.h",
   "$_src/gpu/GrAppliedClip.h",
   "$_src/gpu/GrAuditTrail.cpp",
   "$_src/gpu/GrAuditTrail.h",
@@ -109,7 +110,6 @@ skia_gpu_sources = [
   "$_src/gpu/GrGpuResource.h",
   "$_src/gpu/GrGpuResourceCacheAccess.h",
   "$_src/gpu/GrGpuResourcePriv.h",
-  "$_src/gpu/GrHashMapWithCache.h",
   "$_src/gpu/GrImageContext.cpp",
   "$_src/gpu/GrImageContextPriv.h",
   "$_src/gpu/GrImageInfo.h",

gn/tests.gni

@@ -99,6 +99,7 @@ tests_sources = [
   "$_tests/GpuDrawPathTest.cpp",
   "$_tests/GpuRectanizerTest.cpp",
   "$_tests/GrAHardwareBufferTest.cpp",
+  "$_tests/GrAffinedStorageTest.cpp",
   "$_tests/GrBlockAllocatorTest.cpp",
   "$_tests/GrCCPRTest.cpp",
   "$_tests/GrContextAbandonTest.cpp",

src/gpu/GrAffinedStorage.h

@@ -0,0 +1,57 @@
+/*
+ * Copyright 2020 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#ifndef GrAffinedStorage_DEFINED
+#define GrAffinedStorage_DEFINED
+
+#include "include/private/SkNoncopyable.h"
+
+#include <atomic>
+
+/** A container that can be owned by one thread-affined object at a time and used
+    for a while before being released then potentially picked up by another owner.
+    For example, if you need temporary storage that may be shared by many threads in some
+    use cases, but not shared at all in other use cases, you can use this class for one
+    immediately-available storage and fall back to slower storage when it's occupied.
+*/
+template <typename O, typename V>
+class GrAffinedStorage : public SkNoncopyable {
+public:
+    GrAffinedStorage(V val) : fValue(std::move(val)) {}
+
+    // Attempt to take ownership and set the value.
+    // Returns whether the operation was successful.
+    bool set(O* owner, V value) {
+        const O* oldOwner = nullptr;
+        bool took = fOwner.compare_exchange_strong(oldOwner, owner, std::memory_order_acquire);
+        if (took || oldOwner == owner) {
+            fValue = std::move(value);
+            return true;
+        }
+        return false;
+    }
+
+    // Attempt to access the internal storage for reading.
+    // Returns nullptr if not owned by the given owner.
+    const V* get(const O* owner) const {
+        if (owner == fOwner.load(std::memory_order_relaxed)) {
+            return &fValue;
+        }
+        return nullptr;
+    }
+
+    // Attempt to release ownership. Does nothing if not owned by the given owner.
+    bool release(const O* owner) {
+        return fOwner.compare_exchange_strong(owner, nullptr, std::memory_order_acquire);
+    }
+
+private:
+    std::atomic<const O*> fOwner{nullptr};
+    V                     fValue;
+};
+
+#endif

src/gpu/GrDrawingManager.cpp

@@ -589,6 +589,12 @@ void GrDrawingManager::setLastRenderTask(const GrSurfaceProxy* proxy, GrRenderTa
         SkASSERT(prior->isClosed());
     }
 #endif
+    // First try to store on the surface itself.
+    if (proxy->lastRenderTask().set(this, task)) {
+        return;
+    }
+
+    // Fall back to our table.
     uint32_t key = proxy->uniqueID().asUInt();
     if (task) {
         fLastRenderTasks.set(key, task);
@@ -598,6 +604,12 @@ void GrDrawingManager::setLastRenderTask(const GrSurfaceProxy* proxy, GrRenderTa
 }
 
 GrRenderTask* GrDrawingManager::getLastRenderTask(const GrSurfaceProxy* proxy) const {
+    if (auto* task = proxy->lastRenderTask().get(this)) {
+        return *task;
+    }
+    if (0 == fLastRenderTasks.count()) {
+        return nullptr;
+    }
     auto entry = fLastRenderTasks.find(proxy->uniqueID().asUInt());
     return entry ? *entry : nullptr;
 }

src/gpu/GrDrawingManager.h

@@ -13,7 +13,6 @@
 #include "include/private/SkTHash.h"
 #include "src/gpu/GrBufferAllocPool.h"
 #include "src/gpu/GrDeferredUpload.h"
-#include "src/gpu/GrHashMapWithCache.h"
 #include "src/gpu/GrPathRenderer.h"
 #include "src/gpu/GrPathRendererChain.h"
 #include "src/gpu/GrResourceCache.h"
@@ -253,13 +252,17 @@ private:
     // Note: we do not expect a whole lot of these per flush
     SkTHashMap<uint32_t, GrRenderTargetProxy*> fDDLTargets;
 
-    struct SurfaceIDKeyTraits {
-        static uint32_t GetInvalidKey() {
-            return GrSurfaceProxy::UniqueID::InvalidID().asUInt();
+    struct CheapHash {
+        uint32_t operator()(uint32_t value) {
+            return SkChecksum::CheapMix(value);
         }
     };
 
-    GrHashMapWithCache<uint32_t, GrRenderTask*, SurfaceIDKeyTraits, GrCheapHash> fLastRenderTasks;
+    // The last render task for a given surface ID.
+    // Note: This table only contains entries for GrSurfaceProxies whose inline lastRenderTask
+    // storage was already reserved by a different drawing manager (e.g. DDL recorders).
+    SkTHashMap<uint32_t, GrRenderTask*, CheapHash> fLastRenderTasks;
+
 };
 
 #endif

src/gpu/GrHashMapWithCache.h

@@ -1,84 +0,0 @@
-/*
- * Copyright 2020 Google Inc.
- *
- * Use of this source code is governed by a BSD-style license that can be
- * found in the LICENSE file.
- */
-
-#ifndef GrHashMapWithCache_DEFINED
-#define GrHashMapWithCache_DEFINED
-
-#include "include/private/SkChecksum.h"
-#include "include/private/SkNoncopyable.h"
-#include "include/private/SkTHash.h"
-
-// Cheaper than SkGoodHash and good enough for UniqueID tables.
-struct GrCheapHash {
-    uint32_t operator()(uint32_t val) {
-        return SkChecksum::CheapMix(val);
-    }
-};
-
-/** A hash map that caches the most recently accessed entry.
-    The API is a subset of SkHashMap, and you must provide a
-    sentinel key that will never be present, such as SK_InvalidUniqueID.
-
-    KeyTraits must have:
-      - static K GetInvalidKey()
-*/
-template <typename K, typename V, typename KeyTraits, typename HashT = SkGoodHash>
-class GrHashMapWithCache : public SkNoncopyable {
-public:
-    // How many key/value pairs are in the table?
-    int count() const { return fMap.count(); }
-
-    // Approximately how many bytes of memory do we use beyond sizeof(*this)?
-    size_t approxBytesUsed() const { return fMap.approxBytesUsed(); }
-
-    // N.B. The pointers returned by set() and find() are valid only until the next call to set().
-
-    // If there is key/value entry in the table with this key, return a pointer to the value.
-    // If not, return null.
-    const V* find(const K& key) const {
-        if (key != fLastKey) {
-            fLastKey = key;
-            fLastValue = fMap.find(key);
-        }
-        return fLastValue;
-    }
-
-    // Set key to val in the map, replacing any previous value with the same key.
-    // We copy both key and val, and return a pointer to the value copy now in the map.
-    const V* set(K key, V val) {
-        if (fLastValue && key == fLastKey) {
-            *fLastValue = std::move(val);
-        } else {
-            fLastKey = key;
-            fLastValue = fMap.set(std::move(key), std::move(val));
-        }
-        return fLastValue;
-    }
-
-    // Remove the key/value entry in the table with this key.
-    void remove(K key) {
-        // Match SkTHashMap requirement. The caller can find() if they're unsure.
-        SkASSERT(fMap.find(fLastKey));
-        fLastKey = std::move(key);
-        fLastValue = nullptr;
-        fMap.remove(fLastKey);
-    }
-
-    // Clear the map.
-    void reset() {
-        fLastKey = KeyTraits::GetInvalidKey();
-        fLastValue = nullptr;
-        fMap.reset();
-    }
-
-private:
-    SkTHashMap<K, V, HashT> fMap;
-    mutable K               fLastKey   = KeyTraits::GetInvalidKey();
-    mutable V*              fLastValue = nullptr;
-};
-
-#endif

src/gpu/GrRenderTask.cpp

@@ -38,6 +38,7 @@ void GrRenderTask::disown(GrDrawingManager* drawingMgr) {
     for (const GrSurfaceProxyView& target : fTargets) {
         if (this == drawingMgr->getLastRenderTask(target.proxy())) {
             drawingMgr->setLastRenderTask(target.proxy(), nullptr);
+            target.proxy()->lastRenderTask().release(drawingMgr);
         }
     }
 }

src/gpu/GrSurfaceProxy.h

@@ -11,6 +11,7 @@
 #include "include/core/SkRect.h"
 #include "include/gpu/GrBackendSurface.h"
 #include "include/private/SkNoncopyable.h"
+#include "src/gpu/GrAffinedStorage.h"
 #include "src/gpu/GrGpuResource.h"
 #include "src/gpu/GrNonAtomicRef.h"
 #include "src/gpu/GrSurface.h"
@@ -18,6 +19,7 @@
 
 class GrCaps;
 class GrContext_Base;
+class GrDrawingManager;
 class GrOpsTask;
 class GrRecordingContext;
 class GrRenderTargetProxy;
@@ -316,6 +318,12 @@ public:
                                       SkBackingFit,
                                       SkBudgeted);
 
+    // This accessor should only be used by the drawing manager as fast storage for the
+    // lastRenderTask association.
+    GrAffinedStorage<GrDrawingManager, GrRenderTask*>& lastRenderTask() const {
+        return fLastRenderTask;
+    }
+
 #if GR_TEST_UTILS
     int32_t testingOnly_getBackingRefCnt() const;
     GrInternalSurfaceFlags testingOnly_getFlags() const;
@@ -431,6 +439,8 @@ private:
     // If the proxy computes its own answer that answer is checked (in debug mode) in
     // the instantiation method.
     mutable size_t         fGpuMemorySize;
+
+    mutable GrAffinedStorage<GrDrawingManager, GrRenderTask*> fLastRenderTask{nullptr};
 };
 
 GR_MAKE_BITFIELD_CLASS_OPS(GrSurfaceProxy::ResolveFlags)

tests/GrAffinedStorageTest.cpp

@@ -0,0 +1,28 @@
+/*
+ * Copyright 2020 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include "tests/Test.h"
+
+#include "src/gpu/GrAffinedStorage.h"
+
+DEF_TEST(GrAffinedStorageTest, reporter) {
+    class Owner {};
+    Owner owner1, owner2;
+    GrAffinedStorage<void, int> storage{0};
+    REPORTER_ASSERT(reporter,   storage.set(&owner1, 1));
+    REPORTER_ASSERT(reporter,  !storage.set(&owner2, 2));
+    REPORTER_ASSERT(reporter,   1 == *storage.get(&owner1));
+    REPORTER_ASSERT(reporter,   storage.set(&owner1, 2));
+    REPORTER_ASSERT(reporter,   2 == *storage.get(&owner1));
+    REPORTER_ASSERT(reporter,  !storage.release(&owner2));
+    REPORTER_ASSERT(reporter,   storage.set(&owner1, 3));
+    REPORTER_ASSERT(reporter,   3 == *storage.get(&owner1));
+    REPORTER_ASSERT(reporter,   storage.release(&owner1));
+    REPORTER_ASSERT(reporter,  !storage.get(&owner1));
+    REPORTER_ASSERT(reporter,   storage.set(&owner2, 4));
+    REPORTER_ASSERT(reporter,   4 == *storage.get(&owner2));
+}