Reland "Detach op memory pool from recording context"
This reverts commit ed58654e39df63bd8d58944bea9f65a8ca206986.
Reason for revert: Fix field order in SkDeferredDisplayList to deconstruct
dependent types in the proper order.
Original change's description:
> Revert "Detach op memory pool from recording context"
>
> This reverts commit 6b955167286de1cc0cf215e460389614221903fe.
>
> Reason for revert: breaking some Win10 bots
>
> Original change's description:
> > Detach op memory pool from recording context
> >
> > 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>
>
> TBR=bsalomon@google.com,robertphillips@google.com,michaelludwig@google.com
>
> Change-Id: I942ae1e07fdc63d9311f6ee482bd71beca090502
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/259696
> Reviewed-by: Derek Sollenberger <djsollen@google.com>
> Commit-Queue: Derek Sollenberger <djsollen@google.com>
Change-Id: Ia82fa6e42fc8d75b8aa57e5172894e8dfc7e83d1
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/259816
Reviewed-by: Brian Salomon <bsalomon@google.com>
Commit-Queue: Michael Ludwig <michaelludwig@google.com>
diff --git a/src/gpu/GrContextPriv.cpp b/src/gpu/GrContextPriv.cpp
index b4ae5f6..d209063 100644
--- a/src/gpu/GrContextPriv.cpp
+++ b/src/gpu/GrContextPriv.cpp
@@ -41,10 +41,6 @@
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
index 64522b0..a3ef7d0 100644
--- a/src/gpu/GrContextPriv.h
+++ b/src/gpu/GrContextPriv.h
@@ -59,7 +59,6 @@
// 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
index 083c128..8284a32 100644
--- a/src/gpu/GrDrawingManager.cpp
+++ b/src/gpu/GrDrawingManager.cpp
@@ -581,6 +581,7 @@
renderTask->prePrepare(fContext);
}
+ ddl->fOpMemoryPool = fContext->priv().detachOpMemoryPool();
ddl->fRecordTimeData = fContext->priv().detachRecordTimeAllocator();
fContext->priv().detachProgramInfos(&ddl->fProgramInfos);
@@ -680,7 +681,7 @@
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
index a442830..535ad9b 100644
--- a/src/gpu/GrMemoryPool.h
+++ b/src/gpu/GrMemoryPool.h
@@ -126,9 +126,7 @@
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
index b57066f..ea21b55 100644
--- a/src/gpu/GrOpsTask.cpp
+++ b/src/gpu/GrOpsTask.cpp
@@ -352,11 +352,11 @@
////////////////////////////////////////////////////////////////////////////////
-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 @@
void GrOpsTask::deleteOps() {
for (auto& chain : fOpChains) {
- chain.deleteOps(fOpMemoryPool.get());
+ chain.deleteOps(fOpMemoryPool);
}
fOpChains.reset();
}
@@ -815,7 +815,7 @@
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 @@
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
index a1b2ed1..7d4e15e 100644
--- a/src/gpu/GrOpsTask.h
+++ b/src/gpu/GrOpsTask.h
@@ -38,7 +38,9 @@
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 @@
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
index 18a1114..aa7d1a1 100644
--- a/src/gpu/GrRecordingContext.cpp
+++ b/src/gpu/GrRecordingContext.cpp
@@ -117,23 +117,19 @@
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 @@
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 @@
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 @@
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
index 12d8ce6..35a2eed 100644
--- a/src/gpu/GrRecordingContextPriv.h
+++ b/src/gpu/GrRecordingContextPriv.h
@@ -43,8 +43,8 @@
// 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();