Revert "put an arena on GrSurfaceDrawContext"

This reverts commit 5a2de5e72f.

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>

src/gpu/GrDrawOpTest.h

@@ -27,11 +27,10 @@ void GrDrawRandomOp(SkRandom*, GrSurfaceDrawContext*, GrPaint&&);
 /** 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,                                       \
+                             GrRecordingContext* context, int numSamples)
-                             GrSurfaceDrawContext* sdc, int numSamples)
 #define GR_DRAW_OP_TEST_FRIEND(Op)                                                              \
-    friend GrOp::OpOwner Op##__Test(GrPaint&&, SkRandom*,                                       \
+    friend GrOp::OpOwner Op##__Test(GrPaint&& paint, SkRandom* random,                          \
-                                    GrRecordingContext*, GrSurfaceDrawContext*, int)
+                                    GrRecordingContext* context, int numSamples)
 
 /** Helper for op test factories to pick a random stencil state. */
 const GrUserStencilSettings* GrGetRandomStencil(SkRandom* random, GrContext_Base*);

src/gpu/GrDrawingManager.cpp

@@ -659,17 +659,14 @@ void GrDrawingManager::closeActiveOpsTask() {
 }
 
 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,
+    sk_sp<GrOpsTask> opsTask(new GrOpsTask(this, std::move(surfaceView),
-                                           std::move(surfaceView),
+                                           fContext->priv().auditTrail()));
-                                           fContext->priv().auditTrail(),
-                                           std::move(arenas)));
     SkASSERT(this->getLastRenderTask(opsTask->target(0)) == opsTask.get());
 
     if (flushTimeOpsTask) {

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 @@ public:
     void freeGpuResources();
 
     // OpsTasks created at flush time are stored and handled different from the others.
-    sk_sp<GrOpsTask> newOpsTask(GrSurfaceProxyView,
+    sk_sp<GrOpsTask> newOpsTask(GrSurfaceProxyView, bool flushTimeOpsTask);
-                                sk_sp<GrArenas> arenas,
-                                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

src/gpu/GrOpsTask.cpp

@@ -355,13 +355,11 @@ inline void GrOpsTask::OpChain::validate() const {
 
 GrOpsTask::GrOpsTask(GrDrawingManager* drawingMgr,
                      GrSurfaceProxyView view,
-                     GrAuditTrail* auditTrail,
+                     GrAuditTrail* auditTrail)
-                     sk_sp<GrArenas> arenas)
         : 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());

src/gpu/GrOpsTask.h

@@ -32,21 +32,14 @@ class GrClearOp;
 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.
+    // The Arenas must outlive the GrOpsTask, either by preserving the context that owns
-    GrOpsTask(GrDrawingManager*, GrSurfaceProxyView, GrAuditTrail*, sk_sp<GrArenas>);
+    // 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 @@ private:
     // 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

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"

src/gpu/GrSurfaceFillContext.cpp

@@ -342,8 +342,8 @@ GrOpsTask* GrSurfaceFillContext::getOpsTask() {
     SkDEBUGCODE(this->validate();)
 
     if (!fOpsTask || fOpsTask->isClosed()) {
-        sk_sp<GrOpsTask> newOpsTask = this->drawingManager()->newOpsTask(
+        sk_sp<GrOpsTask> newOpsTask = this->drawingManager()->newOpsTask(this->writeSurfaceView(),
-                this->writeSurfaceView(), fArenas, fFlushTimeOpsTask);
+                                                                         fFlushTimeOpsTask);
         if (fOpsTask) {
             this->willReplaceOpsTask(fOpsTask.get(), newOpsTask.get());
         }

src/gpu/GrSurfaceFillContext.h

@@ -195,8 +195,6 @@ public:
     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 @@ private:
     // 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;

src/gpu/ops/GrAtlasTextOp.cpp

@@ -112,12 +112,8 @@ auto GrAtlasTextOp::Geometry::MakeForBlob(const GrAtlasSubRun& subRun,
                                           SkPoint drawOrigin,
                                           SkIRect clipRect,
                                           sk_sp<GrTextBlob> blob,
-                                          const SkPMColor4f& color,
+                                          const SkPMColor4f& color) -> Geometry* {
-                                          SkArenaAlloc* alloc) -> Geometry* {
+    return new Geometry{subRun,
-    // 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,
@@ -526,6 +522,10 @@ GrOp::Owner GrAtlasTextOp::CreateOpTestingOnly(GrSurfaceDrawContext* rtc,
 }
 
 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 @@ GR_DRAW_OP_TEST_DEFINE(GrAtlasTextOp) {
     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

src/gpu/ops/GrAtlasTextOp.h

@@ -27,7 +27,7 @@ public:
     ~GrAtlasTextOp() override {
         for (const Geometry* g = fHead; g != nullptr;) {
             const Geometry* next = g->fNext;
-            g->~Geometry();
+            delete g;
             g = next;
         }
     }
@@ -67,8 +67,7 @@ public:
                                      SkPoint drawOrigin,
                                      SkIRect clipRect,
                                      sk_sp<GrTextBlob> blob,
-                                     const SkPMColor4f& color,
+                                     const SkPMColor4f& color);
-                                     SkArenaAlloc* alloc);
 
         void fillVertexData(void* dst, int offset, int count) const;
 

src/gpu/text/GrTextBlob.cpp

@@ -677,8 +677,7 @@ DirectMaskSubRun::makeAtlasTextOp(const GrClip* clip, const SkMatrixProvider& vi
             drawOrigin,
             clipRect,
             sk_ref_sp<GrTextBlob>(fBlob),
-            drawingColor,
+            drawingColor);
-            sdc->arenaAlloc());
 
     GrRecordingContext* const rContext = sdc->recordingContext();
     GrOp::Owner op = GrOp::Make<GrAtlasTextOp>(rContext,
@@ -971,8 +970,7 @@ TransformedMaskSubRun::makeAtlasTextOp(const GrClip* clip,
             drawOrigin,
             SkIRect::MakeEmpty(),
             sk_ref_sp<GrTextBlob>(fBlob),
-            drawingColor,
+            drawingColor);
-            sdc->arenaAlloc());
 
     GrRecordingContext* const rContext = sdc->recordingContext();
     GrOp::Owner op = GrOp::Make<GrAtlasTextOp>(
@@ -1254,8 +1252,7 @@ SDFTSubRun::makeAtlasTextOp(const GrClip* clip,
             drawOrigin,
             SkIRect::MakeEmpty(),
             sk_ref_sp<GrTextBlob>(fBlob),
-            drawingColor,
+            drawingColor);
-            sdc->arenaAlloc());
 
     GrRecordingContext* const rContext = sdc->recordingContext();
     GrOp::Owner op = GrOp::Make<GrAtlasTextOp>(

tests/OpChainTest.cpp

@@ -205,7 +205,6 @@ DEF_GPUTEST(OpChainTest, reporter, /*ctxInfo*/) {
     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 @@ DEF_GPUTEST(OpChainTest, reporter, /*ctxInfo*/) {
                                           &tracker);
                 GrOpsTask opsTask(drawingMgr,
                                   GrSurfaceProxyView(proxy, kOrigin, writeSwizzle),
-                                  dContext->priv().auditTrail(),
+                                  dContext->priv().auditTrail());
-                                  arenas);
                 // This assumes the particular values of kRanges.
                 std::fill_n(result, result_width(), -1);
                 std::fill_n(validResult, result_width(), -1);

tools/gpu/GrTest.cpp

@@ -53,7 +53,7 @@ int GrResourceCache::countUniqueKeysWithTag(const char* tag) const {
 
 #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 @@ DRAW_OP_TEST_EXTERN(TextureOp);
 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 @@ void GrDrawRandomOp(SkRandom* random, GrSurfaceDrawContext* surfaceDrawContext,
 
     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),
+    auto op = gFactories[index](
-                                random,
+            std::move(paint), random, context, surfaceDrawContext->numSamples());
-                                context,
-                                surfaceDrawContext,
-                                surfaceDrawContext->numSamples());
 
     // Creating a GrAtlasTextOp my not produce an op if for example, it is totally outside the
     // render target context.