Revert "Add storage on the surface for its last render task"
This reverts commit ca5b36c474476c20805fd643a1abd1081b469be5.
Reason for revert: This may be blocking the Chrome roll
Original change's description:
> 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>
TBR=robertphillips@google.com,adlai@google.com
Change-Id: Id418d042d1123d946cd99b7b1ba438211cb628ec
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: skia:10372
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/299763
Reviewed-by: Robert Phillips <robertphillips@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
diff --git a/gn/gpu.gni b/gn/gpu.gni
index c074c20..196b054 100644
--- a/gn/gpu.gni
+++ b/gn/gpu.gni
@@ -31,7 +31,6 @@
"$_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",
@@ -110,6 +109,7 @@
"$_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 c6ec1b9..70ef573 100644
--- a/gn/tests.gni
+++ b/gn/tests.gni
@@ -99,7 +99,6 @@
"$_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
deleted file mode 100644
index b89c478..0000000
--- a/src/gpu/GrAffinedStorage.h
+++ /dev/null
@@ -1,57 +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 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 c8af207..a19518f 100644
--- a/src/gpu/GrDrawingManager.cpp
+++ b/src/gpu/GrDrawingManager.cpp
@@ -589,12 +589,6 @@
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);
@@ -604,12 +598,6 @@
}
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 fcdf3ec..62b3ed3 100644
--- a/src/gpu/GrDrawingManager.h
+++ b/src/gpu/GrDrawingManager.h
@@ -13,6 +13,7 @@
#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"
@@ -248,17 +249,13 @@
// Note: we do not expect a whole lot of these per flush
SkTHashMap<uint32_t, GrRenderTargetProxy*> fDDLTargets;
- struct CheapHash {
- uint32_t operator()(uint32_t value) {
- return SkChecksum::CheapMix(value);
+ struct SurfaceIDKeyTraits {
+ static uint32_t GetInvalidKey() {
+ return GrSurfaceProxy::UniqueID::InvalidID().asUInt();
}
};
- // 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;
-
+ GrHashMapWithCache<uint32_t, GrRenderTask*, SurfaceIDKeyTraits, GrCheapHash> fLastRenderTasks;
};
#endif
diff --git a/src/gpu/GrHashMapWithCache.h b/src/gpu/GrHashMapWithCache.h
new file mode 100644
index 0000000..8a24ecc
--- /dev/null
+++ b/src/gpu/GrHashMapWithCache.h
@@ -0,0 +1,84 @@
+/*
+ * 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 33ee510..7b294a4 100644
--- a/src/gpu/GrRenderTask.cpp
+++ b/src/gpu/GrRenderTask.cpp
@@ -38,7 +38,6 @@
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 a7ae7ee..f30e21d 100644
--- a/src/gpu/GrSurfaceProxy.h
+++ b/src/gpu/GrSurfaceProxy.h
@@ -11,7 +11,6 @@
#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"
@@ -19,7 +18,6 @@
class GrCaps;
class GrContext_Base;
-class GrDrawingManager;
class GrOpsTask;
class GrRecordingContext;
class GrRenderTargetProxy;
@@ -318,12 +316,6 @@
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;
@@ -439,8 +431,6 @@
// 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
deleted file mode 100644
index 7ef734d..0000000
--- a/tests/GrAffinedStorageTest.cpp
+++ /dev/null
@@ -1,28 +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.
- */
-
-#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));
-}
