Revert "Add async pipelines to the cache inside the async task"
This reverts commit 42e95ff6e859aba07909b9c6637bece9a169ccab.
Reason for revert: https://crbug.com/dawn/2411
Original change's description:
> Add async pipelines to the cache inside the async task
>
> We can do this now that the cache is thread-safe.
> This adds the pipelines into the cache as soon as possible, instead
> of adding it right before calling the user callback.
>
> Bug: dawn:529
> Change-Id: I3996f6285a20dfcee9bbb2f6e03c236f7e239165
> Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/174260
> Kokoro: Austin Eng <enga@chromium.org>
> Reviewed-by: Corentin Wallez <cwallez@chromium.org>
> Commit-Queue: Austin Eng <enga@chromium.org>
> Reviewed-by: Loko Kung <lokokung@google.com>
Bug: dawn:529, dawn:2411
Change-Id: Id2c8ce81303bcb31bd3dd327191c4cfa03c84757
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/175001
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Austin Eng <enga@chromium.org>
Reviewed-by: Loko Kung <lokokung@google.com>
Reviewed-by: Austin Eng <enga@chromium.org>
diff --git a/src/dawn/native/CreatePipelineAsyncTask.cpp b/src/dawn/native/CreatePipelineAsyncTask.cpp
index 20df685..d6ea436 100644
--- a/src/dawn/native/CreatePipelineAsyncTask.cpp
+++ b/src/dawn/native/CreatePipelineAsyncTask.cpp
@@ -73,9 +73,6 @@
device->AddComputePipelineAsyncCallbackTask(
maybeError.AcquireError(), mComputePipeline->GetLabel().c_str(), mCallback, mUserdata);
} else {
- if (device->GetState() == DeviceBase::State::Alive) {
- mComputePipeline = device->AddOrGetCachedComputePipeline(std::move(mComputePipeline));
- }
device->AddComputePipelineAsyncCallbackTask(mComputePipeline, mCallback, mUserdata);
}
}
@@ -134,9 +131,6 @@
device->AddRenderPipelineAsyncCallbackTask(
maybeError.AcquireError(), mRenderPipeline->GetLabel().c_str(), mCallback, mUserdata);
} else {
- if (device->GetState() == DeviceBase::State::Alive) {
- mRenderPipeline = device->AddOrGetCachedRenderPipeline(std::move(mRenderPipeline));
- }
device->AddRenderPipelineAsyncCallbackTask(mRenderPipeline, mCallback, mUserdata);
}
}
diff --git a/src/dawn/native/Device.cpp b/src/dawn/native/Device.cpp
index 15c7b39..b6adac6 100644
--- a/src/dawn/native/Device.cpp
+++ b/src/dawn/native/Device.cpp
@@ -965,12 +965,14 @@
Ref<ComputePipelineBase> DeviceBase::AddOrGetCachedComputePipeline(
Ref<ComputePipelineBase> computePipeline) {
+ DAWN_ASSERT(IsLockedByCurrentThreadIfNeeded());
auto [pipeline, _] = mCaches->computePipelines.Insert(computePipeline.Get());
return std::move(pipeline);
}
Ref<RenderPipelineBase> DeviceBase::AddOrGetCachedRenderPipeline(
Ref<RenderPipelineBase> renderPipeline) {
+ DAWN_ASSERT(IsLockedByCurrentThreadIfNeeded());
auto [pipeline, _] = mCaches->renderPipelines.Insert(renderPipeline.Get());
return std::move(pipeline);
}
@@ -1837,8 +1839,7 @@
AddComputePipelineAsyncCallbackTask(
maybeError.AcquireError(), computePipeline->GetLabel().c_str(), callback, userdata);
} else {
- AddComputePipelineAsyncCallbackTask(
- AddOrGetCachedComputePipeline(std::move(computePipeline)), callback, userdata);
+ AddComputePipelineAsyncCallbackTask(std::move(computePipeline), callback, userdata);
}
}
@@ -1858,8 +1859,7 @@
AddRenderPipelineAsyncCallbackTask(maybeError.AcquireError(),
renderPipeline->GetLabel().c_str(), callback, userdata);
} else {
- AddRenderPipelineAsyncCallbackTask(AddOrGetCachedRenderPipeline(std::move(renderPipeline)),
- callback, userdata);
+ AddRenderPipelineAsyncCallbackTask(std::move(renderPipeline), callback, userdata);
}
}
@@ -2148,6 +2148,22 @@
void* userdata) {
mCallbackTaskManager->AddCallbackTask(
[callback, pipeline = std::move(pipeline), userdata]() mutable {
+ // TODO(dawn:529): call AddOrGetCachedComputePipeline() asynchronously in
+ // CreateComputePipelineAsyncTaskImpl::Run() when the front-end pipeline cache is
+ // thread-safe.
+ DAWN_ASSERT(pipeline != nullptr);
+ {
+ // This is called inside a callback, and no lock will be held by default so we
+ // have to lock now to protect the cache. Note: we don't lock inside
+ // AddOrGetCachedComputePipeline() to avoid deadlock because many places calling
+ // that method might already have the lock held. For example,
+ // APICreateComputePipeline()
+ auto deviceLock(pipeline->GetDevice()->GetScopedLock());
+ if (pipeline->GetDevice()->GetState() == State::Alive) {
+ pipeline =
+ pipeline->GetDevice()->AddOrGetCachedComputePipeline(std::move(pipeline));
+ }
+ }
callback(WGPUCreatePipelineAsyncStatus_Success, ToAPI(ReturnToAPI(std::move(pipeline))),
"", userdata);
});
@@ -2177,11 +2193,26 @@
void DeviceBase::AddRenderPipelineAsyncCallbackTask(Ref<RenderPipelineBase> pipeline,
WGPUCreateRenderPipelineAsyncCallback callback,
void* userdata) {
- mCallbackTaskManager->AddCallbackTask(
- [callback, pipeline = std::move(pipeline), userdata]() mutable {
- callback(WGPUCreatePipelineAsyncStatus_Success, ToAPI(ReturnToAPI(std::move(pipeline))),
- "", userdata);
- });
+ mCallbackTaskManager->AddCallbackTask([callback, pipeline = std::move(pipeline),
+ userdata]() mutable {
+ // TODO(dawn:529): call AddOrGetCachedRenderPipeline() asynchronously in
+ // CreateRenderPipelineAsyncTaskImpl::Run() when the front-end pipeline cache is
+ // thread-safe.
+ DAWN_ASSERT(pipeline != nullptr);
+ {
+ // This is called inside a callback, and no lock will be held by default so we have
+ // to lock now to protect the cache.
+ // Note: we don't lock inside AddOrGetCachedRenderPipeline() to avoid deadlock
+ // because many places calling that method might already have the lock held. For
+ // example, APICreateRenderPipeline()
+ auto deviceLock(pipeline->GetDevice()->GetScopedLock());
+ if (pipeline->GetDevice()->GetState() == State::Alive) {
+ pipeline = pipeline->GetDevice()->AddOrGetCachedRenderPipeline(std::move(pipeline));
+ }
+ }
+ callback(WGPUCreatePipelineAsyncStatus_Success, ToAPI(ReturnToAPI(std::move(pipeline))), "",
+ userdata);
+ });
}
PipelineCompatibilityToken DeviceBase::GetNextPipelineCompatibilityToken() {
diff --git a/src/dawn/native/Device.h b/src/dawn/native/Device.h
index 13ff30a..596457d 100644
--- a/src/dawn/native/Device.h
+++ b/src/dawn/native/Device.h
@@ -425,10 +425,6 @@
CallbackTaskManager* GetCallbackTaskManager() const;
dawn::platform::WorkerTaskPool* GetWorkerTaskPool() const;
- Ref<ComputePipelineBase> AddOrGetCachedComputePipeline(
- Ref<ComputePipelineBase> computePipeline);
- Ref<RenderPipelineBase> AddOrGetCachedRenderPipeline(Ref<RenderPipelineBase> renderPipeline);
-
// Enqueue a successfully-create async pipeline creation callback.
void AddComputePipelineAsyncCallbackTask(Ref<ComputePipelineBase> pipeline,
WGPUCreateComputePipelineAsyncCallback callback,
@@ -536,6 +532,9 @@
ComputePipelineBase* uninitializedComputePipeline);
Ref<RenderPipelineBase> GetCachedRenderPipeline(
RenderPipelineBase* uninitializedRenderPipeline);
+ Ref<ComputePipelineBase> AddOrGetCachedComputePipeline(
+ Ref<ComputePipelineBase> computePipeline);
+ Ref<RenderPipelineBase> AddOrGetCachedRenderPipeline(Ref<RenderPipelineBase> renderPipeline);
virtual Ref<PipelineCacheBase> GetOrCreatePipelineCacheImpl(const CacheKey& key);
virtual void InitializeComputePipelineAsyncImpl(Ref<ComputePipelineBase> computePipeline,
WGPUCreateComputePipelineAsyncCallback callback,
