Revert "put an arena on GrSurfaceDrawContext"
This reverts commit 5a2de5e72f24d1bfbdc532252be3dcdefa7b75a2.
Reason for revert: Upon further investigation this still leaks
Original change's description:
> put an arena on GrSurfaceDrawContext
>
> This is part one of two CLs. In this CL, I put a
> sk_sp<GrArenas> on GrSurfaceFillContext where GrArenas wraps
> a SkArenaAlloc to add ref counting. Creating
> a GrOpsTask shares the GrArenas with the ops task. New plumbing
> was added to GR_DRAW_OP_TEST_DEFINE to allow a proper
> GrSurfaceDrawContext to be passed to GrAtlasTextOp's
> GR_DRAW_OP_TEST_DEFINE so the arena will have a proper lifetime.
>
> The second CL will work on replacing GrOpsTask's fAllocators
> system with the shared arena.
>
> Change-Id: Ife3be0ab265441cbffab360f2808f5eed86db8b3
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/392936
> Reviewed-by: Brian Salomon <bsalomon@google.com>
> Commit-Queue: Herb Derby <herb@google.com>
TBR=bsalomon@google.com,herb@google.com
Change-Id: I9ca5c8b1e16b468003788cd3126eda1d40ff93ed
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/393177
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Herb Derby <herb@google.com>
diff --git a/src/gpu/GrDrawOpTest.h b/src/gpu/GrDrawOpTest.h
index a0d0b6c..23f744e 100644
--- a/src/gpu/GrDrawOpTest.h
+++ b/src/gpu/GrDrawOpTest.h
@@ -26,12 +26,11 @@
/** GrDrawOp subclasses should define test factory functions using this macro. */
#define GR_DRAW_OP_TEST_DEFINE(Op) \
- GrOp::Owner Op##__Test(GrPaint&& paint, SkRandom* random, \
- GrRecordingContext* context, \
- GrSurfaceDrawContext* sdc, int numSamples)
+ GrOp::Owner Op##__Test(GrPaint&& paint, SkRandom* random, \
+ GrRecordingContext* context, int numSamples)
#define GR_DRAW_OP_TEST_FRIEND(Op) \
- friend GrOp::OpOwner Op##__Test(GrPaint&&, SkRandom*, \
- GrRecordingContext*, GrSurfaceDrawContext*, int)
+ friend GrOp::OpOwner Op##__Test(GrPaint&& paint, SkRandom* random, \
+ GrRecordingContext* context, int numSamples)
/** Helper for op test factories to pick a random stencil state. */
const GrUserStencilSettings* GrGetRandomStencil(SkRandom* random, GrContext_Base*);
diff --git a/src/gpu/GrDrawingManager.cpp b/src/gpu/GrDrawingManager.cpp
index 7b9f618..8aec8f3 100644
--- a/src/gpu/GrDrawingManager.cpp
+++ b/src/gpu/GrDrawingManager.cpp
@@ -659,17 +659,14 @@
}
sk_sp<GrOpsTask> GrDrawingManager::newOpsTask(GrSurfaceProxyView surfaceView,
- sk_sp<GrArenas> arenas,
bool flushTimeOpsTask) {
SkDEBUGCODE(this->validate());
SkASSERT(fContext);
this->closeActiveOpsTask();
- sk_sp<GrOpsTask> opsTask(new GrOpsTask(this,
- std::move(surfaceView),
- fContext->priv().auditTrail(),
- std::move(arenas)));
+ sk_sp<GrOpsTask> opsTask(new GrOpsTask(this, std::move(surfaceView),
+ fContext->priv().auditTrail()));
SkASSERT(this->getLastRenderTask(opsTask->target(0)) == opsTask.get());
if (flushTimeOpsTask) {
diff --git a/src/gpu/GrDrawingManager.h b/src/gpu/GrDrawingManager.h
index 8eac15a..1d91b24 100644
--- a/src/gpu/GrDrawingManager.h
+++ b/src/gpu/GrDrawingManager.h
@@ -23,7 +23,6 @@
// Enabling this will print out which path renderers are being chosen
#define GR_PATH_RENDERER_SPEW 0
-class GrArenas;
class GrCoverageCountingPathRenderer;
class GrGpuBuffer;
class GrOnFlushCallbackObject;
@@ -47,9 +46,7 @@
void freeGpuResources();
// OpsTasks created at flush time are stored and handled different from the others.
- sk_sp<GrOpsTask> newOpsTask(GrSurfaceProxyView,
- sk_sp<GrArenas> arenas,
- bool flushTimeOpsTask);
+ sk_sp<GrOpsTask> newOpsTask(GrSurfaceProxyView, bool flushTimeOpsTask);
// Create a render task that can resolve MSAA and/or regenerate mipmap levels on proxies. This
// method will only add the new render task to the list. It is up to the caller to call
diff --git a/src/gpu/GrOpsTask.cpp b/src/gpu/GrOpsTask.cpp
index 2d106d1..cc7c8f3 100644
--- a/src/gpu/GrOpsTask.cpp
+++ b/src/gpu/GrOpsTask.cpp
@@ -355,13 +355,11 @@
GrOpsTask::GrOpsTask(GrDrawingManager* drawingMgr,
GrSurfaceProxyView view,
- GrAuditTrail* auditTrail,
- sk_sp<GrArenas> arenas)
+ GrAuditTrail* auditTrail)
: GrRenderTask()
, fAuditTrail(auditTrail)
, fTargetSwizzle(view.swizzle())
, fTargetOrigin(view.origin())
- , fArenas{std::move(arenas)}
SkDEBUGCODE(, fNumClips(0)) {
fAllocators.push_back(std::make_unique<SkArenaAlloc>(4096));
this->addTarget(drawingMgr, view.detachProxy());
diff --git a/src/gpu/GrOpsTask.h b/src/gpu/GrOpsTask.h
index 8ec0595..8b29ece 100644
--- a/src/gpu/GrOpsTask.h
+++ b/src/gpu/GrOpsTask.h
@@ -32,21 +32,14 @@
class GrGpuBuffer;
class GrRenderTargetProxy;
-class GrArenas : public SkNVRefCnt<GrArenas> {
-public:
- SkArenaAlloc* arenaAlloc() { return &fArenaAlloc; }
-
-private:
- SkArenaAlloc fArenaAlloc{1024};
-};
-
class GrOpsTask : public GrRenderTask {
private:
using DstProxyView = GrXferProcessor::DstProxyView;
public:
- // Manage the arenas life time by maintaining are reference to it.
- GrOpsTask(GrDrawingManager*, GrSurfaceProxyView, GrAuditTrail*, sk_sp<GrArenas>);
+ // The Arenas 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(GrDrawingManager*, GrSurfaceProxyView, GrAuditTrail*);
~GrOpsTask() override;
GrOpsTask* asOpsTask() override { return this; }
@@ -275,7 +268,6 @@
// 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;)
// TODO: We could look into this being a set if we find we're adding a lot of duplicates that is
diff --git a/src/gpu/GrSurfaceContext.h b/src/gpu/GrSurfaceContext.h
index 189e732..deb2242 100644
--- a/src/gpu/GrSurfaceContext.h
+++ b/src/gpu/GrSurfaceContext.h
@@ -17,7 +17,6 @@
#include "src/gpu/GrColorInfo.h"
#include "src/gpu/GrDataUtils.h"
#include "src/gpu/GrImageInfo.h"
-#include "src/gpu/GrOpsTask.h"
#include "src/gpu/GrPixmap.h"
#include "src/gpu/GrRenderTask.h"
#include "src/gpu/GrSurfaceProxy.h"
diff --git a/src/gpu/GrSurfaceFillContext.cpp b/src/gpu/GrSurfaceFillContext.cpp
index 9ef6889..49bf84d 100644
--- a/src/gpu/GrSurfaceFillContext.cpp
+++ b/src/gpu/GrSurfaceFillContext.cpp
@@ -342,8 +342,8 @@
SkDEBUGCODE(this->validate();)
if (!fOpsTask || fOpsTask->isClosed()) {
- sk_sp<GrOpsTask> newOpsTask = this->drawingManager()->newOpsTask(
- this->writeSurfaceView(), fArenas, fFlushTimeOpsTask);
+ sk_sp<GrOpsTask> newOpsTask = this->drawingManager()->newOpsTask(this->writeSurfaceView(),
+ fFlushTimeOpsTask);
if (fOpsTask) {
this->willReplaceOpsTask(fOpsTask.get(), newOpsTask.get());
}
diff --git a/src/gpu/GrSurfaceFillContext.h b/src/gpu/GrSurfaceFillContext.h
index b8763f0..d7e4b5c 100644
--- a/src/gpu/GrSurfaceFillContext.h
+++ b/src/gpu/GrSurfaceFillContext.h
@@ -195,8 +195,6 @@
bool wrapsVkSecondaryCB() const { return this->asRenderTargetProxy()->wrapsVkSecondaryCB(); }
GrMipmapped mipmapped() const;
- SkArenaAlloc* arenaAlloc() { return fArenas->arenaAlloc(); }
-
#if GR_TEST_UTILS
GrOpsTask* testingOnly_PeekLastOpsTask() { return fOpsTask.get(); }
#endif
@@ -243,9 +241,6 @@
// reason, the GrOpsTask should only ever be accessed via 'getOpsTask'.
sk_sp<GrOpsTask> fOpsTask;
- // The arenas shared by the OpsTask.
- sk_sp<GrArenas> fArenas = sk_make_sp<GrArenas>();
-
bool fFlushTimeOpsTask;
using INHERITED = GrSurfaceContext;
diff --git a/src/gpu/ops/GrAtlasTextOp.cpp b/src/gpu/ops/GrAtlasTextOp.cpp
index c528362..dcaf5e3 100644
--- a/src/gpu/ops/GrAtlasTextOp.cpp
+++ b/src/gpu/ops/GrAtlasTextOp.cpp
@@ -112,18 +112,14 @@
SkPoint drawOrigin,
SkIRect clipRect,
sk_sp<GrTextBlob> blob,
- const SkPMColor4f& color,
- SkArenaAlloc* alloc) -> Geometry* {
- // Bypass the automatic dtor behavior in SkArenaAlloc. I'm leaving this up to the Op to run
- // all geometry dtors for now.
- void* geo = alloc->makeBytesAlignedTo(sizeof(Geometry), alignof(Geometry));
- return new(geo) Geometry{subRun,
- drawMatrix,
- drawOrigin,
- clipRect,
- std::move(blob),
- nullptr,
- color};
+ const SkPMColor4f& color) -> Geometry* {
+ return new Geometry{subRun,
+ drawMatrix,
+ drawOrigin,
+ clipRect,
+ std::move(blob),
+ nullptr,
+ color};
}
void GrAtlasTextOp::Geometry::fillVertexData(void *dst, int offset, int count) const {
@@ -526,6 +522,10 @@
}
GR_DRAW_OP_TEST_DEFINE(GrAtlasTextOp) {
+ // Setup dummy SkPaint / GrPaint / GrSurfaceDrawContext
+ auto rtc = GrSurfaceDrawContext::Make(
+ context, GrColorType::kRGBA_8888, nullptr, SkBackingFit::kApprox, {1024, 1024});
+
SkSimpleMatrixProvider matrixProvider(GrTest::TestMatrixInvertible(random));
SkPaint skPaint;
@@ -548,7 +548,8 @@
int xInt = (random->nextU() % kMaxTrans) * xPos;
int yInt = (random->nextU() % kMaxTrans) * yPos;
- return GrAtlasTextOp::CreateOpTestingOnly(sdc, skPaint, font, matrixProvider, text, xInt, yInt);
+ return GrAtlasTextOp::CreateOpTestingOnly(
+ rtc.get(), skPaint, font, matrixProvider, text, xInt, yInt);
}
#endif
diff --git a/src/gpu/ops/GrAtlasTextOp.h b/src/gpu/ops/GrAtlasTextOp.h
index 0a8710b..17e5a97 100644
--- a/src/gpu/ops/GrAtlasTextOp.h
+++ b/src/gpu/ops/GrAtlasTextOp.h
@@ -27,7 +27,7 @@
~GrAtlasTextOp() override {
for (const Geometry* g = fHead; g != nullptr;) {
const Geometry* next = g->fNext;
- g->~Geometry();
+ delete g;
g = next;
}
}
@@ -67,8 +67,7 @@
SkPoint drawOrigin,
SkIRect clipRect,
sk_sp<GrTextBlob> blob,
- const SkPMColor4f& color,
- SkArenaAlloc* alloc);
+ const SkPMColor4f& color);
void fillVertexData(void* dst, int offset, int count) const;
diff --git a/src/gpu/text/GrTextBlob.cpp b/src/gpu/text/GrTextBlob.cpp
index 92d5071..a272251 100644
--- a/src/gpu/text/GrTextBlob.cpp
+++ b/src/gpu/text/GrTextBlob.cpp
@@ -677,8 +677,7 @@
drawOrigin,
clipRect,
sk_ref_sp<GrTextBlob>(fBlob),
- drawingColor,
- sdc->arenaAlloc());
+ drawingColor);
GrRecordingContext* const rContext = sdc->recordingContext();
GrOp::Owner op = GrOp::Make<GrAtlasTextOp>(rContext,
@@ -971,8 +970,7 @@
drawOrigin,
SkIRect::MakeEmpty(),
sk_ref_sp<GrTextBlob>(fBlob),
- drawingColor,
- sdc->arenaAlloc());
+ drawingColor);
GrRecordingContext* const rContext = sdc->recordingContext();
GrOp::Owner op = GrOp::Make<GrAtlasTextOp>(
@@ -1254,8 +1252,7 @@
drawOrigin,
SkIRect::MakeEmpty(),
sk_ref_sp<GrTextBlob>(fBlob),
- drawingColor,
- sdc->arenaAlloc());
+ drawingColor);
GrRecordingContext* const rContext = sdc->recordingContext();
GrOp::Owner op = GrOp::Make<GrAtlasTextOp>(
diff --git a/tests/OpChainTest.cpp b/tests/OpChainTest.cpp
index 2761f21..df9e535 100644
--- a/tests/OpChainTest.cpp
+++ b/tests/OpChainTest.cpp
@@ -205,7 +205,6 @@
bool repeat = false;
Combinable combinable;
GrDrawingManager* drawingMgr = dContext->priv().drawingManager();
- sk_sp<GrArenas> arenas = sk_make_sp<GrArenas>();
for (int p = 0; p < kNumPermutations; ++p) {
for (int i = 0; i < kNumOps - 2 && !repeat; ++i) {
// The current implementation of nextULessThan() is biased. :(
@@ -222,8 +221,7 @@
&tracker);
GrOpsTask opsTask(drawingMgr,
GrSurfaceProxyView(proxy, kOrigin, writeSwizzle),
- dContext->priv().auditTrail(),
- arenas);
+ dContext->priv().auditTrail());
// This assumes the particular values of kRanges.
std::fill_n(result, result_width(), -1);
std::fill_n(validResult, result_width(), -1);
diff --git a/tools/gpu/GrTest.cpp b/tools/gpu/GrTest.cpp
index ac03100..85f25cb 100644
--- a/tools/gpu/GrTest.cpp
+++ b/tools/gpu/GrTest.cpp
@@ -53,7 +53,7 @@
#define DRAW_OP_TEST_EXTERN(Op) \
extern GrOp::Owner Op##__Test(GrPaint&&, SkRandom*, \
- GrRecordingContext*, GrSurfaceDrawContext*, int)
+ GrRecordingContext*, int numSamples)
#define DRAW_OP_TEST_ENTRY(Op) Op##__Test
DRAW_OP_TEST_EXTERN(AAConvexPathOp);
@@ -81,7 +81,7 @@
void GrDrawRandomOp(SkRandom* random, GrSurfaceDrawContext* surfaceDrawContext, GrPaint&& paint) {
auto context = surfaceDrawContext->recordingContext();
using MakeDrawOpFn = GrOp::Owner (GrPaint&&, SkRandom*,
- GrRecordingContext*, GrSurfaceDrawContext*, int numSamples);
+ GrRecordingContext*, int numSamples);
static constexpr MakeDrawOpFn* gFactories[] = {
DRAW_OP_TEST_ENTRY(AAConvexPathOp),
DRAW_OP_TEST_ENTRY(AAFlatteningConvexPathOp),
@@ -108,11 +108,8 @@
static constexpr size_t kTotal = SK_ARRAY_COUNT(gFactories);
uint32_t index = random->nextULessThan(static_cast<uint32_t>(kTotal));
- auto op = gFactories[index](std::move(paint),
- random,
- context,
- surfaceDrawContext,
- surfaceDrawContext->numSamples());
+ auto op = gFactories[index](
+ std::move(paint), random, context, surfaceDrawContext->numSamples());
// Creating a GrAtlasTextOp my not produce an op if for example, it is totally outside the
// render target context.
