DawnNative: implement Instance::ProcessEvents
Since https://dawn-review.googlesource.com/c/dawn/+/120940, callbacks
will be deferred to be executed in next device.APITick() instead of
immediately.
However, if the device is already destroyed (last ref dropped),
user/wire_server has no chance to call device.APITick() anymore, leading
to the callbacks waiting in queue forever.
This is also possibly the cause of memory leaks in cluserfuzz tests.
This CL attempt to fix it by implementing Instance::ProcessEvents():
In this method, every created device will invoke APITick() even if it is
already lost/externally released.
bug: chromium:1422507
bug: dawn:752
Change-Id: Iec69ad3b547a7e88c6e1a2225b13ad060a501a4f
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/123420
Commit-Queue: Quyen Le <lehoangquyen@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Austin Eng <enga@chromium.org>
diff --git a/dawn.json b/dawn.json
index 0d81e52..9306d64 100644
--- a/dawn.json
+++ b/dawn.json
@@ -1530,8 +1530,7 @@
]
},
{
- "name": "process events",
- "tags": ["upstream", "emscripten"]
+ "name": "process events"
},
{
"name": "request adapter",
diff --git a/include/dawn/native/DawnNative.h b/include/dawn/native/DawnNative.h
index 7e1315c..cf074cc 100644
--- a/include/dawn/native/DawnNative.h
+++ b/include/dawn/native/DawnNative.h
@@ -187,6 +187,8 @@
uint64_t GetDeviceCountForTesting() const;
+ bool ProcessEvents();
+
// Returns the underlying WGPUInstance object.
WGPUInstance Get() const;
diff --git a/src/dawn/fuzzers/DawnWireServerFuzzer.cpp b/src/dawn/fuzzers/DawnWireServerFuzzer.cpp
index 89f1a3c..53e5604 100644
--- a/src/dawn/fuzzers/DawnWireServerFuzzer.cpp
+++ b/src/dawn/fuzzers/DawnWireServerFuzzer.cpp
@@ -133,6 +133,10 @@
wireServer->InjectInstance(sInstance->Get(), 1, 0);
wireServer->HandleCommands(reinterpret_cast<const char*>(data), size);
+ // Flush remaining callbacks to avoid memory leaks.
+ do {
+ } while (sInstance->ProcessEvents());
+
// Note: Deleting the server will release all created objects.
// Deleted devices will wait for idle on destruction.
wireServer = nullptr;
diff --git a/src/dawn/native/DawnNative.cpp b/src/dawn/native/DawnNative.cpp
index 5bdf2a1..3e6ca72 100644
--- a/src/dawn/native/DawnNative.cpp
+++ b/src/dawn/native/DawnNative.cpp
@@ -251,6 +251,10 @@
return mImpl->GetDeviceCountForTesting();
}
+bool Instance::ProcessEvents() {
+ return mImpl->APIProcessEvents();
+}
+
WGPUInstance Instance::Get() const {
return ToAPI(mImpl);
}
diff --git a/src/dawn/native/Device.cpp b/src/dawn/native/Device.cpp
index 51beee5..d8b2eff 100644
--- a/src/dawn/native/Device.cpp
+++ b/src/dawn/native/Device.cpp
@@ -174,7 +174,6 @@
const DeviceDescriptor* descriptor,
const TogglesState& deviceToggles)
: mAdapter(adapter), mToggles(deviceToggles), mNextPipelineCompatibilityToken(1) {
- mAdapter->GetInstance()->IncrementDeviceCountForTesting();
ASSERT(descriptor != nullptr);
AdapterProperties adapterProperties;
@@ -218,8 +217,9 @@
// Queue does not get destroyed after the Device.
mQueue = nullptr;
// mAdapter is not set for mock test devices.
+ // TODO(crbug.com/dawn/1702): using a mock adapter could avoid the null checking.
if (mAdapter != nullptr) {
- mAdapter->GetInstance()->DecrementDeviceCountForTesting();
+ mAdapter->GetInstance()->RemoveDevice(this);
}
}
@@ -282,6 +282,11 @@
CreateShaderModule(&descriptor));
}
+ // TODO(crbug.com/dawn/1702): using a mock adapter could avoid the null checking.
+ if (mAdapter != nullptr) {
+ mAdapter->GetInstance()->AddDevice(this);
+ }
+
return {};
}
diff --git a/src/dawn/native/Instance.cpp b/src/dawn/native/Instance.cpp
index 7035c6f..63e3f1c 100644
--- a/src/dawn/native/Instance.cpp
+++ b/src/dawn/native/Instance.cpp
@@ -21,6 +21,7 @@
#include "dawn/common/Log.h"
#include "dawn/common/SystemUtils.h"
#include "dawn/native/ChainUtils_autogen.h"
+#include "dawn/native/Device.h"
#include "dawn/native/ErrorData.h"
#include "dawn/native/Surface.h"
#include "dawn/native/Toggles.h"
@@ -504,15 +505,35 @@
}
uint64_t InstanceBase::GetDeviceCountForTesting() const {
- return mDeviceCountForTesting.load();
+ std::lock_guard<std::mutex> lg(mDevicesListMutex);
+ return mDevicesList.size();
}
-void InstanceBase::IncrementDeviceCountForTesting() {
- mDeviceCountForTesting++;
+void InstanceBase::AddDevice(DeviceBase* device) {
+ std::lock_guard<std::mutex> lg(mDevicesListMutex);
+ mDevicesList.insert(device);
}
-void InstanceBase::DecrementDeviceCountForTesting() {
- mDeviceCountForTesting--;
+void InstanceBase::RemoveDevice(DeviceBase* device) {
+ std::lock_guard<std::mutex> lg(mDevicesListMutex);
+ mDevicesList.erase(device);
+}
+
+bool InstanceBase::APIProcessEvents() {
+ std::vector<Ref<DeviceBase>> devices;
+ {
+ std::lock_guard<std::mutex> lg(mDevicesListMutex);
+ for (auto device : mDevicesList) {
+ devices.push_back(device);
+ }
+ }
+
+ bool hasMoreEvents = false;
+ for (auto device : devices) {
+ hasMoreEvents = device->APITick() || hasMoreEvents;
+ }
+
+ return hasMoreEvents;
}
const std::vector<std::string>& InstanceBase::GetRuntimeSearchPaths() const {
diff --git a/src/dawn/native/Instance.h b/src/dawn/native/Instance.h
index 6ae291c..f00a95a 100644
--- a/src/dawn/native/Instance.h
+++ b/src/dawn/native/Instance.h
@@ -17,6 +17,8 @@
#include <array>
#include <memory>
+#include <mutex>
+#include <set>
#include <string>
#include <unordered_map>
#include <vector>
@@ -37,6 +39,7 @@
namespace dawn::native {
+class DeviceBase;
class Surface;
class XlibXcbFunctions;
@@ -103,8 +106,8 @@
BlobCache* GetBlobCache(bool enabled = true);
uint64_t GetDeviceCountForTesting() const;
- void IncrementDeviceCountForTesting();
- void DecrementDeviceCountForTesting();
+ void AddDevice(DeviceBase* device);
+ void RemoveDevice(DeviceBase* device);
const std::vector<std::string>& GetRuntimeSearchPaths() const;
@@ -113,6 +116,7 @@
// Dawn API
Surface* APICreateSurface(const SurfaceDescriptor* descriptor);
+ bool APIProcessEvents();
private:
explicit InstanceBase(const TogglesState& instanceToggles);
@@ -161,7 +165,8 @@
std::unique_ptr<XlibXcbFunctions> mXlibXcbFunctions;
#endif // defined(DAWN_USE_X11)
- std::atomic_uint64_t mDeviceCountForTesting{0};
+ std::set<DeviceBase*> mDevicesList;
+ mutable std::mutex mDevicesListMutex;
};
} // namespace dawn::native
diff --git a/src/dawn/tests/DawnTest.cpp b/src/dawn/tests/DawnTest.cpp
index 3765c44..04aa0aca 100644
--- a/src/dawn/tests/DawnTest.cpp
+++ b/src/dawn/tests/DawnTest.cpp
@@ -1439,12 +1439,12 @@
return EXPECT_TEXTURE_EQ(colorData.data(), colorTexture, {0, 0}, {width, height});
}
-void DawnTestBase::WaitABit(wgpu::Device targetDevice) {
- if (targetDevice == nullptr) {
- targetDevice = this->device;
+void DawnTestBase::WaitABit(wgpu::Instance targetInstance) {
+ if (targetInstance == nullptr) {
+ targetInstance = mInstance;
}
- if (targetDevice != nullptr) {
- targetDevice.Tick();
+ if (targetInstance != nullptr) {
+ targetInstance.ProcessEvents();
}
FlushWire();
@@ -1498,8 +1498,6 @@
// immediately.
mNumPendingMapOperations = mReadbackSlots.size();
- std::vector<wgpu::Device> pendingDevices;
-
// Map all readback slots
for (size_t i = 0; i < mReadbackSlots.size(); ++i) {
MapReadUserdata* userdata = new MapReadUserdata{this, i};
@@ -1507,15 +1505,11 @@
const ReadbackSlot& slot = mReadbackSlots[i];
slot.buffer.MapAsync(wgpu::MapMode::Read, 0, wgpu::kWholeMapSize, SlotMapCallback,
userdata);
-
- pendingDevices.push_back(slot.device);
}
// Busy wait until all map operations are done.
while (mNumPendingMapOperations != 0) {
- for (const wgpu::Device& device : pendingDevices) {
- WaitABit(device);
- }
+ WaitABit();
}
}
diff --git a/src/dawn/tests/DawnTest.h b/src/dawn/tests/DawnTest.h
index b41aee9..a62e112 100644
--- a/src/dawn/tests/DawnTest.h
+++ b/src/dawn/tests/DawnTest.h
@@ -550,7 +550,7 @@
mipLevel, {}, &expectedStencil);
}
- void WaitABit(wgpu::Device = nullptr);
+ void WaitABit(wgpu::Instance = nullptr);
void FlushWire();
void WaitForAllOperations();
diff --git a/src/dawn/tests/end2end/DeviceLifetimeTests.cpp b/src/dawn/tests/end2end/DeviceLifetimeTests.cpp
index f1c3a5e..ad760fb 100644
--- a/src/dawn/tests/end2end/DeviceLifetimeTests.cpp
+++ b/src/dawn/tests/end2end/DeviceLifetimeTests.cpp
@@ -189,10 +189,7 @@
// Test that the device can be dropped before a buffer created from it, then mapping the buffer
// fails.
-// TODO(crbug.com/dawn/752): Re-enable this test once we implement Instance.ProcessEvents().
-// Currently the callbacks are called inside Device.Tick() only. However, since we drop the device,
-// there is no way to call Device.Tick() anymore.
-TEST_P(DeviceLifetimeTests, DISABLED_DroppedThenMapBuffer) {
+TEST_P(DeviceLifetimeTests, DroppedThenMapBuffer) {
wgpu::BufferDescriptor desc = {};
desc.size = 4;
desc.usage = wgpu::BufferUsage::MapRead | wgpu::BufferUsage::CopyDst;
@@ -481,6 +478,36 @@
}
}
+// Tests that dropping 2nd device inside 1st device's callback triggered by instance.ProcessEvents
+// won't crash.
+TEST_P(DeviceLifetimeTests, DropDevice2InProcessEvents) {
+ wgpu::Device device2 = CreateDevice();
+
+ struct UserData {
+ wgpu::Device device2;
+ bool done = false;
+ } userdata;
+
+ userdata.device2 = std::move(device2);
+
+ device.PushErrorScope(wgpu::ErrorFilter::Validation);
+
+ // The following callback will drop the 2nd device. It won't be triggered until
+ // instance.ProcessEvents() is called.
+ device.PopErrorScope(
+ [](WGPUErrorType type, const char*, void* userdataPtr) {
+ auto userdata = static_cast<UserData*>(userdataPtr);
+
+ userdata->device2 = nullptr;
+ userdata->done = true;
+ },
+ &userdata);
+
+ while (!userdata.done) {
+ WaitABit();
+ }
+}
+
DAWN_INSTANTIATE_TEST(DeviceLifetimeTests,
D3D12Backend(),
MetalBackend(),
diff --git a/src/dawn/tests/unittests/wire/WireTest.cpp b/src/dawn/tests/unittests/wire/WireTest.cpp
index 4a9f0d6..5f4fbea 100644
--- a/src/dawn/tests/unittests/wire/WireTest.cpp
+++ b/src/dawn/tests/unittests/wire/WireTest.cpp
@@ -150,5 +150,6 @@
}
void WireTest::SetupIgnoredCallExpectations() {
+ EXPECT_CALL(api, InstanceProcessEvents(_)).Times(AnyNumber());
EXPECT_CALL(api, DeviceTick(_)).Times(AnyNumber());
}
