[dawn][native] Some cleanup of DeviceLostEvent code. - Uses the refcount payload in RefCountedWithExternalCount to track whether an external ref count exists. Note that we are using a "ForTesting" function to implement the check of the count even though this is not for testing. We are special casing this because it is a valid check for the external ref count. This is needed to explicitly handle the spec behavior that the device lost event should pass nullptr for the device when the last external reference has been dropped. - Makes APIGetLostFuture use cached future id when possible. - Moves the device lost related members in Device to private since the mock isn't actually using the alternative constructor anymore. Also removes said constructor. - Finally, defer cleanup of the callback infos to the completion of the device lost callback instead of having it called in two different places, the callback and in DeviceBase::WillDropLastExternalRef. Change-Id: I31b57391438c0782ba223884a72991ca77426bec Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/299539 Commit-Queue: Loko Kung <lokokung@google.com> Reviewed-by: Kai Ninomiya <kainino@chromium.org>
diff --git a/src/dawn/common/RefCountedWithExternalCount.h b/src/dawn/common/RefCountedWithExternalCount.h index 86bd70b..b88b2ca 100644 --- a/src/dawn/common/RefCountedWithExternalCount.h +++ b/src/dawn/common/RefCountedWithExternalCount.h
@@ -71,6 +71,13 @@ return mExternalRefCount.GetValueForTesting(); } + protected: + bool HasExternalRef() const { + // Note that we call the "ForTesting" function here but this is a use case where it is not + // for testing, and we are allowing it as a special case for external reference counting. + return mExternalRefCount.GetValueForTesting() > 0; + } + private: virtual void WillAddFirstExternalRef() {} virtual void WillDropLastExternalRef() = 0;
diff --git a/src/dawn/native/Adapter.cpp b/src/dawn/native/Adapter.cpp index 7afcc66..1d2847d 100644 --- a/src/dawn/native/Adapter.cpp +++ b/src/dawn/native/Adapter.cpp
@@ -360,13 +360,11 @@ "Failed to create device:\n" + error->GetFormattedMessage()); // When the device fails to initialize, we need to both promote the device ref to an - // external ref to clean up resources, and drop it, so we acquire it in this scope. + // external ref to clean up resources, and drop it, so we acquire it in this scope. Note + // that we move the device ref out of the event to also let the event know that the event + // should be resolved. APIRef<DeviceBase> device; device.Acquire(ReturnToAPI(std::move(lostEvent->mDevice))); - // Reset the device's lost event to avoid double SetLost during destruction. - if (device) { - device->ResetLostEvent(); - } return {lostEvent, std::move(error)}; }
diff --git a/src/dawn/native/Device.cpp b/src/dawn/native/Device.cpp index e9be296..44910a9 100644 --- a/src/dawn/native/Device.cpp +++ b/src/dawn/native/Device.cpp
@@ -178,6 +178,10 @@ void DeviceBase::DeviceLostEvent::SetLost(EventManager* eventManager, wgpu::DeviceLostReason reason, std::string_view message) { + // If this event is already ready, don't bother overriding the cause. + if (IsReadyToComplete()) { + return; + } mReason = reason; mMessage = message; eventManager->SetFutureReady(this); @@ -201,7 +205,8 @@ void* userdata2 = mUserdata2.ExtractAsDangling(); if (mReason == wgpu::DeviceLostReason::CallbackCancelled || - mReason == wgpu::DeviceLostReason::FailedCreation) { + mReason == wgpu::DeviceLostReason::FailedCreation || + (mDevice && !mDevice->HasExternalRef())) { device = nullptr; } if (mCallback) { @@ -384,18 +389,6 @@ StreamIn(&mDeviceCacheKey, kDawnVersion, hash); } -DeviceBase::DeviceBase() : mState(State::Alive), mToggles(ToggleStage::Device) { - GetDefaultLimits(&mLimits, wgpu::FeatureLevel::Core); - EnforceLimitSpecInvariants(&mLimits, wgpu::FeatureLevel::Core); - mFormatTable = BuildFormatTable(this); - - DeviceDescriptor desc = {}; - desc.deviceLostCallbackInfo = {nullptr, WGPUCallbackMode_AllowSpontaneous, nullptr, nullptr, - nullptr}; - mLostEvent = DeviceLostEvent::Create(&desc); - mLostEvent->mDevice = this; -} - DeviceBase::~DeviceBase() { // We need to explicitly release the Queue before we complete the destructor so that the // Queue does not get destroyed after the Device. @@ -464,13 +457,6 @@ // the lock. auto deviceGuard = GetGuard(); - // Set DeviceLostEvent to pass a null device to the callback (which may happen in Destroy() - // depending on the CallbackMode). This also makes DeviceLostEvent skip unregistering the - // UncapturedError and Logging callbacks; they'll be unregistered later in this function. - if (mLostEvent) { - mLostEvent->mDevice = nullptr; - } - // DeviceBase uses RefCountedWithExternalCount to break refcycles. // // DeviceBase holds multiple Refs to various API objects (pipelines, buffers, etc.) which @@ -503,10 +489,6 @@ // reference, it's UB if they try to get the queue from APIGetQueue(). mQueue = nullptr; - // Reset callbacks since after dropping the last external reference, the application may have - // freed any device-scope memory needed to run the callback. - mCallbackInfos.Clear(); - GetInstance()->RemoveDevice(this); // Once last external ref dropped, all callbacks should be forwarded to Instance's callback @@ -1930,9 +1912,12 @@ return mAdapter->APIGetInfo(adapterInfo); } -Future DeviceBase::APIGetLostFuture() const { +Future DeviceBase::APIGetLostFuture() { + if (mLostFuture.id != kNullFutureID) { + return mLostFuture; + } if (mLostEvent) { - return mLostEvent->GetFuture(); + mLostFuture = mLostEvent->GetFuture(); } DAWN_ASSERT(mLostFuture.id != kNullFutureID); return mLostFuture; @@ -2737,10 +2722,6 @@ return mIsolatedEntryPointName; } -void DeviceBase::ResetLostEvent() { - mLostEvent.Reset(); -} - IgnoreLazyClearCountScope::IgnoreLazyClearCountScope(DeviceBase* device) : mDevice(device), mLazyClearCountForTesting(device->mLazyClearCountForTesting) {}
diff --git a/src/dawn/native/Device.h b/src/dawn/native/Device.h index 0f7cc6f..85a257d 100644 --- a/src/dawn/native/Device.h +++ b/src/dawn/native/Device.h
@@ -307,7 +307,7 @@ void APIGetFeatures(wgpu::SupportedFeatures* features) const; void APIGetFeatures(SupportedFeatures* features) const; wgpu::Status APIGetAdapterInfo(AdapterInfo* adapterInfo) const; - Future APIGetLostFuture() const; + Future APIGetLostFuture(); void APIInjectError(wgpu::ErrorType type, StringView message); bool APITick(); void APIValidateTextureDescriptor(const TextureDescriptor* desc); @@ -489,15 +489,7 @@ tint::InternalCompilerErrorCallbackInfo GetTintInternalCompilerErrorCallback(); - // During adapter's creation of devices, if device initialization fails, adapter will trigger - // the device lost event. After that, adapter should reset the lost event to avoid double - // triggering it when the device object get destructed. - void ResetLostEvent(); - protected: - // Constructor used only for mocking and testing. - DeviceBase(); - void ForceEnableFeatureForTesting(Feature feature); MaybeError Initialize(const UnpackedPtr<DeviceDescriptor>& descriptor, @@ -511,12 +503,6 @@ DAWN_UNREACHABLE(); } - // Device lost event needs to be protected for now because mock device needs it. - // TODO(dawn:1702) Make this private and move the class in the implementation file when we mock - // the adapter. - Ref<DeviceLostEvent> mLostEvent = nullptr; - Future mLostFuture = {kNullFutureID}; - // Returns a pair of a filename and a boolean indicating whether to start tracing // and if so, what filename to save the trace under. std::pair<std::string, bool> GetTraceInfo(); @@ -632,6 +618,9 @@ const TextureCopy& dst, const Extent3D& copySizePixels) = 0; + Ref<DeviceLostEvent> mLostEvent = nullptr; + Future mLostFuture = {kNullFutureID}; + WGPUDeviceCallbackInfos mCallbackInfos; // Error scopes need to be thread local, but also need to be cleaned up when the device is
diff --git a/src/dawn/tests/unittests/native/mocks/DawnMockTest.cpp b/src/dawn/tests/unittests/native/mocks/DawnMockTest.cpp index cad3ac5..c6fb74d 100644 --- a/src/dawn/tests/unittests/native/mocks/DawnMockTest.cpp +++ b/src/dawn/tests/unittests/native/mocks/DawnMockTest.cpp
@@ -62,9 +62,10 @@ mDeviceToggles.SetForTesting(Toggle::AllowUnsafeAPIs, true, true); auto deviceMock = AcquireRef(new ::testing::NiceMock<DeviceMock>( - adapters[0].Get(), unpackedDesc, mDeviceToggles, std::move(lostEvent))); + adapters[0].Get(), unpackedDesc, mDeviceToggles, lostEvent.Get())); mDeviceMock = deviceMock.Get(); device = wgpu::Device::Acquire(ToAPI(ReturnToAPI<DeviceBase>(std::move(deviceMock)))); + instance->GetEventManager()->TrackEvent(std::move(lostEvent)); } void DawnMockTest::DropDevice() {
diff --git a/src/dawn/tests/unittests/native/mocks/DeviceMock.cpp b/src/dawn/tests/unittests/native/mocks/DeviceMock.cpp index 5205216..15d19b5 100644 --- a/src/dawn/tests/unittests/native/mocks/DeviceMock.cpp +++ b/src/dawn/tests/unittests/native/mocks/DeviceMock.cpp
@@ -129,7 +129,6 @@ QueueDescriptor desc = {}; EXPECT_FALSE( Initialize(descriptor, AcquireRef(new NiceMock<QueueMock>(this, &desc))).IsError()); - GetInstance()->GetEventManager()->TrackEvent(mLostEvent); } DeviceMock::~DeviceMock() = default;