Ensure all wire child objects are destroyed before their device

Destroying a device will implicit destroy all its child objects.
Attempting to use a child object after results in a fatal error.

Bug: dawn:384
Change-Id: I43c27c92cacde759be83cca79ac890f41bac3927
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/37002
Commit-Queue: Austin Eng <enga@chromium.org>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Jiawei Shao <jiawei.shao@intel.com>
This commit is contained in:
Austin Eng 2021-01-13 20:58:18 +00:00 committed by Commit Bot service account
parent 0562802757
commit 8ba0a01d1e
19 changed files with 240 additions and 28 deletions

View File

@ -33,20 +33,20 @@
{ "name": "write flush info", "type": "uint8_t", "annotation": "const*", "length": "write flush info length", "skip_serialize": true}
],
"device create buffer": [
{ "name": "device", "type": "device" },
{ "name": "device id", "type": "ObjectId" },
{ "name": "descriptor", "type": "buffer descriptor", "annotation": "const*" },
{ "name": "result", "type": "ObjectHandle", "handle_type": "buffer" },
{ "name": "handle create info length", "type": "uint64_t" },
{ "name": "handle create info", "type": "uint8_t", "annotation": "const*", "length": "handle create info length", "skip_serialize": true}
],
"device create ready compute pipeline": [
{ "name": "device", "type": "device" },
{ "name": "device id", "type": "ObjectId" },
{ "name": "request serial", "type": "uint64_t" },
{ "name": "pipeline object handle", "type": "ObjectHandle", "handle_type": "compute pipeline"},
{ "name": "descriptor", "type": "compute pipeline descriptor", "annotation": "const*"}
],
"device create ready render pipeline": [
{ "name": "device", "type": "device" },
{ "name": "device id", "type": "ObjectId" },
{ "name": "request serial", "type": "uint64_t" },
{ "name": "pipeline object handle", "type": "ObjectHandle", "handle_type": "render pipeline"},
{ "name": "descriptor", "type": "render pipeline descriptor", "annotation": "const*"}

View File

@ -32,7 +32,7 @@ namespace dawn_wire { namespace server {
protected:
void DestroyAllObjects(const DawnProcTable& procs) {
//* Free all objects when the server is destroyed
{% for type in by_category["object"] %}
{% for type in by_category["object"] if type.name.get() != "device" %}
{
std::vector<{{as_cType(type.name)}}> handles = mKnown{{type.name.CamelCase()}}.AcquireAllHandles();
for ({{as_cType(type.name)}} handle : handles) {
@ -40,6 +40,13 @@ namespace dawn_wire { namespace server {
}
}
{% endfor %}
//* Release devices last because dawn_native requires this.
{
std::vector<WGPUDevice> handles = mKnownDevice.AcquireAllHandles();
for (WGPUDevice handle : handles) {
procs.deviceRelease(handle);
}
}
}
{% for type in by_category["object"] %}

View File

@ -77,9 +77,28 @@ namespace dawn_wire { namespace server {
if (data == nullptr) {
return false;
}
if (data->device != nullptr) {
auto* device = static_cast<ObjectData<WGPUDevice>*>(data->device);
if (!UntrackDeviceChild(device, objectType, objectId)) {
return false;
}
}
{% if type.name.CamelCase() in server_reverse_lookup_objects %}
{{type.name.CamelCase()}}ObjectIdTable().Remove(data->handle);
{% endif %}
{% if type.name.get() == "device" %}
//* TODO(crbug.com/dawn/384): This is a hack to make sure that all child objects
//* are destroyed before their device. We should have a solution in
//* Dawn native that makes all child objects internally null if their
//* Device is destroyed.
while (data->childObjectTypesAndIds.size() > 0) {
ObjectType childObjectType;
ObjectId childObjectId;
std::tie(childObjectType, childObjectId) = UnpackObjectTypeAndId(
*data->childObjectTypesAndIds.begin());
DoDestroyObject(childObjectType, childObjectId);
}
{% endif %}
if (data->handle != nullptr) {
mProcs.{{as_varName(type.name, Name("release"))}}(data->handle);
}

View File

@ -17,7 +17,6 @@
namespace dawn_wire { namespace server {
{% for command in cmd_records["command"] %}
{% set type = command.derived_object %}
{% set method = command.derived_method %}
{% set is_method = method != None %}
{% set returns = is_method and method.return_type.name.canonical_case() != "void" %}
@ -53,6 +52,24 @@ namespace dawn_wire { namespace server {
return false;
}
{{name}}Data->generation = cmd.{{name}}.generation;
//* TODO(crbug.com/dawn/384): This is a hack to make sure that all child objects
//* are destroyed before their device. The dawn_native device needs to track all child objects so
//* it can destroy them if the device is destroyed first.
{% if command.derived_object %}
{% set type = command.derived_object %}
{% if type.name.get() == "device" %}
{{name}}Data->device = DeviceObjects().Get(cmd.selfId);
{% else %}
auto* selfData = {{type.name.CamelCase()}}Objects().Get(cmd.selfId);
{{name}}Data->device = selfData->device;
{% endif %}
if ({{name}}Data->device != nullptr) {
if (!TrackDeviceChild({{name}}Data->device, ObjectType::{{Type}}, cmd.{{name}}.id)) {
return false;
}
}
{% endif %}
{% endfor %}
//* Do command

View File

@ -32,8 +32,12 @@ namespace dawn_wire {
return mImpl->HandleCommands(commands, size);
}
bool WireServer::InjectTexture(WGPUTexture texture, uint32_t id, uint32_t generation) {
return mImpl->InjectTexture(texture, id, generation);
bool WireServer::InjectTexture(WGPUTexture texture,
uint32_t id,
uint32_t generation,
uint32_t deviceId,
uint32_t deviceGeneration) {
return mImpl->InjectTexture(texture, id, generation, deviceId, deviceGeneration);
}
namespace server {

View File

@ -66,7 +66,7 @@ namespace dawn_wire { namespace client {
buffer->mSize = descriptor->size;
DeviceCreateBufferCmd cmd;
cmd.device = ToAPI(device);
cmd.deviceId = device->id;
cmd.descriptor = descriptor;
cmd.result = ObjectHandle{buffer->id, bufferObjectAndSerial->generation};
cmd.handleCreateInfoLength = writeHandleCreateInfoLength;

View File

@ -59,6 +59,9 @@ namespace dawn_wire { namespace client {
void Client::DestroyAllObjects() {
for (auto& objectList : mObjects) {
ObjectType objectType = static_cast<ObjectType>(&objectList - mObjects.data());
if (objectType == ObjectType::Device) {
continue;
}
while (!objectList.empty()) {
ObjectBase* object = objectList.head()->value();
@ -69,6 +72,16 @@ namespace dawn_wire { namespace client {
FreeObject(objectType, object);
}
}
while (!mObjects[ObjectType::Device].empty()) {
ObjectBase* object = mObjects[ObjectType::Device].head()->value();
DestroyObjectCmd cmd;
cmd.objectType = ObjectType::Device;
cmd.objectId = object->id;
SerializeCommand(cmd);
FreeObject(ObjectType::Device, object);
}
}
WGPUDevice Client::GetDevice() {
@ -85,6 +98,8 @@ namespace dawn_wire { namespace client {
result.texture = ToAPI(allocation->object.get());
result.id = allocation->object->id;
result.generation = allocation->generation;
result.deviceId = FromAPI(device)->id;
result.deviceGeneration = DeviceAllocator().GetGeneration(FromAPI(device)->id);
return result;
}

View File

@ -215,7 +215,7 @@ namespace dawn_wire { namespace client {
}
DeviceCreateReadyComputePipelineCmd cmd;
cmd.device = ToAPI(this);
cmd.deviceId = this->id;
cmd.descriptor = descriptor;
uint64_t serial = mCreateReadyPipelineRequestSerial++;
@ -270,7 +270,7 @@ namespace dawn_wire { namespace client {
"GPU device disconnected", userdata);
}
DeviceCreateReadyRenderPipelineCmd cmd;
cmd.device = ToAPI(this);
cmd.deviceId = this->id;
cmd.descriptor = descriptor;
uint64_t serial = mCreateReadyPipelineRequestSerial++;

View File

@ -20,6 +20,7 @@
#include <algorithm>
#include <map>
#include <unordered_set>
namespace dawn_wire { namespace server {
@ -32,6 +33,8 @@ namespace dawn_wire { namespace server {
// Whether this object has been allocated, used by the KnownObjects queries
// TODO(cwallez@chromium.org): make this an internal bit vector in KnownObjects.
bool allocated;
ObjectDataBase<WGPUDevice>* device = nullptr;
};
// Stores what the backend knows about the type.
@ -48,6 +51,26 @@ namespace dawn_wire { namespace server {
BufferMapWriteState mapWriteState = BufferMapWriteState::Unmapped;
};
// Pack the ObjectType and ObjectId as a single value for storage in
// an std::unordered_set. This lets us avoid providing our own hash and
// equality comparison operators.
inline uint64_t PackObjectTypeAndId(ObjectType type, ObjectId id) {
static_assert(sizeof(ObjectType) * 8 <= 32, "");
static_assert(sizeof(ObjectId) * 8 <= 32, "");
return (static_cast<uint64_t>(type) << 32) + id;
}
inline std::pair<ObjectType, ObjectId> UnpackObjectTypeAndId(uint64_t payload) {
ObjectType type = static_cast<ObjectType>(payload >> 32);
ObjectId id = payload & 0xFFFFFFFF;
return std::make_pair(type, id);
}
template <>
struct ObjectData<WGPUDevice> : public ObjectDataBase<WGPUDevice> {
std::unordered_set<uint64_t> childObjectTypesAndIds;
};
// Keeps track of the mapping between client IDs and backend objects.
template <typename T>
class KnownObjects {

View File

@ -66,15 +66,29 @@ namespace dawn_wire { namespace server {
DestroyAllObjects(mProcs);
}
bool Server::InjectTexture(WGPUTexture texture, uint32_t id, uint32_t generation) {
bool Server::InjectTexture(WGPUTexture texture,
uint32_t id,
uint32_t generation,
uint32_t deviceId,
uint32_t deviceGeneration) {
ObjectData<WGPUDevice>* device = DeviceObjects().Get(deviceId);
if (device == nullptr || device->generation != deviceGeneration) {
return false;
}
ObjectData<WGPUTexture>* data = TextureObjects().Allocate(id);
if (data == nullptr) {
return false;
}
if (!TrackDeviceChild(device, ObjectType::Texture, id)) {
return false;
}
data->handle = texture;
data->generation = generation;
data->allocated = true;
data->device = device;
// The texture is externally owned so it shouldn't be destroyed when we receive a destroy
// message from the client. Add a reference to counterbalance the eventual release.
@ -83,4 +97,25 @@ namespace dawn_wire { namespace server {
return true;
}
bool TrackDeviceChild(ObjectDataBase<WGPUDevice>* device, ObjectType type, ObjectId id) {
auto it = static_cast<ObjectData<WGPUDevice>*>(device)->childObjectTypesAndIds.insert(
PackObjectTypeAndId(type, id));
if (!it.second) {
// An object of this type and id already exists.
return false;
}
return true;
}
bool UntrackDeviceChild(ObjectDataBase<WGPUDevice>* device, ObjectType type, ObjectId id) {
auto& children = static_cast<ObjectData<WGPUDevice>*>(device)->childObjectTypesAndIds;
auto it = children.find(PackObjectTypeAndId(type, id));
if (it == children.end()) {
// An object of this type and id was already deleted.
return false;
}
children.erase(it);
return true;
}
}} // namespace dawn_wire::server

View File

@ -161,7 +161,11 @@ namespace dawn_wire { namespace server {
const volatile char* HandleCommandsImpl(const volatile char* commands,
size_t size) override;
bool InjectTexture(WGPUTexture texture, uint32_t id, uint32_t generation);
bool InjectTexture(WGPUTexture texture,
uint32_t id,
uint32_t generation,
uint32_t deviceId,
uint32_t deviceGeneration);
template <typename T,
typename Enable = std::enable_if<std::is_base_of<CallbackUserdata, T>::value>>
@ -215,6 +219,9 @@ namespace dawn_wire { namespace server {
std::shared_ptr<bool> mIsAlive;
};
bool TrackDeviceChild(ObjectDataBase<WGPUDevice>* device, ObjectType type, ObjectId id);
bool UntrackDeviceChild(ObjectDataBase<WGPUDevice>* device, ObjectType type, ObjectId id);
std::unique_ptr<MemoryTransferService> CreateInlineMemoryTransferService();
}} // namespace dawn_wire::server

View File

@ -120,18 +120,27 @@ namespace dawn_wire { namespace server {
return true;
}
bool Server::DoDeviceCreateBuffer(WGPUDevice device,
bool Server::DoDeviceCreateBuffer(ObjectId deviceId,
const WGPUBufferDescriptor* descriptor,
ObjectHandle bufferResult,
uint64_t handleCreateInfoLength,
const uint8_t* handleCreateInfo) {
auto* device = DeviceObjects().Get(deviceId);
if (device == nullptr) {
return false;
}
// Create and register the buffer object.
auto* resultData = BufferObjects().Allocate(bufferResult.id);
if (resultData == nullptr) {
return false;
}
resultData->generation = bufferResult.generation;
resultData->handle = mProcs.deviceCreateBuffer(device, descriptor);
resultData->handle = mProcs.deviceCreateBuffer(device->handle, descriptor);
resultData->device = device;
if (!TrackDeviceChild(device, ObjectType::Buffer, bufferResult.id)) {
return false;
}
// If the buffer isn't mapped at creation, we are done.
if (!descriptor->mappedAtCreation) {

View File

@ -59,23 +59,32 @@ namespace dawn_wire { namespace server {
}
bool Server::DoDeviceCreateReadyComputePipeline(
WGPUDevice cDevice,
ObjectId deviceId,
uint64_t requestSerial,
ObjectHandle pipelineObjectHandle,
const WGPUComputePipelineDescriptor* descriptor) {
auto* device = DeviceObjects().Get(deviceId);
if (device == nullptr) {
return false;
}
auto* resultData = ComputePipelineObjects().Allocate(pipelineObjectHandle.id);
if (resultData == nullptr) {
return false;
}
resultData->generation = pipelineObjectHandle.generation;
resultData->device = device;
if (!TrackDeviceChild(device, ObjectType::ComputePipeline, pipelineObjectHandle.id)) {
return false;
}
auto userdata = MakeUserdata<CreateReadyPipelineUserData>();
userdata->requestSerial = requestSerial;
userdata->pipelineObjectID = pipelineObjectHandle.id;
mProcs.deviceCreateReadyComputePipeline(
cDevice, descriptor,
device->handle, descriptor,
ForwardToServer<decltype(&Server::OnCreateReadyComputePipelineCallback)>::Func<
&Server::OnCreateReadyComputePipelineCallback>(),
userdata.release());
@ -116,23 +125,32 @@ namespace dawn_wire { namespace server {
SerializeCommand(cmd);
}
bool Server::DoDeviceCreateReadyRenderPipeline(WGPUDevice cDevice,
bool Server::DoDeviceCreateReadyRenderPipeline(ObjectId deviceId,
uint64_t requestSerial,
ObjectHandle pipelineObjectHandle,
const WGPURenderPipelineDescriptor* descriptor) {
auto* device = DeviceObjects().Get(deviceId);
if (device == nullptr) {
return false;
}
auto* resultData = RenderPipelineObjects().Allocate(pipelineObjectHandle.id);
if (resultData == nullptr) {
return false;
}
resultData->generation = pipelineObjectHandle.generation;
resultData->device = device;
if (!TrackDeviceChild(device, ObjectType::RenderPipeline, pipelineObjectHandle.id)) {
return false;
}
auto userdata = MakeUserdata<CreateReadyPipelineUserData>();
userdata->requestSerial = requestSerial;
userdata->pipelineObjectID = pipelineObjectHandle.id;
mProcs.deviceCreateReadyRenderPipeline(
cDevice, descriptor,
device->handle, descriptor,
ForwardToServer<decltype(&Server::OnCreateReadyRenderPipelineCallback)>::Func<
&Server::OnCreateReadyRenderPipelineCallback>(),
userdata.release());

View File

@ -34,6 +34,8 @@ namespace dawn_wire {
WGPUTexture texture;
uint32_t id;
uint32_t generation;
uint32_t deviceId;
uint32_t deviceGeneration;
};
struct DAWN_WIRE_EXPORT WireClientDescriptor {

View File

@ -43,7 +43,12 @@ namespace dawn_wire {
const volatile char* HandleCommands(const volatile char* commands,
size_t size) override final;
bool InjectTexture(WGPUTexture texture, uint32_t id, uint32_t generation);
// TODO(enga): Remove defaults after updating Chrome.
bool InjectTexture(WGPUTexture texture,
uint32_t id,
uint32_t generation,
uint32_t deviceId = 1,
uint32_t deviceGeneration = 0);
private:
std::unique_ptr<server::Server> mImpl;

View File

@ -218,6 +218,7 @@ test("dawn_unittests") {
"unittests/wire/WireBasicTests.cpp",
"unittests/wire/WireBufferMappingTests.cpp",
"unittests/wire/WireCreateReadyPipelineTests.cpp",
"unittests/wire/WireDestroyObjectTests.cpp",
"unittests/wire/WireDisconnectTests.cpp",
"unittests/wire/WireErrorCallbackTests.cpp",
"unittests/wire/WireExtensionTests.cpp",

View File

@ -0,0 +1,45 @@
// Copyright 2021 The Dawn Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
#include "tests/unittests/wire/WireTest.h"
using namespace testing;
using namespace dawn_wire;
class WireDestroyObjectTests : public WireTest {};
// Test that destroying the device also destroys child objects.
TEST_F(WireDestroyObjectTests, DestroyDeviceDestroysChildren) {
WGPUCommandEncoder encoder = wgpuDeviceCreateCommandEncoder(device, nullptr);
WGPUCommandEncoder apiEncoder = api.GetNewCommandEncoder();
EXPECT_CALL(api, DeviceCreateCommandEncoder(apiDevice, nullptr)).WillOnce(Return(apiEncoder));
FlushClient();
// Release the device. It should cause the command encoder to be destroyed.
wgpuDeviceRelease(device);
Sequence s1, s2;
// The device and child objects should be released.
EXPECT_CALL(api, CommandEncoderRelease(apiEncoder)).InSequence(s1);
EXPECT_CALL(api, QueueRelease(apiQueue)).InSequence(s2);
EXPECT_CALL(api, DeviceRelease(apiDevice)).InSequence(s1, s2);
FlushClient();
// Using the command encoder should be an error.
wgpuCommandEncoderFinish(encoder, nullptr);
FlushClient(false);
}

View File

@ -145,9 +145,10 @@ TEST_F(WireDisconnectTests, DeleteClientDestroysObjects) {
DeleteClient();
// Expect release on all objects created by the client.
EXPECT_CALL(api, DeviceRelease(apiDevice)).Times(1);
EXPECT_CALL(api, QueueRelease(apiQueue)).Times(1);
EXPECT_CALL(api, CommandEncoderRelease(apiCommandEncoder)).Times(1);
EXPECT_CALL(api, SamplerRelease(apiSampler)).Times(1);
Sequence s1, s2, s3;
EXPECT_CALL(api, QueueRelease(apiQueue)).Times(1).InSequence(s1);
EXPECT_CALL(api, CommandEncoderRelease(apiCommandEncoder)).Times(1).InSequence(s2);
EXPECT_CALL(api, SamplerRelease(apiSampler)).Times(1).InSequence(s3);
EXPECT_CALL(api, DeviceRelease(apiDevice)).Times(1).InSequence(s1, s2, s3);
FlushClient();
}

View File

@ -34,7 +34,8 @@ TEST_F(WireInjectTextureTests, CallAfterReserveInject) {
WGPUTexture apiTexture = api.GetNewTexture();
EXPECT_CALL(api, TextureReference(apiTexture));
ASSERT_TRUE(GetWireServer()->InjectTexture(apiTexture, reservation.id, reservation.generation));
ASSERT_TRUE(GetWireServer()->InjectTexture(apiTexture, reservation.id, reservation.generation,
reservation.deviceId, reservation.deviceGeneration));
wgpuTextureCreateView(reservation.texture, nullptr);
WGPUTextureView apiDummyView = api.GetNewTextureView();
@ -57,11 +58,13 @@ TEST_F(WireInjectTextureTests, InjectExistingID) {
WGPUTexture apiTexture = api.GetNewTexture();
EXPECT_CALL(api, TextureReference(apiTexture));
ASSERT_TRUE(GetWireServer()->InjectTexture(apiTexture, reservation.id, reservation.generation));
ASSERT_TRUE(GetWireServer()->InjectTexture(apiTexture, reservation.id, reservation.generation,
reservation.deviceId, reservation.deviceGeneration));
// ID already in use, call fails.
ASSERT_FALSE(
GetWireServer()->InjectTexture(apiTexture, reservation.id, reservation.generation));
ASSERT_FALSE(GetWireServer()->InjectTexture(apiTexture, reservation.id, reservation.generation,
reservation.deviceId,
reservation.deviceGeneration));
}
// Test that the server only borrows the texture and does a single reference-release
@ -71,7 +74,8 @@ TEST_F(WireInjectTextureTests, InjectedTextureLifetime) {
// Injecting the texture adds a reference
WGPUTexture apiTexture = api.GetNewTexture();
EXPECT_CALL(api, TextureReference(apiTexture));
ASSERT_TRUE(GetWireServer()->InjectTexture(apiTexture, reservation.id, reservation.generation));
ASSERT_TRUE(GetWireServer()->InjectTexture(apiTexture, reservation.id, reservation.generation,
reservation.deviceId, reservation.deviceGeneration));
// Releasing the texture removes a single reference.
wgpuTextureRelease(reservation.texture);