Detach op memory pool from recording context

lhkbob · Skia Commit-Bot · commit 6b955167286d · 2019-12-12T15:37:04.000Z
This changes GrOpMemoryPool to no longer extend SkRefCnt, and all usages either are std::unique_ptr for owners, or GrOpMemoryPool* when ownership is held somewhere else. The culmination of this is that DDLs explicitly detach the memory pool from the recording context instead of the GrOpsTask maintaining a strong ref that preserved the memory somewhat sneakily. Change-Id: I33e2caebea70cebe8fd7681207c631feeaf2c703 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/259424 Commit-Queue: Michael Ludwig <michaelludwig@google.com> Reviewed-by: Robert Phillips <robertphillips@google.com> Reviewed-by: Brian Salomon <bsalomon@google.com>
diff --git a/include/private/GrRecordingContext.h b/include/private/GrRecordingContext.h
@@ -54,8 +54,10 @@ class GrRecordingContext : public GrImageContext {
 
     GrDrawingManager* drawingManager();
 
-    sk_sp<GrOpMemoryPool> refOpMemoryPool();
     GrOpMemoryPool* opMemoryPool();
+    // This entry point should only be used for DDL creation where we want the ops' lifetime to
+    // match that of the DDL.
+    std::unique_ptr<GrOpMemoryPool> detachOpMemoryPool();
 
     SkArenaAlloc* recordTimeAllocator();
     // This entry point should only be used for DDL creation where we want the ops' data's lifetime
@@ -148,7 +150,7 @@ class GrRecordingContext : public GrImageContext {
 private:
     std::unique_ptr<GrDrawingManager> fDrawingManager;
     // All the GrOp-derived classes use this pool.
-    sk_sp<GrOpMemoryPool>             fOpMemoryPool;
+    std::unique_ptr<GrOpMemoryPool>   fOpMemoryPool;
     std::unique_ptr<SkArenaAlloc>     fRecordTimeAllocator;
 
     std::unique_ptr<GrStrikeCache>    fStrikeCache;
diff --git a/include/private/SkDeferredDisplayList.h b/include/private/SkDeferredDisplayList.h
@@ -17,6 +17,7 @@ class SkDeferredDisplayListPriv;
 #if SK_SUPPORT_GPU
 #include "include/private/SkTArray.h"
 #include <map>
+class GrOpMemoryPool;
 class GrRenderTask;
 class GrRenderTargetProxy;
 struct GrCCPerOpsTaskPaths;
@@ -79,6 +80,7 @@ class SkDeferredDisplayList {
 
     SkTArray<sk_sp<GrRenderTask>>   fRenderTasks;
     PendingPathsMap                 fPendingPaths;  // This is the path data from CCPR.
+    std::unique_ptr<GrOpMemoryPool> fOpMemoryPool;
     std::unique_ptr<SkArenaAlloc>   fRecordTimeData;
 
     // The program infos should be stored in 'fRecordTimeData' so do not need to be ref counted
diff --git a/src/atlastext/SkAtlasTextTarget.cpp b/src/atlastext/SkAtlasTextTarget.cpp
@@ -85,9 +85,7 @@ class SkInternalAtlasTextTarget : public GrTextTarget, public SkAtlasTextTarget
                               void* handle)
             : GrTextTarget(width, height, kColorInfo)
             , SkAtlasTextTarget(std::move(context), width, height, handle)
-            , fGlyphPainter(kProps, kColorInfo) {
-        fOpMemoryPool = fContext->internal().grContext()->priv().refOpMemoryPool();
-    }
+            , fGlyphPainter(kProps, kColorInfo) {}
 
     ~SkInternalAtlasTextTarget() override {
         this->deleteOps();
@@ -124,11 +122,14 @@ class SkInternalAtlasTextTarget : public GrTextTarget, public SkAtlasTextTarget
 private:
     void deleteOps();
 
+    GrOpMemoryPool* opMemoryPool() {
+        return fContext->internal().grContext()->priv().opMemoryPool();
+    }
+
     uint32_t fColor;
     using SkAtlasTextTarget::fWidth;
     using SkAtlasTextTarget::fHeight;
     SkTArray<std::unique_ptr<GrAtlasTextOp>, true> fOps;
-    sk_sp<GrOpMemoryPool> fOpMemoryPool;
     SkGlyphRunListPainter fGlyphPainter;
 };
 
@@ -175,10 +176,12 @@ void SkInternalAtlasTextTarget::addDrawOp(const GrClip& clip, std::unique_ptr<Gr
     const GrCaps& caps = *this->context()->internal().grContext()->priv().caps();
     op->finalizeForTextTarget(fColor, caps);
     int n = SkTMin(kMaxBatchLookBack, fOps.count());
+
+    GrOpMemoryPool* pool = this->opMemoryPool();
     for (int i = 0; i < n; ++i) {
         GrAtlasTextOp* other = fOps.fromBack(i).get();
         if (other->combineIfPossible(op.get(), caps) == GrOp::CombineResult::kMerged) {
-            fOpMemoryPool->release(std::move(op));
+            pool->release(std::move(op));
             return;
         }
         if (GrRectsOverlap(op->bounds(), other->bounds())) {
@@ -189,9 +192,10 @@ void SkInternalAtlasTextTarget::addDrawOp(const GrClip& clip, std::unique_ptr<Gr
 }
 
 void SkInternalAtlasTextTarget::deleteOps() {
+    GrOpMemoryPool* pool = this->opMemoryPool();
     for (int i = 0; i < fOps.count(); ++i) {
         if (fOps[i]) {
-            fOpMemoryPool->release(std::move(fOps[i]));
+            pool->release(std::move(fOps[i]));
         }
     }
     fOps.reset();
diff --git a/src/core/SkDeferredDisplayList.cpp b/src/core/SkDeferredDisplayList.cpp
@@ -13,6 +13,7 @@
 class SkSurfaceCharacterization;
 
 #if SK_SUPPORT_GPU
+#include "src/gpu/GrMemoryPool.h"
 #include "src/gpu/GrRenderTask.h"
 #include "src/gpu/ccpr/GrCCPerOpsTaskPaths.h"
 #endif
diff --git a/src/gpu/GrContextPriv.cpp b/src/gpu/GrContextPriv.cpp
@@ -41,10 +41,6 @@ sk_sp<GrSkSLFPFactoryCache> GrContextPriv::fpFactoryCache() {
     return fContext->fpFactoryCache();
 }
 
-sk_sp<GrOpMemoryPool> GrContextPriv::refOpMemoryPool() {
-    return fContext->refOpMemoryPool();
-}
-
 void GrContextPriv::addOnFlushCallbackObject(GrOnFlushCallbackObject* onFlushCBObject) {
     fContext->addOnFlushCallbackObject(onFlushCBObject);
 }
diff --git a/src/gpu/GrContextPriv.h b/src/gpu/GrContextPriv.h
@@ -59,7 +59,6 @@ class GrContextPriv {
     // from GrRecordingContext
     GrDrawingManager* drawingManager() { return fContext->drawingManager(); }
 
-    sk_sp<GrOpMemoryPool> refOpMemoryPool();
     GrOpMemoryPool* opMemoryPool() { return fContext->opMemoryPool(); }
 
     GrStrikeCache* getGrStrikeCache() { return fContext->getGrStrikeCache(); }
diff --git a/src/gpu/GrDrawingManager.cpp b/src/gpu/GrDrawingManager.cpp
@@ -581,6 +581,7 @@ void GrDrawingManager::moveRenderTasksToDDL(SkDeferredDisplayList* ddl) {
         renderTask->prePrepare(fContext);
     }
 
+    ddl->fOpMemoryPool = fContext->priv().detachOpMemoryPool();
     ddl->fRecordTimeData = fContext->priv().detachRecordTimeAllocator();
 
     fContext->priv().detachProgramInfos(&ddl->fProgramInfos);
@@ -680,7 +681,7 @@ sk_sp<GrOpsTask> GrDrawingManager::newOpsTask(GrSurfaceProxyView surfaceView,
     GrSurfaceProxy* proxy = surfaceView.proxy();
     this->closeRenderTasksForNewRenderTask(proxy);
 
-    sk_sp<GrOpsTask> opsTask(new GrOpsTask(fContext->priv().refOpMemoryPool(),
+    sk_sp<GrOpsTask> opsTask(new GrOpsTask(fContext->priv().opMemoryPool(),
                                            std::move(surfaceView), fContext->priv().auditTrail()));
     SkASSERT(proxy->getLastRenderTask() == opsTask.get());
 
diff --git a/src/gpu/GrMemoryPool.h b/src/gpu/GrMemoryPool.h
@@ -126,9 +126,7 @@ class GrMemoryPool {
 
 class GrOp;
 
-// DDL TODO: for the DLL use case this could probably be the non-intrinsic-based style of
-// ref counting
-class GrOpMemoryPool : public SkRefCnt {
+class GrOpMemoryPool {
 public:
     GrOpMemoryPool(size_t preallocSize, size_t minAllocSize)
             : fMemoryPool(preallocSize, minAllocSize) {
diff --git a/src/gpu/GrOpsTask.cpp b/src/gpu/GrOpsTask.cpp
@@ -352,11 +352,11 @@ inline void GrOpsTask::OpChain::validate() const {
 
 ////////////////////////////////////////////////////////////////////////////////
 
-GrOpsTask::GrOpsTask(sk_sp<GrOpMemoryPool> opMemoryPool,
+GrOpsTask::GrOpsTask(GrOpMemoryPool* opMemoryPool,
                      GrSurfaceProxyView view,
                      GrAuditTrail* auditTrail)
         : GrRenderTask(std::move(view))
-        , fOpMemoryPool(std::move(opMemoryPool))
+        , fOpMemoryPool(opMemoryPool)
         , fAuditTrail(auditTrail)
         , fLastClipStackGenID(SK_InvalidUniqueID)
         SkDEBUGCODE(, fNumClips(0)) {
@@ -366,7 +366,7 @@ GrOpsTask::GrOpsTask(sk_sp<GrOpMemoryPool> opMemoryPool,
 
 void GrOpsTask::deleteOps() {
     for (auto& chain : fOpChains) {
-        chain.deleteOps(fOpMemoryPool.get());
+        chain.deleteOps(fOpMemoryPool);
     }
     fOpChains.reset();
 }
@@ -815,7 +815,7 @@ void GrOpsTask::recordOp(
         while (true) {
             OpChain& candidate = fOpChains.fromBack(i);
             op = candidate.appendOp(std::move(op), processorAnalysis, dstProxyView, clip, caps,
-                                    fOpMemoryPool.get(), fAuditTrail);
+                                    fOpMemoryPool, fAuditTrail);
             if (!op) {
                 return;
             }
@@ -850,7 +850,7 @@ void GrOpsTask::forwardCombine(const GrCaps& caps) {
         int j = i + 1;
         while (true) {
             OpChain& candidate = fOpChains[j];
-            if (candidate.prependChain(&chain, caps, fOpMemoryPool.get(), fAuditTrail)) {
+            if (candidate.prependChain(&chain, caps, fOpMemoryPool, fAuditTrail)) {
                 break;
             }
             // Stop traversing if we would cause a painter's order violation.
diff --git a/src/gpu/GrOpsTask.h b/src/gpu/GrOpsTask.h
@@ -38,7 +38,9 @@ class GrOpsTask : public GrRenderTask {
     using DstProxyView = GrXferProcessor::DstProxyView;
 
 public:
-    GrOpsTask(sk_sp<GrOpMemoryPool>, GrSurfaceProxyView, GrAuditTrail*);
+    // The GrOpMemoryPool must outlive the GrOpsTask, either by preserving the context that owns
+    // the pool, or by moving the pool to the DDL that takes over the GrOpsTask.
+    GrOpsTask(GrOpMemoryPool*, GrSurfaceProxyView, GrAuditTrail*);
     ~GrOpsTask() override;
 
     GrOpsTask* asOpsTask() override { return this; }
@@ -275,10 +277,10 @@ class GrOpsTask : public GrRenderTask {
     friend class GrRenderTargetContext;
 
     // This is a backpointer to the GrOpMemoryPool that holds the memory for this GrOpsTask's ops.
-    // In the DDL case, these back pointers keep the DDL's GrOpMemoryPool alive as long as its
-    // constituent GrOpsTask survives.
-    sk_sp<GrOpMemoryPool> fOpMemoryPool;
-    GrAuditTrail* fAuditTrail;
+    // In the DDL case, the GrOpMemoryPool must have been detached from the original recording
+    // context and moved into the owning DDL.
+    GrOpMemoryPool* fOpMemoryPool;
+    GrAuditTrail*   fAuditTrail;
 
     GrLoadOp fColorLoadOp = GrLoadOp::kLoad;
     SkPMColor4f fLoadClearColor = SK_PMColor4fTRANSPARENT;
diff --git a/src/gpu/GrRecordingContext.cpp b/src/gpu/GrRecordingContext.cpp
@@ -117,23 +117,19 @@ GrDrawingManager* GrRecordingContext::drawingManager() {
     return fDrawingManager.get();
 }
 
-// This entry point exists bc the GrOpsTask (and SkAtlasTextTarget) take refs on the memory pool.
-// Ostensibly, this is to keep the op's data alive in DDL mode but the back pointer is also
-// used for deletion.
-sk_sp<GrOpMemoryPool> GrRecordingContext::refOpMemoryPool() {
+GrOpMemoryPool* GrRecordingContext::opMemoryPool() {
     if (!fOpMemoryPool) {
         // DDL TODO: should the size of the memory pool be decreased in DDL mode? CPU-side memory
         // consumed in DDL mode vs. normal mode for a single skp might be a good metric of wasted
         // memory.
-        fOpMemoryPool = sk_sp<GrOpMemoryPool>(new GrOpMemoryPool(16384, 16384));
+        fOpMemoryPool = std::make_unique<GrOpMemoryPool>(16384, 16384);
     }
 
-    SkASSERT(fOpMemoryPool);
-    return fOpMemoryPool;
+    return fOpMemoryPool.get();
 }
 
-GrOpMemoryPool* GrRecordingContext::opMemoryPool() {
-    return this->refOpMemoryPool().get();
+std::unique_ptr<GrOpMemoryPool> GrRecordingContext::detachOpMemoryPool() {
+    return std::move(fOpMemoryPool);
 }
 
 // Stored in this arena:
@@ -142,11 +138,9 @@ GrOpMemoryPool* GrRecordingContext::opMemoryPool() {
 SkArenaAlloc* GrRecordingContext::recordTimeAllocator() {
     if (!fRecordTimeAllocator) {
         // TODO: empirically determine a better number for SkArenaAlloc's firstHeapAllocation param
-        fRecordTimeAllocator = std::unique_ptr<SkArenaAlloc>(
-                                                    new SkArenaAlloc(sizeof(GrPipeline) * 100));
+        fRecordTimeAllocator = std::make_unique<SkArenaAlloc>(sizeof(GrPipeline) * 100);
     }
 
-    SkASSERT(fRecordTimeAllocator);
     return fRecordTimeAllocator.get();
 }
 
@@ -325,6 +319,10 @@ sk_sp<const GrCaps> GrRecordingContextPriv::refCaps() const {
     return fContext->refCaps();
 }
 
+std::unique_ptr<GrOpMemoryPool> GrRecordingContextPriv::detachOpMemoryPool() {
+    return fContext->detachOpMemoryPool();
+}
+
 std::unique_ptr<SkArenaAlloc> GrRecordingContextPriv::detachRecordTimeAllocator() {
     return fContext->detachRecordTimeAllocator();
 }
@@ -333,10 +331,6 @@ sk_sp<GrSkSLFPFactoryCache> GrRecordingContextPriv::fpFactoryCache() {
     return fContext->fpFactoryCache();
 }
 
-sk_sp<GrOpMemoryPool> GrRecordingContextPriv::refOpMemoryPool() {
-    return fContext->refOpMemoryPool();
-}
-
 void GrRecordingContextPriv::addOnFlushCallbackObject(GrOnFlushCallbackObject* onFlushCBObject) {
     fContext->addOnFlushCallbackObject(onFlushCBObject);
 }
diff --git a/src/gpu/GrRecordingContextPriv.h b/src/gpu/GrRecordingContextPriv.h
@@ -43,8 +43,8 @@ class GrRecordingContextPriv {
     // from GrRecordingContext
     GrDrawingManager* drawingManager() { return fContext->drawingManager(); }
 
-    sk_sp<GrOpMemoryPool> refOpMemoryPool();
     GrOpMemoryPool* opMemoryPool() { return fContext->opMemoryPool(); }
+    std::unique_ptr<GrOpMemoryPool> detachOpMemoryPool();
 
     SkArenaAlloc* recordTimeAllocator() { return fContext->recordTimeAllocator(); }
     std::unique_ptr<SkArenaAlloc> detachRecordTimeAllocator();
diff --git a/tests/OpChainTest.cpp b/tests/OpChainTest.cpp
@@ -210,7 +210,7 @@ DEF_GPUTEST(OpChainTest, reporter, /*ctxInfo*/) {
                 GrOpFlushState flushState(context->priv().getGpu(),
                                           context->priv().resourceProvider(),
                                           &tracker);
-                GrOpsTask opsTask(context->priv().refOpMemoryPool(),
+                GrOpsTask opsTask(context->priv().opMemoryPool(),
                                   GrSurfaceProxyView(proxy, kOrigin, outSwizzle),
                                   context->priv().auditTrail());
                 // This assumes the particular values of kRanges.

Original file line number	Diff line number	Diff line change
`@@ -41,10 +41,6 @@ sk_sp<GrSkSLFPFactoryCache> GrContextPriv::fpFactoryCache() {`
`41`	`41`	`return fContext->fpFactoryCache();`
`42`	`42`	`}`
`43`	`43`
`44`		`-sk_sp<GrOpMemoryPool> GrContextPriv::refOpMemoryPool() {`
`45`		`- return fContext->refOpMemoryPool();`
`46`		`-}`
`47`		`-`
`48`	`44`	`void GrContextPriv::addOnFlushCallbackObject(GrOnFlushCallbackObject* onFlushCBObject) {`
`49`	`45`	`fContext->addOnFlushCallbackObject(onFlushCBObject);`
`50`	`46`	`}`