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>
diff --git a/gn/gpu.gni b/gn/gpu.gni
index 196b054..c074c20 100644
--- a/gn/gpu.gni
+++ b/gn/gpu.gni
@@ -31,6 +31,7 @@
"$_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 @@
"$_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",
diff --git a/gn/tests.gni b/gn/tests.gni
index 70ef573..c6ec1b9 100644
--- a/gn/tests.gni
+++ b/gn/tests.gni
@@ -99,6 +99,7 @@
"$_tests/GpuDrawPathTest.cpp",
"$_tests/GpuRectanizerTest.cpp",
"$_tests/GrAHardwareBufferTest.cpp",
+ "$_tests/GrAffinedStorageTest.cpp",
"$_tests/GrBlockAllocatorTest.cpp",
"$_tests/GrCCPRTest.cpp",
"$_tests/GrContextAbandonTest.cpp",
diff --git a/src/gpu/GrAffinedStorage.h b/src/gpu/GrAffinedStorage.h
new file mode 100644
index 0000000..b89c478
--- /dev/null
+++ b/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
diff --git a/src/gpu/GrDrawingManager.cpp b/src/gpu/GrDrawingManager.cpp
index 504579b..d6028a3 100644
--- a/src/gpu/GrDrawingManager.cpp
+++ b/src/gpu/GrDrawingManager.cpp
@@ -589,6 +589,12 @@
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 @@
}
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;
}
diff --git a/src/gpu/GrDrawingManager.h b/src/gpu/GrDrawingManager.h
index 6274bce..d869f24 100644
--- a/src/gpu/GrDrawingManager.h
+++ b/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 @@
// 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
diff --git a/src/gpu/GrHashMapWithCache.h b/src/gpu/GrHashMapWithCache.h
deleted file mode 100644
index 8a24ecc..0000000
--- a/src/gpu/GrHashMapWithCache.h
+++ /dev/null
@@ -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
diff --git a/src/gpu/GrRenderTask.cpp b/src/gpu/GrRenderTask.cpp
index 7b294a4..33ee510 100644
--- a/src/gpu/GrRenderTask.cpp
+++ b/src/gpu/GrRenderTask.cpp
@@ -38,6 +38,7 @@
for (const GrSurfaceProxyView& target : fTargets) {
if (this == drawingMgr->getLastRenderTask(target.proxy())) {
drawingMgr->setLastRenderTask(target.proxy(), nullptr);
+ target.proxy()->lastRenderTask().release(drawingMgr);
}
}
}
diff --git a/src/gpu/GrSurfaceProxy.h b/src/gpu/GrSurfaceProxy.h
index f30e21d..a7ae7ee 100644
--- a/src/gpu/GrSurfaceProxy.h
+++ b/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 @@
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 @@
// 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)
diff --git a/tests/GrAffinedStorageTest.cpp b/tests/GrAffinedStorageTest.cpp
new file mode 100644
index 0000000..7ef734d
--- /dev/null
+++ b/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));
+}
