Simplify GrResourceAllocator API
There's really only one failure at this point, and it's a failed
instantiation. The original intention of the allocation failure
system was to only drop the ops that referred to the uninstantiated
proxies, but somewhere along the way that was lost and
we started dropping arbitrarily large chunks of ops. Lets just
bail and not crash instead.
Bug: skia:10877
Change-Id: I675358e8a1fbd2d75ea29b72ccfc50c7df90343e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/371337
Commit-Queue: Adlai Holler <adlai@google.com>
Reviewed-by: Robert Phillips <robertphillips@google.com>
diff --git a/src/gpu/GrCopyRenderTask.h b/src/gpu/GrCopyRenderTask.h
index 6c6a743..756e598 100644
--- a/src/gpu/GrCopyRenderTask.h
+++ b/src/gpu/GrCopyRenderTask.h
@@ -33,8 +33,6 @@
void onCanSkip() override { fSrc.reset(); }
bool onIsUsed(GrSurfaceProxy* proxy) const override { return proxy == fSrc.get(); }
- // If instantiation failed, at flush time we simply will skip doing the copy.
- void handleInternalAllocationFailure() override {}
void gatherProxyIntervals(GrResourceAllocator*) const override;
ExpectedOutcome onMakeClosed(const GrCaps&, SkIRect* targetUpdateBounds) override;
bool onExecute(GrOpFlushState*) override;
diff --git a/src/gpu/GrDDLTask.cpp b/src/gpu/GrDDLTask.cpp
index f689ec2..42f62e9 100644
--- a/src/gpu/GrDDLTask.cpp
+++ b/src/gpu/GrDDLTask.cpp
@@ -64,12 +64,6 @@
return false;
}
-void GrDDLTask::handleInternalAllocationFailure() {
- for (auto& task : fDDL->priv().renderTasks()) {
- task->handleInternalAllocationFailure();
- }
-}
-
void GrDDLTask::gatherProxyIntervals(GrResourceAllocator* alloc) const {
// We don't have any proxies, but the resource allocator will still bark
// if a task doesn't claim any op indices, so we oblige it.
diff --git a/src/gpu/GrDDLTask.h b/src/gpu/GrDDLTask.h
index 820799e..e94d5d0 100644
--- a/src/gpu/GrDDLTask.h
+++ b/src/gpu/GrDDLTask.h
@@ -39,8 +39,6 @@
private:
bool onIsUsed(GrSurfaceProxy*) const override;
- void handleInternalAllocationFailure() override;
-
void gatherProxyIntervals(GrResourceAllocator*) const override;
ExpectedOutcome onMakeClosed(const GrCaps&, SkIRect* targetUpdateBounds) override;
diff --git a/src/gpu/GrDrawingManager.cpp b/src/gpu/GrDrawingManager.cpp
index 628d0fe..3bc1b7d 100644
--- a/src/gpu/GrDrawingManager.cpp
+++ b/src/gpu/GrDrawingManager.cpp
@@ -211,29 +211,9 @@
task->gatherProxyIntervals(&alloc);
}
- GrResourceAllocator::AssignError error = GrResourceAllocator::AssignError::kNoError;
- alloc.assign(&error);
- if (GrResourceAllocator::AssignError::kFailedProxyInstantiation == error) {
- for (const auto& renderTask : fDAG) {
- SkASSERT(renderTask);
- if (!renderTask->isInstantiated()) {
- // No need to call the renderTask's handleInternalAllocationFailure
- // since we will already skip executing the renderTask since it is not
- // instantiated.
- continue;
- }
- // TODO: If we're going to remove all the render tasks do we really need this call?
- renderTask->handleInternalAllocationFailure();
- }
- this->removeRenderTasks();
- }
-
- if (this->executeRenderTasks(&flushState)) {
- flushed = true;
- }
+ flushed = alloc.assign() && this->executeRenderTasks(&flushState);
}
-
- SkASSERT(fDAG.empty());
+ this->removeRenderTasks();
#ifdef SK_DEBUG
// In non-DDL mode this checks that all the flushed ops have been freed from the memory pool.
@@ -351,8 +331,6 @@
// resources are the last to be purged by the resource cache.
flushState->reset();
- this->removeRenderTasks();
-
return anyRenderTasksExecuted;
}
diff --git a/src/gpu/GrOpsTask.cpp b/src/gpu/GrOpsTask.cpp
index c8d5f42..895fecc 100644
--- a/src/gpu/GrOpsTask.cpp
+++ b/src/gpu/GrOpsTask.cpp
@@ -890,22 +890,6 @@
return used;
}
-void GrOpsTask::handleInternalAllocationFailure() {
- bool hasUninstantiatedProxy = false;
- auto checkInstantiation = [&hasUninstantiatedProxy](GrSurfaceProxy* p, GrMipmapped) {
- if (!p->isInstantiated()) {
- hasUninstantiatedProxy = true;
- }
- };
- for (OpChain& recordedOp : fOpChains) {
- hasUninstantiatedProxy = false;
- recordedOp.visitProxies(checkInstantiation);
- if (hasUninstantiatedProxy) {
- recordedOp.setSkipExecuteFlag();
- }
- }
-}
-
void GrOpsTask::gatherProxyIntervals(GrResourceAllocator* alloc) const {
for (int i = 0; i < fDeferredProxies.count(); ++i) {
SkASSERT(!fDeferredProxies[i]->isInstantiated());
diff --git a/src/gpu/GrOpsTask.h b/src/gpu/GrOpsTask.h
index f55ee9e..0f2d584 100644
--- a/src/gpu/GrOpsTask.h
+++ b/src/gpu/GrOpsTask.h
@@ -181,9 +181,8 @@
const DstProxyView*, const GrAppliedClip*, const GrCaps&,
GrRecordingContext::Arenas*, GrAuditTrail*);
- void setSkipExecuteFlag() { fSkipExecute = true; }
bool shouldExecute() const {
- return SkToBool(this->head()) && !fSkipExecute;
+ return SkToBool(this->head());
}
private:
@@ -222,18 +221,12 @@
DstProxyView fDstProxyView;
GrAppliedClip* fAppliedClip;
SkRect fBounds;
-
- // We set this flag to true if any of the ops' proxies fail to instantiate so that we know
- // not to try and draw the op.
- bool fSkipExecute = false;
};
void onCanSkip() override;
bool onIsUsed(GrSurfaceProxy*) const override;
- void handleInternalAllocationFailure() override;
-
void gatherProxyIntervals(GrResourceAllocator*) const override;
void recordOp(GrOp::Owner, GrProcessorSet::Analysis, GrAppliedClip*,
diff --git a/src/gpu/GrRenderTask.h b/src/gpu/GrRenderTask.h
index 37ac2c1..16dedfd 100644
--- a/src/gpu/GrRenderTask.h
+++ b/src/gpu/GrRenderTask.h
@@ -129,11 +129,6 @@
return this->onIsUsed(proxy);
}
- // Drops any pending operations that reference proxies that are not instantiated.
- // NOTE: Derived classes don't need to check targets. That is handled when the
- // drawingManager calls isInstantiated.
- virtual void handleInternalAllocationFailure() = 0;
-
// Feed proxy usage intervals to the GrResourceAllocator class
virtual void gatherProxyIntervals(GrResourceAllocator*) const = 0;
diff --git a/src/gpu/GrResourceAllocator.cpp b/src/gpu/GrResourceAllocator.cpp
index 29bf950..fd5dc7f 100644
--- a/src/gpu/GrResourceAllocator.cpp
+++ b/src/gpu/GrResourceAllocator.cpp
@@ -70,7 +70,7 @@
// with the same texture.
if (proxy->readOnly()) {
if (proxy->isLazy() && !proxy->priv().doLazyInstantiation(fResourceProvider)) {
- fLazyInstantiationError = true;
+ fFailedInstantiation = true;
} else {
// Since we aren't going to add an interval we won't revisit this proxy in assign(). So
// must already be instantiated or it must be a lazy proxy that we instantiated above.
@@ -293,17 +293,13 @@
}
}
-void GrResourceAllocator::assign(AssignError* outError) {
- SkASSERT(outError);
- *outError = fLazyInstantiationError ? AssignError::kFailedProxyInstantiation
- : AssignError::kNoError;
-
+bool GrResourceAllocator::assign() {
fIntvlHash.reset(); // we don't need the interval hash anymore
SkDEBUGCODE(fAssigned = true;)
if (fIntvlList.empty()) {
- return; // no resources to assign
+ return !fFailedInstantiation; // no resources to assign
}
#if GR_ALLOCATION_SPEW
@@ -314,7 +310,8 @@
// TODO: Can this be done inline during the main iteration?
this->determineRecyclability();
- while (Interval* cur = fIntvlList.popHead()) {
+ Interval* cur = nullptr;
+ while ((cur = fIntvlList.popHead()) && !fFailedInstantiation) {
this->expire(cur->start());
if (cur->proxy()->isInstantiated()) {
@@ -325,7 +322,7 @@
if (cur->proxy()->isLazy()) {
if (!cur->proxy()->priv().doLazyInstantiation(fResourceProvider)) {
- *outError = AssignError::kFailedProxyInstantiation;
+ fFailedInstantiation = true;
}
} else if (sk_sp<GrSurface> surface = this->findSurfaceFor(cur->proxy())) {
// TODO: make getUniqueKey virtual on GrSurfaceProxy
@@ -348,7 +345,7 @@
cur->assign(std::move(surface));
} else {
SkASSERT(!cur->proxy()->isInstantiated());
- *outError = AssignError::kFailedProxyInstantiation;
+ fFailedInstantiation = true;
}
fActiveIntvls.insertByIncreasingEnd(cur);
@@ -356,6 +353,7 @@
// expire all the remaining intervals to drain the active interval list
this->expire(std::numeric_limits<unsigned int>::max());
+ return !fFailedInstantiation;
}
#if GR_ALLOCATION_SPEW
diff --git a/src/gpu/GrResourceAllocator.h b/src/gpu/GrResourceAllocator.h
index 8b7071e..c69dc2e 100644
--- a/src/gpu/GrResourceAllocator.h
+++ b/src/gpu/GrResourceAllocator.h
@@ -90,14 +90,8 @@
void addInterval(GrSurfaceProxy*, unsigned int start, unsigned int end, ActualUse actualUse
SkDEBUGCODE(, bool isDirectDstRead = false));
- enum class AssignError {
- kNoError,
- kFailedProxyInstantiation
- };
-
- // If any proxy fails to instantiate, the AssignError will be set to kFailedProxyInstantiation.
- // If this happens, the caller should remove all ops which reference an uninstantiated proxy.
- void assign(AssignError* outError);
+ // Assign resources to all proxies. Returns whether the assignment was successful.
+ bool assign();
#if GR_ALLOCATION_SPEW
void dumpIntervals();
@@ -258,10 +252,9 @@
SkDEBUGCODE(bool fAssigned = false;)
- char fStorage[kInitialArenaSize];
- SkArenaAlloc fIntervalAllocator{fStorage, kInitialArenaSize, kInitialArenaSize};
- Interval* fFreeIntervalList = nullptr;
- bool fLazyInstantiationError = false;
+ SkSTArenaAlloc<kInitialArenaSize> fIntervalAllocator;
+ Interval* fFreeIntervalList = nullptr;
+ bool fFailedInstantiation = false;
};
#endif // GrResourceAllocator_DEFINED
diff --git a/src/gpu/GrTextureResolveRenderTask.h b/src/gpu/GrTextureResolveRenderTask.h
index 4cfe24c..ab6e7f5 100644
--- a/src/gpu/GrTextureResolveRenderTask.h
+++ b/src/gpu/GrTextureResolveRenderTask.h
@@ -21,9 +21,6 @@
bool onIsUsed(GrSurfaceProxy* proxy) const override {
return false;
}
- void handleInternalAllocationFailure() override {
- // No need to do anything special here. We just double check the proxies during onExecute.
- }
void gatherProxyIntervals(GrResourceAllocator*) const override;
ExpectedOutcome onMakeClosed(const GrCaps&, SkIRect*) override {
diff --git a/src/gpu/GrTransferFromRenderTask.h b/src/gpu/GrTransferFromRenderTask.h
index aa80be4..08513bf 100644
--- a/src/gpu/GrTransferFromRenderTask.h
+++ b/src/gpu/GrTransferFromRenderTask.h
@@ -31,8 +31,6 @@
SkASSERT(0 == this->numTargets());
return proxy == fSrcProxy.get();
}
- // If fSrcProxy is uninstantiated at flush time we simply will skip doing the transfer.
- void handleInternalAllocationFailure() override {}
void gatherProxyIntervals(GrResourceAllocator*) const override;
ExpectedOutcome onMakeClosed(const GrCaps&, SkIRect*) override {
diff --git a/src/gpu/GrWaitRenderTask.h b/src/gpu/GrWaitRenderTask.h
index 980f35e..c32a581 100644
--- a/src/gpu/GrWaitRenderTask.h
+++ b/src/gpu/GrWaitRenderTask.h
@@ -25,7 +25,6 @@
bool onIsUsed(GrSurfaceProxy* proxy) const override {
return proxy == fWaitedOn.proxy();
}
- void handleInternalAllocationFailure() override {}
void gatherProxyIntervals(GrResourceAllocator*) const override;
ExpectedOutcome onMakeClosed(const GrCaps&, SkIRect*) override {
diff --git a/src/gpu/GrWritePixelsRenderTask.h b/src/gpu/GrWritePixelsRenderTask.h
index 9de091f..a2a49ae 100644
--- a/src/gpu/GrWritePixelsRenderTask.h
+++ b/src/gpu/GrWritePixelsRenderTask.h
@@ -32,8 +32,6 @@
sk_sp<SkData> pixelStorage);
bool onIsUsed(GrSurfaceProxy* proxy) const override { return false; }
- // If instantiation failed, at flush time we simply will skip doing the write.
- void handleInternalAllocationFailure() override {}
void gatherProxyIntervals(GrResourceAllocator*) const override;
ExpectedOutcome onMakeClosed(const GrCaps&, SkIRect* targetUpdateBounds) override;
bool onExecute(GrOpFlushState*) override;
diff --git a/src/gpu/mock/GrMockRenderTask.h b/src/gpu/mock/GrMockRenderTask.h
index 8564bb6..ea20f7e 100644
--- a/src/gpu/mock/GrMockRenderTask.h
+++ b/src/gpu/mock/GrMockRenderTask.h
@@ -25,7 +25,6 @@
#ifdef SK_DEBUG
void visitProxies_debugOnly(const GrOp::VisitProxyFunc&) const override { return; }
#endif
- void handleInternalAllocationFailure() override {}
void gatherProxyIntervals(GrResourceAllocator*) const override {}
ExpectedOutcome onMakeClosed(const GrCaps&, SkIRect*) override { SkUNREACHABLE; }
bool onIsUsed(GrSurfaceProxy* proxy) const override {
diff --git a/tests/ResourceAllocatorTest.cpp b/tests/ResourceAllocatorTest.cpp
index a8b43e9..a938984 100644
--- a/tests/ResourceAllocatorTest.cpp
+++ b/tests/ResourceAllocatorTest.cpp
@@ -68,9 +68,7 @@
alloc.addInterval(p2.get(), 1, 2, GrResourceAllocator::ActualUse::kYes);
alloc.incOps();
- GrResourceAllocator::AssignError error;
- alloc.assign(&error);
- REPORTER_ASSERT(reporter, GrResourceAllocator::AssignError::kNoError == error);
+ REPORTER_ASSERT(reporter, alloc.assign());
REPORTER_ASSERT(reporter, p1->peekSurface());
REPORTER_ASSERT(reporter, p2->peekSurface());
@@ -95,9 +93,7 @@
alloc.addInterval(p1.get(), 0, 2, GrResourceAllocator::ActualUse::kYes);
alloc.addInterval(p2.get(), 3, 5, GrResourceAllocator::ActualUse::kYes);
- GrResourceAllocator::AssignError error;
- alloc.assign(&error);
- REPORTER_ASSERT(reporter, GrResourceAllocator::AssignError::kNoError == error);
+ REPORTER_ASSERT(reporter, alloc.assign());
REPORTER_ASSERT(reporter, p1->peekSurface());
REPORTER_ASSERT(reporter, p2->peekSurface());