From 5a2de5e72f24d1bfbdc532252be3dcdefa7b75a2 Mon Sep 17 00:00:00 2001 From: Herb Derby Date: Mon, 5 Apr 2021 18:38:35 -0400 Subject: [PATCH] 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 --- 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, 68 insertions(+), 37 deletions(-) diff --git a/src/gpu/GrDrawOpTest.h b/src/gpu/GrDrawOpTest.h index 23f744ee99..a0d0b6c28c 100644 --- a/src/gpu/GrDrawOpTest.h +++ b/src/gpu/GrDrawOpTest.h @@ -26,11 +26,12 @@ 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, int numSamples) + GrOp::Owner Op##__Test(GrPaint&& paint, SkRandom* random, \ + GrRecordingContext* context, \ + GrSurfaceDrawContext* sdc, int numSamples) #define GR_DRAW_OP_TEST_FRIEND(Op) \ - friend GrOp::OpOwner Op##__Test(GrPaint&& paint, SkRandom* random, \ - GrRecordingContext* context, int numSamples) + friend GrOp::OpOwner Op##__Test(GrPaint&&, SkRandom*, \ + GrRecordingContext*, GrSurfaceDrawContext*, int) /** 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 8aec8f3ad9..7b9f61842b 100644 --- a/src/gpu/GrDrawingManager.cpp +++ b/src/gpu/GrDrawingManager.cpp @@ -659,14 +659,17 @@ 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())); + sk_sp opsTask(new GrOpsTask(this, + std::move(surfaceView), + fContext->priv().auditTrail(), + std::move(arenas))); SkASSERT(this->getLastRenderTask(opsTask->target(0)) == opsTask.get()); if (flushTimeOpsTask) { diff --git a/src/gpu/GrDrawingManager.h b/src/gpu/GrDrawingManager.h index 1d91b248c7..8eac15a36f 100644 --- a/src/gpu/GrDrawingManager.h +++ b/src/gpu/GrDrawingManager.h @@ -23,6 +23,7 @@ // 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; @@ -46,7 +47,9 @@ public: void freeGpuResources(); // OpsTasks created at flush time are stored and handled different from the others. - sk_sp newOpsTask(GrSurfaceProxyView, bool flushTimeOpsTask); + sk_sp newOpsTask(GrSurfaceProxyView, + sk_sp 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 diff --git a/src/gpu/GrOpsTask.cpp b/src/gpu/GrOpsTask.cpp index cc7c8f3ab4..2d106d14a2 100644 --- a/src/gpu/GrOpsTask.cpp +++ b/src/gpu/GrOpsTask.cpp @@ -355,11 +355,13 @@ inline void GrOpsTask::OpChain::validate() const { GrOpsTask::GrOpsTask(GrDrawingManager* drawingMgr, GrSurfaceProxyView view, - GrAuditTrail* auditTrail) + GrAuditTrail* auditTrail, + sk_sp arenas) : 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 8b29ece31e..8ec0595b49 100644 --- a/src/gpu/GrOpsTask.h +++ b/src/gpu/GrOpsTask.h @@ -32,14 +32,21 @@ 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: - // 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*); + // Manage the arenas life time by maintaining are reference to it. + GrOpsTask(GrDrawingManager*, GrSurfaceProxyView, GrAuditTrail*, sk_sp); ~GrOpsTask() override; GrOpsTask* asOpsTask() override { return this; } @@ -268,6 +275,7 @@ 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 deb2242204..189e732eb1 100644 --- a/src/gpu/GrSurfaceContext.h +++ b/src/gpu/GrSurfaceContext.h @@ -17,6 +17,7 @@ #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 49bf84d7a3..9ef6889d6f 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(), - fFlushTimeOpsTask); + sk_sp newOpsTask = this->drawingManager()->newOpsTask( + this->writeSurfaceView(), fArenas, fFlushTimeOpsTask); if (fOpsTask) { this->willReplaceOpsTask(fOpsTask.get(), newOpsTask.get()); } diff --git a/src/gpu/GrSurfaceFillContext.h b/src/gpu/GrSurfaceFillContext.h index d7e4b5ca0f..b8763f0bba 100644 --- a/src/gpu/GrSurfaceFillContext.h +++ b/src/gpu/GrSurfaceFillContext.h @@ -195,6 +195,8 @@ 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 @@ -241,6 +243,9 @@ 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 dcaf5e33b1..c528362e09 100644 --- a/src/gpu/ops/GrAtlasTextOp.cpp +++ b/src/gpu/ops/GrAtlasTextOp.cpp @@ -112,14 +112,18 @@ auto GrAtlasTextOp::Geometry::MakeForBlob(const GrAtlasSubRun& subRun, SkPoint drawOrigin, SkIRect clipRect, sk_sp blob, - const SkPMColor4f& color) -> Geometry* { - return new Geometry{subRun, - drawMatrix, - drawOrigin, - clipRect, - std::move(blob), - nullptr, - color}; + 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}; } void GrAtlasTextOp::Geometry::fillVertexData(void *dst, int offset, int count) const { @@ -522,10 +526,6 @@ 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,8 +548,7 @@ GR_DRAW_OP_TEST_DEFINE(GrAtlasTextOp) { int xInt = (random->nextU() % kMaxTrans) * xPos; int yInt = (random->nextU() % kMaxTrans) * yPos; - return GrAtlasTextOp::CreateOpTestingOnly( - rtc.get(), skPaint, font, matrixProvider, text, xInt, yInt); + return GrAtlasTextOp::CreateOpTestingOnly(sdc, skPaint, font, matrixProvider, text, xInt, yInt); } #endif diff --git a/src/gpu/ops/GrAtlasTextOp.h b/src/gpu/ops/GrAtlasTextOp.h index 17e5a97b8d..0a8710bbba 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; - delete g; + g->~Geometry(); g = next; } } @@ -67,7 +67,8 @@ public: SkPoint drawOrigin, SkIRect clipRect, sk_sp blob, - const SkPMColor4f& color); + const SkPMColor4f& color, + SkArenaAlloc* alloc); void fillVertexData(void* dst, int offset, int count) const; diff --git a/src/gpu/text/GrTextBlob.cpp b/src/gpu/text/GrTextBlob.cpp index a272251a76..92d50718bd 100644 --- a/src/gpu/text/GrTextBlob.cpp +++ b/src/gpu/text/GrTextBlob.cpp @@ -677,7 +677,8 @@ DirectMaskSubRun::makeAtlasTextOp(const GrClip* clip, const SkMatrixProvider& vi drawOrigin, clipRect, sk_ref_sp(fBlob), - drawingColor); + drawingColor, + sdc->arenaAlloc()); GrRecordingContext* const rContext = sdc->recordingContext(); GrOp::Owner op = GrOp::Make(rContext, @@ -970,7 +971,8 @@ TransformedMaskSubRun::makeAtlasTextOp(const GrClip* clip, drawOrigin, SkIRect::MakeEmpty(), sk_ref_sp(fBlob), - drawingColor); + drawingColor, + sdc->arenaAlloc()); GrRecordingContext* const rContext = sdc->recordingContext(); GrOp::Owner op = GrOp::Make( @@ -1252,7 +1254,8 @@ SDFTSubRun::makeAtlasTextOp(const GrClip* clip, drawOrigin, SkIRect::MakeEmpty(), sk_ref_sp(fBlob), - drawingColor); + drawingColor, + sdc->arenaAlloc()); GrRecordingContext* const rContext = sdc->recordingContext(); GrOp::Owner op = GrOp::Make( diff --git a/tests/OpChainTest.cpp b/tests/OpChainTest.cpp index df9e535fef..2761f217f3 100644 --- a/tests/OpChainTest.cpp +++ b/tests/OpChainTest.cpp @@ -205,6 +205,7 @@ 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. :( @@ -221,7 +222,8 @@ 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); diff --git a/tools/gpu/GrTest.cpp b/tools/gpu/GrTest.cpp index 85f25cb170..ac03100d1a 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*, int numSamples) + GrRecordingContext*, GrSurfaceDrawContext*, int) #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*, int numSamples); + GrRecordingContext*, GrSurfaceDrawContext*, int numSamples); static constexpr MakeDrawOpFn* gFactories[] = { DRAW_OP_TEST_ENTRY(AAConvexPathOp), DRAW_OP_TEST_ENTRY(AAFlatteningConvexPathOp), @@ -108,8 +108,11 @@ 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->numSamples()); + auto op = gFactories[index](std::move(paint), + random, + context, + surfaceDrawContext, + surfaceDrawContext->numSamples()); // Creating a GrAtlasTextOp my not produce an op if for example, it is totally outside the // render target context.