Revert "remove fAllocators from GrOpsTask"
This reverts commit f00d6a8efd723ecccda716d0360df49bdc48feaa.
Reason for revert: In the long run, it seems like this might leak.
Original change's description:
> remove fAllocators from GrOpsTask
>
> Change-Id: I5901f005c2758a92692e5cd70ba46a2b5ad797fd
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/393116
> Commit-Queue: Herb Derby <herb@google.com>
> Reviewed-by: Adlai Holler <adlai@google.com>
> Reviewed-by: Chris Dalton <csmartdalton@google.com>
> Reviewed-by: Robert Phillips <robertphillips@google.com>
TBR=herb@google.com,robertphillips@google.com,csmartdalton@google.com,adlai@google.com
Change-Id: I895e47c999c6961fa14c356d6d5bde80dd86b63e
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/393176
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Herb Derby <herb@google.com>
diff --git a/src/gpu/GrOpsTask.cpp b/src/gpu/GrOpsTask.cpp
index 0db04fc..2d106d1 100644
--- a/src/gpu/GrOpsTask.cpp
+++ b/src/gpu/GrOpsTask.cpp
@@ -363,6 +363,7 @@
, fTargetOrigin(view.origin())
, fArenas{std::move(arenas)}
SkDEBUGCODE(, fNumClips(0)) {
+ fAllocators.push_back(std::make_unique<SkArenaAlloc>(4096));
this->addTarget(drawingMgr, view.detachProxy());
}
@@ -425,6 +426,7 @@
void GrOpsTask::endFlush(GrDrawingManager* drawingMgr) {
fLastClipStackGenID = SK_InvalidUniqueID;
this->deleteOps();
+ fAllocators.reset();
fDeferredProxies.reset();
fSampledProxies.reset();
@@ -671,6 +673,7 @@
void GrOpsTask::reset() {
fDeferredProxies.reset();
fSampledProxies.reset();
+ fAllocators.reset();
fClippedContentBounds = SkIRect::MakeEmpty();
fTotalBounds = SkRect::MakeEmpty();
this->deleteOps();
@@ -686,7 +689,6 @@
if (!opsTask || opsTask->target(0) != this->target(0)) {
break;
}
- SkASSERT(this->fArenas == opsTask->fArenas);
SkASSERT(fTargetSwizzle == opsTask->fTargetSwizzle);
SkASSERT(fTargetOrigin == opsTask->fTargetOrigin);
if (GrLoadOp::kClear == opsTask->fColorLoadOp) {
@@ -735,6 +737,7 @@
fDeferredProxies.reserve_back(addlDeferredProxyCount);
fSampledProxies.reserve_back(addlProxyCount);
fOpChains.reserve_back(addlOpChainCount);
+ fAllocators.reserve_back(opsTasks.count());
for (const auto& opsTask : opsTasks) {
fDeferredProxies.move_back_n(opsTask->fDeferredProxies.count(),
opsTask->fDeferredProxies.data());
@@ -742,6 +745,9 @@
opsTask->fSampledProxies.data());
fOpChains.move_back_n(opsTask->fOpChains.count(),
opsTask->fOpChains.data());
+ SkASSERT(1 == opsTask->fAllocators.count());
+ fAllocators.push_back(std::move(opsTask->fAllocators[0]));
+ opsTask->fAllocators.reset();
opsTask->fDeferredProxies.reset();
opsTask->fSampledProxies.reset();
opsTask->fOpChains.reset();
@@ -963,7 +969,7 @@
while (true) {
OpChain& candidate = fOpChains.fromBack(i);
op = candidate.appendOp(std::move(op), processorAnalysis, dstProxyView, clip, caps,
- fArenas->arenaAlloc(), fAuditTrail);
+ fAllocators.front().get(), fAuditTrail);
if (!op) {
return;
}
@@ -982,7 +988,7 @@
GrOP_INFO("\t\tBackward: FirstOp\n");
}
if (clip) {
- clip = fArenas->arenaAlloc()->make<GrAppliedClip>(std::move(*clip));
+ clip = fAllocators[0]->make<GrAppliedClip>(std::move(*clip));
SkDEBUGCODE(fNumClips++;)
}
fOpChains.emplace_back(std::move(op), processorAnalysis, clip, dstProxyView);
@@ -998,7 +1004,7 @@
int j = i + 1;
while (true) {
OpChain& candidate = fOpChains[j];
- if (candidate.prependChain(&chain, caps, fArenas->arenaAlloc(), fAuditTrail)) {
+ if (candidate.prependChain(&chain, caps, fAllocators.front().get(), 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 967adb1..8ec0595 100644
--- a/src/gpu/GrOpsTask.h
+++ b/src/gpu/GrOpsTask.h
@@ -272,6 +272,9 @@
// For ops/opsTask we have mean: 5 stdDev: 28
SkSTArray<25, OpChain> fOpChains;
+ // MDB TODO: 4096 for the first allocation may be huge overkill. Gather statistics to determine
+ // the correct size.
+ SkSTArray<1, std::unique_ptr<SkArenaAlloc>> fAllocators;
sk_sp<GrArenas> fArenas;
SkDEBUGCODE(int fNumClips;)
diff --git a/tests/GrCCPRTest.cpp b/tests/GrCCPRTest.cpp
index 1a1f82e..f8aae59 100644
--- a/tests/GrCCPRTest.cpp
+++ b/tests/GrCCPRTest.cpp
@@ -169,6 +169,22 @@
void onRun(skiatest::Reporter* reporter, CCPRPathDrawer& ccpr) override {
REPORTER_ASSERT(reporter, SkPathPriv::TestingOnly_unique(fPath));
+ // Ensure paths get unreffed.
+ for (int i = 0; i < 10; ++i) {
+ ccpr.clipFullscreenRect(fPath);
+ }
+ REPORTER_ASSERT(reporter, !SkPathPriv::TestingOnly_unique(fPath));
+ ccpr.flush();
+ REPORTER_ASSERT(reporter, SkPathPriv::TestingOnly_unique(fPath));
+
+ // Ensure clip paths get unreffed.
+ for (int i = 0; i < 10; ++i) {
+ ccpr.clipFullscreenRect(fPath);
+ }
+ REPORTER_ASSERT(reporter, !SkPathPriv::TestingOnly_unique(fPath));
+ ccpr.flush();
+ REPORTER_ASSERT(reporter, SkPathPriv::TestingOnly_unique(fPath));
+
// Ensure paths get unreffed when we delete the context without flushing.
for (int i = 0; i < 10; ++i) {
ccpr.clipFullscreenRect(fPath);
@@ -230,6 +246,26 @@
};
DEF_CCPR_TEST(CCPR_parseEmptyPath)
+class CCPR_unrefPerOpsTaskPathsBeforeOps : public CCPRTest {
+ void onRun(skiatest::Reporter* reporter, CCPRPathDrawer& ccpr) override {
+ REPORTER_ASSERT(reporter, SkPathPriv::TestingOnly_unique(fPath));
+ for (int i = 0; i < 10000; ++i) {
+ // Draw enough paths to make the arena allocator hit the heap.
+ ccpr.clipFullscreenRect(fPath);
+ }
+
+ // Unref the GrCCPerOpsTaskPaths object.
+ auto perOpsTaskPathsMap = ccpr.ccpr()->detachPendingPaths();
+ perOpsTaskPathsMap.clear();
+
+ // Now delete the Op and all its draws.
+ REPORTER_ASSERT(reporter, !SkPathPriv::TestingOnly_unique(fPath));
+ ccpr.flush();
+ REPORTER_ASSERT(reporter, SkPathPriv::TestingOnly_unique(fPath));
+ }
+};
+DEF_CCPR_TEST(CCPR_unrefPerOpsTaskPathsBeforeOps)
+
class CCPRRenderingTest {
public:
void run(skiatest::Reporter* reporter, GrDirectContext* dContext) const {
