From 27ce7df01360a8960b4a78e89286a6aa1b2f7375 Mon Sep 17 00:00:00 2001 From: Herb Derby Date: Tue, 6 Apr 2021 20:15:50 +0000 Subject: [PATCH] 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 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 > Commit-Queue: Herb Derby 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 Commit-Queue: Herb Derby --- src/gpu/GrDrawOpTest.h | 9 ++++----- src/gpu/GrDrawingManager.cpp | 7 ++----- src/gpu/GrDrawingManager.h | 5 +---- src/gpu/GrOpsTask.cpp | 4 +--- src/gpu/GrOpsTask.h | 14 +++----------- src/gpu/GrSurfaceContext.h | 1 - src/gpu/GrSurfaceFillContext.cpp | 4 ++-- src/gpu/GrSurfaceFillContext.h | 5 ----- src/gpu/ops/GrAtlasTextOp.cpp | 27 ++++++++++++++------------- src/gpu/ops/GrAtlasTextOp.h | 5 ++--- src/gpu/text/GrTextBlob.cpp | 9 +++------ tests/OpChainTest.cpp | 4 +--- tools/gpu/GrTest.cpp | 11 ++++------- 13 files changed, 37 insertions(+), 68 deletions(-) diff --git a/src/gpu/GrDrawOpTest.h b/src/gpu/GrDrawOpTest.h index a0d0b6c28c..23f744ee99 100644 --- a/src/gpu/GrDrawOpTest.h +++ b/src/gpu/GrDrawOpTest.h @@ -26,12 +26,11 @@ 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, \ - 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 7b9f61842b..8aec8f3ad9 100644 --- a/src/gpu/GrDrawingManager.cpp +++ b/src/gpu/GrDrawingManager.cpp @@ -659,17 +659,14 @@ void GrDrawingManager::closeActiveOpsTask() { } sk_sp GrDrawingManager::newOpsTask(GrSurfaceProxyView surfaceView, - sk_sp arenas, bool flushTimeOpsTask) { SkDEBUGCODE(this->validate()); SkASSERT(fContext); this->closeActiveOpsTask(); - sk_sp opsTask(new GrOpsTask(this, - std::move(surfaceView), - fContext->priv().auditTrail(), - std::move(arenas))); + sk_sp 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 8eac15a36f..1d91b248c7 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 @@ public: void freeGpuResources(); // OpsTasks created at flush time are stored and handled different from the others. - sk_sp newOpsTask(GrSurfaceProxyView, - sk_sp arenas, - bool flushTimeOpsTask); + sk_sp 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 2d106d14a2..cc7c8f3ab4 100644 --- a/src/gpu/GrOpsTask.cpp +++ b/src/gpu/GrOpsTask.cpp @@ -355,13 +355,11 @@ inline void GrOpsTask::OpChain::validate() const { GrOpsTask::GrOpsTask(GrDrawingManager* drawingMgr, GrSurfaceProxyView view, - GrAuditTrail* auditTrail, - sk_sp 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(4096)); this->addTarget(drawingMgr, view.detachProxy()); diff --git a/src/gpu/GrOpsTask.h b/src/gpu/GrOpsTask.h index 8ec0595b49..8b29ece31e 100644 --- a/src/gpu/GrOpsTask.h +++ b/src/gpu/GrOpsTask.h @@ -32,21 +32,14 @@ class GrClearOp; class GrGpuBuffer; class GrRenderTargetProxy; -class GrArenas : public SkNVRefCnt { -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); + // 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 @@ private: // MDB TODO: 4096 for the first allocation may be huge overkill. Gather statistics to determine // the correct size. SkSTArray<1, std::unique_ptr> fAllocators; - sk_sp 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 189e732eb1..deb2242204 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 9ef6889d6f..49bf84d7a3 100644 --- a/src/gpu/GrSurfaceFillContext.cpp +++ b/src/gpu/GrSurfaceFillContext.cpp @@ -342,8 +342,8 @@ GrOpsTask* GrSurfaceFillContext::getOpsTask() { SkDEBUGCODE(this->validate();) if (!fOpsTask || fOpsTask->isClosed()) { - sk_sp newOpsTask = this->drawingManager()->newOpsTask( - this->writeSurfaceView(), fArenas, fFlushTimeOpsTask); + sk_sp 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 b8763f0bba..d7e4b5ca0f 100644 --- a/src/gpu/GrSurfaceFillContext.h +++ b/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 fOpsTask; - // The arenas shared by the OpsTask. - sk_sp fArenas = sk_make_sp(); - bool fFlushTimeOpsTask; using INHERITED = GrSurfaceContext; diff --git a/src/gpu/ops/GrAtlasTextOp.cpp b/src/gpu/ops/GrAtlasTextOp.cpp index c528362e09..dcaf5e33b1 100644 --- a/src/gpu/ops/GrAtlasTextOp.cpp +++ b/src/gpu/ops/GrAtlasTextOp.cpp @@ -112,18 +112,14 @@ auto GrAtlasTextOp::Geometry::MakeForBlob(const GrAtlasSubRun& subRun, SkPoint drawOrigin, SkIRect clipRect, sk_sp 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 @@ 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 diff --git a/src/gpu/ops/GrAtlasTextOp.h b/src/gpu/ops/GrAtlasTextOp.h index 0a8710bbba..17e5a97b8d 100644 --- a/src/gpu/ops/GrAtlasTextOp.h +++ b/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 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 92d50718bd..a272251a76 100644 --- a/src/gpu/text/GrTextBlob.cpp +++ b/src/gpu/text/GrTextBlob.cpp @@ -677,8 +677,7 @@ DirectMaskSubRun::makeAtlasTextOp(const GrClip* clip, const SkMatrixProvider& vi drawOrigin, clipRect, sk_ref_sp(fBlob), - drawingColor, - sdc->arenaAlloc()); + drawingColor); GrRecordingContext* const rContext = sdc->recordingContext(); GrOp::Owner op = GrOp::Make(rContext, @@ -971,8 +970,7 @@ TransformedMaskSubRun::makeAtlasTextOp(const GrClip* clip, drawOrigin, SkIRect::MakeEmpty(), sk_ref_sp(fBlob), - drawingColor, - sdc->arenaAlloc()); + drawingColor); GrRecordingContext* const rContext = sdc->recordingContext(); GrOp::Owner op = GrOp::Make( @@ -1254,8 +1252,7 @@ SDFTSubRun::makeAtlasTextOp(const GrClip* clip, drawOrigin, SkIRect::MakeEmpty(), sk_ref_sp(fBlob), - drawingColor, - sdc->arenaAlloc()); + drawingColor); GrRecordingContext* const rContext = sdc->recordingContext(); GrOp::Owner op = GrOp::Make( diff --git a/tests/OpChainTest.cpp b/tests/OpChainTest.cpp index 2761f217f3..df9e535fef 100644 --- a/tests/OpChainTest.cpp +++ b/tests/OpChainTest.cpp @@ -205,7 +205,6 @@ DEF_GPUTEST(OpChainTest, reporter, /*ctxInfo*/) { bool repeat = false; Combinable combinable; GrDrawingManager* drawingMgr = dContext->priv().drawingManager(); - sk_sp arenas = sk_make_sp(); 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(), - 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 ac03100d1a..85f25cb170 100644 --- a/tools/gpu/GrTest.cpp +++ b/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(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.