Revert "[WGPUFuture] Update buffer mapping in wire to keep reference to Buffer."
This reverts commit b445b0be7f620f1a662b7d835adbf59675fb45ea.
Reason for revert: Broke CI because raced submit with https://dawn-review.googlesource.com/c/dawn/+/166220
Original change's description:
> [WGPUFuture] Update buffer mapping in wire to keep reference to Buffer.
>
> - Also updates buffer mapping on the wire to be more consistent with
> other async future functions.
> - Note that buffer release no longer causes map async callbacks to
> trigger since the event holds a reference to the buffer now.
>
> Bug: dawn:2061
> Change-Id: Ifd0d0588f61ca531f0e75d1dd4082a3dc3e257a4
> Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/168800
> Commit-Queue: Loko Kung <lokokung@google.com>
> Reviewed-by: Kai Ninomiya <kainino@chromium.org>
> Reviewed-by: Corentin Wallez <cwallez@chromium.org>
> Kokoro: Kokoro <noreply+kokoro@google.com>
TBR=cwallez@chromium.org,kainino@chromium.org,noreply+kokoro@google.com,dawn-scoped@luci-project-accounts.iam.gserviceaccount.com,lokokung@google.com
Change-Id: Icae2acf26efd641b77c097936b5313529da5cc47
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: dawn:2061
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/170581
Reviewed-by: Loko Kung <lokokung@google.com>
Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
Kokoro: Loko Kung <lokokung@google.com>
Commit-Queue: Loko Kung <lokokung@google.com>
diff --git a/src/dawn/dawn_wire.json b/src/dawn/dawn_wire.json
index 5e92ad7..f37d6ac 100644
--- a/src/dawn/dawn_wire.json
+++ b/src/dawn/dawn_wire.json
@@ -116,9 +116,10 @@
},
"return commands": {
"buffer map async callback": [
+ { "name": "buffer", "type": "ObjectHandle", "handle_type": "buffer", "_comment": "TODO(dawn:2061) Remove this field once mapping is updated." },
{ "name": "event manager", "type": "ObjectHandle" },
{ "name": "future", "type": "future" },
- { "name": "status", "type": "buffer map async status" },
+ { "name": "status", "type": "uint32_t" },
{ "name": "read data update info length", "type": "uint64_t" },
{ "name": "read data update info", "type": "uint8_t", "annotation": "const*", "length": "read data update info length", "skip_serialize": true }
],
diff --git a/src/dawn/tests/unittests/wire/WireBufferMappingTests.cpp b/src/dawn/tests/unittests/wire/WireBufferMappingTests.cpp
index d72f166..c75ea3a 100644
--- a/src/dawn/tests/unittests/wire/WireBufferMappingTests.cpp
+++ b/src/dawn/tests/unittests/wire/WireBufferMappingTests.cpp
@@ -303,6 +303,22 @@
WGPUBufferMapAsyncStatus_DestroyedBeforeCallback);
}
+// Check the map callback is called with "DestroyedBeforeCallback" when the map request would have
+// worked, but Release() was called on the last ref.
+TEST_P(WireBufferMappingTests, ReleaseCalledTooEarly) {
+ TestEarlyMapCancelled(
+ &wgpuBufferRelease, [&]() { EXPECT_CALL(api, BufferRelease(apiBuffer)); },
+ WGPUBufferMapAsyncStatus_DestroyedBeforeCallback);
+}
+
+// Check that if Release() was called early client-side on the last ref, we disregard server-side
+// validation errors.
+TEST_P(WireBufferMappingTests, ReleaseCalledTooEarlyServerSideError) {
+ TestEarlyMapErrorCancelled(
+ &wgpuBufferRelease, [&]() { EXPECT_CALL(api, BufferRelease(apiBuffer)); },
+ WGPUBufferMapAsyncStatus_DestroyedBeforeCallback);
+}
+
// Test that the callback isn't fired twice when Unmap() is called inside the callback.
TEST_P(WireBufferMappingTests, UnmapInsideMapCallback) {
TestCancelInCallback(&wgpuBufferUnmap, [&]() { EXPECT_CALL(api, BufferUnmap(apiBuffer)); });
@@ -815,6 +831,52 @@
}
}
+#if defined(DAWN_ENABLE_ASSERTS)
+// Test that request inside user callbacks after object release is called.
+TEST_P(WireBufferMappingTests, MapInsideCallbackAfterRelease) {
+ WGPUMapMode mapMode = GetMapMode();
+ SetupBuffer(mapMode);
+ BufferMapAsync(buffer, mapMode, 0, kBufferSize, nullptr);
+
+ uint32_t bufferContent = 31337;
+ EXPECT_CALL(api, OnBufferMapAsync(apiBuffer, mapMode, 0, kBufferSize, _, _))
+ .WillOnce(InvokeWithoutArgs(
+ [&] { api.CallBufferMapAsyncCallback(apiBuffer, WGPUBufferMapAsyncStatus_Success); }));
+ ExpectMappedRangeCall(kBufferSize, &bufferContent);
+ EXPECT_CALL(api, BufferRelease(apiBuffer));
+
+ // By releasing the buffer the refcount reaches zero and pending map async
+ // should fail with destroyed before callback status.
+ if (IsSpontaneous()) {
+ // In spontaneous mode, the callback gets called as a part of the release.
+ ExpectWireCallbacksWhen([&](auto& mockCb) {
+ EXPECT_CALL(mockCb, Call(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback, _))
+ .WillOnce([&]() {
+ ASSERT_DEATH_IF_SUPPORTED(
+ { BufferMapAsync(buffer, mapMode, 0, kBufferSize, nullptr); }, "");
+ });
+
+ wgpuBufferRelease(buffer);
+ });
+ FlushClient();
+ FlushCallbacks();
+ } else {
+ // Otherwise, the callback happens when we flush it.
+ wgpuBufferRelease(buffer);
+ FlushClient();
+ ExpectWireCallbacksWhen([&](auto& mockCb) {
+ EXPECT_CALL(mockCb, Call(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback, _))
+ .WillOnce([&]() {
+ ASSERT_DEATH_IF_SUPPORTED(
+ { BufferMapAsync(buffer, mapMode, 0, kBufferSize, nullptr); }, "");
+ });
+
+ FlushCallbacks();
+ });
+ }
+}
+#endif // defined(DAWN_ENABLE_ASSERTS)
+
// Test that requests inside user callbacks before disconnect are called.
TEST_P(WireBufferMappingTests, MapInsideCallbackBeforeDisconnect) {
WGPUMapMode mapMode = GetMapMode();
@@ -844,8 +906,8 @@
});
}
-// Test that requests inside user callbacks before buffer destroy are called.
-TEST_P(WireBufferMappingTests, MapInsideCallbackBeforeDestroy) {
+// Test that requests inside user callbacks before object release are called.
+TEST_P(WireBufferMappingTests, MapInsideCallbackBeforeRelease) {
WGPUMapMode mapMode = GetMapMode();
SetupBuffer(mapMode);
BufferMapAsync(buffer, mapMode, 0, kBufferSize, nullptr);
@@ -863,7 +925,7 @@
if (IsSpontaneous()) {
// In spontaneous mode, when the success callback fires, the first MapAsync request
// generated by the callback is queued, then all subsequent requests' callbacks are
- // immediately called with MappingAlreadyPending. Finally, when we call Destroy, the queued
+ // immediately called with MappingAlreadyPending. Finally, when we call Release, the queued
// request's callback is then called with DestroyedBeforeCallback.
ExpectWireCallbacksWhen([&](auto& mockCb) {
EXPECT_CALL(mockCb, Call(WGPUBufferMapAsyncStatus_Success, _)).WillOnce([&]() {
@@ -879,13 +941,13 @@
ExpectWireCallbacksWhen([&](auto& mockCb) {
EXPECT_CALL(mockCb, Call(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback, _)).Times(1);
- wgpuBufferDestroy(buffer);
+ wgpuBufferRelease(buffer);
});
FlushCallbacks();
} else {
// In non-spontaneous modes, the first callback doesn't trigger any other immediate
// callbacks, but internally, all but the first MapAsync call's callback is set to be ready
- // with MappingAlreadyPending. When we call Destroy, the first pending request is then
+ // with MappingAlreadyPending. When we call Release, the first pending request is then
// marked ready with DestroyedBeforeCallback. The callbacks all run when we flush them.
ExpectWireCallbacksWhen([&](auto& mockCb) {
EXPECT_CALL(mockCb, Call(WGPUBufferMapAsyncStatus_Success, _)).WillOnce([&]() {
@@ -896,7 +958,7 @@
FlushCallbacks();
});
- wgpuBufferDestroy(buffer);
+ wgpuBufferRelease(buffer);
ExpectWireCallbacksWhen([&](auto& mockCb) {
EXPECT_CALL(mockCb, Call(WGPUBufferMapAsyncStatus_MappingAlreadyPending, _))
.Times(kNumRequests - 1);
diff --git a/src/dawn/tests/unittests/wire/WireMemoryTransferServiceTests.cpp b/src/dawn/tests/unittests/wire/WireMemoryTransferServiceTests.cpp
index 79ea2b8..e038b01 100644
--- a/src/dawn/tests/unittests/wire/WireMemoryTransferServiceTests.cpp
+++ b/src/dawn/tests/unittests/wire/WireMemoryTransferServiceTests.cpp
@@ -526,7 +526,7 @@
// Failed deserialization is a fatal failure and the client synchronously receives a
// DEVICE_LOST callback.
- EXPECT_CALL(*mockBufferMapCallback, Call(WGPUBufferMapAsyncStatus_Unknown, _)).Times(1);
+ EXPECT_CALL(*mockBufferMapCallback, Call(WGPUBufferMapAsyncStatus_DeviceLost, _)).Times(1);
FlushServer(false);
diff --git a/src/dawn/wire/client/Buffer.cpp b/src/dawn/wire/client/Buffer.cpp
index 238f19f..4dcf340 100644
--- a/src/dawn/wire/client/Buffer.cpp
+++ b/src/dawn/wire/client/Buffer.cpp
@@ -40,6 +40,7 @@
namespace dawn::wire::client {
namespace {
+
WGPUBuffer CreateErrorBufferOOMAtClient(Device* device, const WGPUBufferDescriptor* descriptor) {
if (descriptor->mappedAtCreation) {
return nullptr;
@@ -51,112 +52,23 @@
errorBufferDescriptor.nextInChain = &errorInfo.chain;
return GetProcs().deviceCreateErrorBuffer(ToAPI(device), &errorBufferDescriptor);
}
-} // anonymous namespace
-class Buffer::MapAsyncEvent : public TrackedEvent {
+class MapAsyncEvent : public TrackedEvent {
public:
static constexpr EventType kType = EventType::MapAsync;
- MapAsyncEvent(const WGPUBufferMapCallbackInfo& callbackInfo, Buffer* buffer)
+ explicit MapAsyncEvent(const WGPUBufferMapCallbackInfo& callbackInfo,
+ const Ref<MapStateData>& mapStateData)
: TrackedEvent(callbackInfo.mode),
mCallback(callbackInfo.callback),
mUserdata(callbackInfo.userdata),
- mBuffer(buffer) {
- DAWN_ASSERT(buffer != nullptr);
- GetProcs().bufferReference(ToAPI(mBuffer));
+ mMapStateData(mapStateData) {
+ DAWN_ASSERT(mMapStateData.Get() != nullptr);
}
- ~MapAsyncEvent() override { GetProcs().bufferRelease(ToAPI(mBuffer)); }
-
EventType GetType() override { return kType; }
- bool IsPendingRequest(FutureID futureID) {
- return mBuffer->mPendingMapRequest && mBuffer->mPendingMapRequest->futureID == futureID;
- }
-
- WireResult ReadyHook(FutureID futureID,
- WGPUBufferMapAsyncStatus status,
- uint64_t readDataUpdateInfoLength = 0,
- const uint8_t* readDataUpdateInfo = nullptr) {
- auto FailRequest = [this]() -> WireResult {
- mStatus = WGPUBufferMapAsyncStatus_Unknown;
- return WireResult::FatalError;
- };
-
- // Handling for different statuses.
- switch (status) {
- case WGPUBufferMapAsyncStatus_MappingAlreadyPending: {
- DAWN_ASSERT(!IsPendingRequest(futureID));
- mStatus = status;
- break;
- }
-
- // For client-side rejection errors, we clear the pending request now since they always
- // take precedence.
- case WGPUBufferMapAsyncStatus_DestroyedBeforeCallback:
- case WGPUBufferMapAsyncStatus_UnmappedBeforeCallback: {
- mStatus = status;
- mBuffer->mPendingMapRequest = std::nullopt;
- break;
- }
-
- case WGPUBufferMapAsyncStatus_Success: {
- if (!IsPendingRequest(futureID)) {
- // If a success occurs (which must come from the server), but it does not
- // correspond to the pending request, the pending request must have been
- // rejected early and hence the status must be set.
- DAWN_ASSERT(mStatus);
- break;
- }
- mStatus = status;
- auto& pending = mBuffer->mPendingMapRequest.value();
- if (!pending.type) {
- return FailRequest();
- }
- switch (*pending.type) {
- case MapRequestType::Read: {
- if (readDataUpdateInfoLength > std::numeric_limits<size_t>::max()) {
- // This is the size of data deserialized from the command stream, which
- // must be CPU-addressable.
- return FailRequest();
- }
-
- // Validate to prevent bad map request; buffer destroyed during map request
- if (mBuffer->mReadHandle == nullptr) {
- return FailRequest();
- }
- // Update user map data with server returned data
- if (!mBuffer->mReadHandle->DeserializeDataUpdate(
- readDataUpdateInfo, static_cast<size_t>(readDataUpdateInfoLength),
- pending.offset, pending.size)) {
- return FailRequest();
- }
- mBuffer->mMappedData = const_cast<void*>(mBuffer->mReadHandle->GetData());
- break;
- }
- case MapRequestType::Write: {
- if (mBuffer->mWriteHandle == nullptr) {
- return FailRequest();
- }
- mBuffer->mMappedData = mBuffer->mWriteHandle->GetData();
- break;
- }
- }
- mBuffer->mMappedOffset = pending.offset;
- mBuffer->mMappedSize = pending.size;
- break;
- }
-
- // All other statuses are server-side status.
- default: {
- if (!IsPendingRequest(futureID)) {
- break;
- }
- mStatus = status;
- }
- }
- return WireResult::Success;
- }
+ void ReadyHook(WGPUBufferMapAsyncStatus status) { mStatus = status; }
private:
void CompleteImpl(FutureID futureID, EventCompletionType completionType) override {
@@ -166,38 +78,24 @@
if (mStatus) {
status = *mStatus;
}
-
- auto Callback = [this, &status]() {
- if (mCallback) {
- mCallback(status, mUserdata);
+ if (mMapStateData->pendingRequest && futureID == mMapStateData->pendingRequest->futureID) {
+ if (status == WGPUBufferMapAsyncStatus_Success) {
+ switch (mMapStateData->pendingRequest->type) {
+ case MapRequestType::Read:
+ mMapStateData->mapState = MapState::MappedForRead;
+ break;
+ case MapRequestType::Write:
+ mMapStateData->mapState = MapState::MappedForWrite;
+ break;
+ default:
+ DAWN_UNREACHABLE();
+ }
}
- };
-
- if (!IsPendingRequest(futureID)) {
- DAWN_ASSERT(status != WGPUBufferMapAsyncStatus_Success);
- return Callback();
+ mMapStateData->pendingRequest = std::nullopt;
}
-
- if (status == WGPUBufferMapAsyncStatus_Success) {
- if (mBuffer->mIsDeviceAlive.expired()) {
- // If the device lost its last ref before this callback was resolved, we want to
- // overwrite the status. This is necessary because otherwise dropping the last
- // device reference could race w.r.t what this callback would see.
- status = WGPUBufferMapAsyncStatus_DestroyedBeforeCallback;
- return Callback();
- }
- DAWN_ASSERT(mBuffer->mPendingMapRequest->type);
- switch (*mBuffer->mPendingMapRequest->type) {
- case MapRequestType::Read:
- mBuffer->mMappedState = MapState::MappedForRead;
- break;
- case MapRequestType::Write:
- mBuffer->mMappedState = MapState::MappedForWrite;
- break;
- }
+ if (mCallback) {
+ mCallback(status, mUserdata);
}
- mBuffer->mPendingMapRequest = std::nullopt;
- return Callback();
}
WGPUBufferMapCallback mCallback;
@@ -206,10 +104,12 @@
std::optional<WGPUBufferMapAsyncStatus> mStatus;
- // Strong reference to the buffer so that when we call the callback we can pass the buffer.
- Buffer* const mBuffer;
+ // Shared data with the Buffer that may be modified between event handling and user inputs.
+ Ref<MapStateData> mMapStateData;
};
+} // anonymous namespace
+
// static
WGPUBuffer Buffer::Create(Device* device, const WGPUBufferDescriptor* descriptor) {
Client* wireClient = device->GetClient();
@@ -262,15 +162,20 @@
// This must happen after any potential error buffer creation
// as server expects allocating ids to be monotonically increasing
Buffer* buffer = wireClient->Make<Buffer>(device->GetEventManagerHandle(), descriptor);
- buffer->mIsDeviceAlive = device->GetAliveWeakPtr();
+ buffer->mDestructWriteHandleOnUnmap = false;
if (descriptor->mappedAtCreation) {
// If the buffer is mapped at creation, a write handle is created and will be
// destructed on unmap if the buffer doesn't have MapWrite usage
// The buffer is mapped right now.
- buffer->mMappedState = MapState::MappedAtCreation;
- buffer->mMappedOffset = 0;
- buffer->mMappedSize = buffer->mSize;
+ buffer->mMapStateData->mapState = MapState::MappedAtCreation;
+
+ // This flag is for write handle created by mappedAtCreation
+ // instead of MapWrite usage. We don't have such a case for read handle
+ buffer->mDestructWriteHandleOnUnmap = (descriptor->usage & WGPUBufferUsage_MapWrite) == 0;
+
+ buffer->mMapOffset = 0;
+ buffer->mMapSize = buffer->mSize;
DAWN_ASSERT(writeHandle != nullptr);
buffer->mMappedData = writeHandle->GetData();
}
@@ -306,23 +211,32 @@
const ObjectHandle& eventManagerHandle,
const WGPUBufferDescriptor* descriptor)
: ObjectWithEventsBase(params, eventManagerHandle),
+ mMapStateData(AcquireRef(new MapStateData{})),
mSize(descriptor->size),
- mUsage(static_cast<WGPUBufferUsage>(descriptor->usage)),
- // This flag is for the write handle created by mappedAtCreation
- // instead of MapWrite usage. We don't have such a case for read handle.
- mDestructWriteHandleOnUnmap(descriptor->mappedAtCreation &&
- ((descriptor->usage & WGPUBufferUsage_MapWrite) == 0)) {}
+ mUsage(static_cast<WGPUBufferUsage>(descriptor->usage)) {}
Buffer::~Buffer() {
FreeMappedData();
+ SetFutureStatusAndClearPending(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback);
}
-void Buffer::SetFutureStatus(WGPUBufferMapAsyncStatus status) {
- if (!mPendingMapRequest) {
+bool Buffer::SetFutureStatus(WGPUBufferMapAsyncStatus status) {
+ DAWN_ASSERT(mMapStateData->pendingRequest);
+ return GetEventManager().SetFutureReady<MapAsyncEvent>(mMapStateData->pendingRequest->futureID,
+ status) == WireResult::Success;
+}
+
+void Buffer::SetFutureStatusAndClearPending(WGPUBufferMapAsyncStatus status) {
+ if (!mMapStateData->pendingRequest) {
+ // Since this is unconditionally called on destruction, we might not have a pending map
+ // request all the time.
return;
}
- DAWN_CHECK(GetEventManager().SetFutureReady<MapAsyncEvent>(mPendingMapRequest->futureID,
- status) == WireResult::Success);
+
+ FutureID futureID = mMapStateData->pendingRequest->futureID;
+ mMapStateData->pendingRequest = std::nullopt;
+ DAWN_UNUSED(GetEventManager().SetFutureReady<MapAsyncEvent>(futureID, status));
+ return;
}
void Buffer::MapAsync(WGPUMapModeFlags mode,
@@ -345,12 +259,12 @@
Client* client = GetClient();
auto [futureIDInternal, tracked] =
- GetEventManager().TrackEvent(std::make_unique<MapAsyncEvent>(callbackInfo, this));
+ GetEventManager().TrackEvent(std::make_unique<MapAsyncEvent>(callbackInfo, mMapStateData));
if (!tracked) {
return {futureIDInternal};
}
- if (mPendingMapRequest) {
+ if (mMapStateData->pendingRequest) {
DAWN_UNUSED(GetEventManager().SetFutureReady<MapAsyncEvent>(
futureIDInternal, WGPUBufferMapAsyncStatus_MappingAlreadyPending));
return {futureIDInternal};
@@ -362,14 +276,14 @@
}
// Set up the request structure that will hold information while this mapping is in flight.
- std::optional<MapRequestType> mapMode;
+ MapRequestType mapMode = MapRequestType::None;
if (mode & WGPUMapMode_Read) {
mapMode = MapRequestType::Read;
} else if (mode & WGPUMapMode_Write) {
mapMode = MapRequestType::Write;
}
- mPendingMapRequest = {futureIDInternal, offset, size, mapMode};
+ mMapStateData->pendingRequest = {futureIDInternal, offset, size, mapMode};
// Serialize the command to send to the server.
BufferMapAsyncCmd cmd;
@@ -384,14 +298,63 @@
return {futureIDInternal};
}
-bool Client::DoBufferMapAsyncCallback(ObjectHandle eventManager,
- WGPUFuture future,
- WGPUBufferMapAsyncStatus status,
- uint64_t readDataUpdateInfoLength,
- const uint8_t* readDataUpdateInfo) {
- return GetEventManager(eventManager)
- .SetFutureReady<Buffer::MapAsyncEvent>(future.id, status, readDataUpdateInfoLength,
- readDataUpdateInfo) == WireResult::Success;
+bool Buffer::OnMapAsyncCallback(WGPUFuture future,
+ uint32_t status,
+ uint64_t readDataUpdateInfoLength,
+ const uint8_t* readDataUpdateInfo) {
+ // Check that the response doesn't correspond to a request that has already been rejected by
+ // unmap or destroy.
+ if (!mMapStateData->pendingRequest) {
+ return true;
+ }
+ MapRequestData& pendingRequest = mMapStateData->pendingRequest.value();
+ if (pendingRequest.futureID != future.id) {
+ return true;
+ }
+
+ auto FailRequest = [this]() -> bool {
+ SetFutureStatus(WGPUBufferMapAsyncStatus_DeviceLost);
+ return false;
+ };
+
+ if (status == WGPUBufferMapAsyncStatus_Success) {
+ switch (pendingRequest.type) {
+ case MapRequestType::Read: {
+ if (readDataUpdateInfoLength > std::numeric_limits<size_t>::max()) {
+ // This is the size of data deserialized from the command stream, which must
+ // be CPU-addressable.
+ return FailRequest();
+ }
+
+ // Validate to prevent bad map request; buffer destroyed during map request
+ if (mReadHandle == nullptr) {
+ return FailRequest();
+ }
+ // Update user map data with server returned data
+ if (!mReadHandle->DeserializeDataUpdate(
+ readDataUpdateInfo, static_cast<size_t>(readDataUpdateInfoLength),
+ pendingRequest.offset, pendingRequest.size)) {
+ return FailRequest();
+ }
+ mMappedData = const_cast<void*>(mReadHandle->GetData());
+ break;
+ }
+ case MapRequestType::Write: {
+ if (mWriteHandle == nullptr) {
+ return FailRequest();
+ }
+ mMappedData = mWriteHandle->GetData();
+ break;
+ }
+ default:
+ DAWN_UNREACHABLE();
+ }
+
+ mMapOffset = pendingRequest.offset;
+ mMapSize = pendingRequest.size;
+ }
+
+ return SetFutureStatus(static_cast<WGPUBufferMapAsyncStatus>(status));
}
void* Buffer::GetMappedRange(size_t offset, size_t size) {
@@ -420,20 +383,23 @@
// - Server -> Client: Result of MapRequest2
Client* client = GetClient();
- if (IsMappedForWriting()) {
+ // mWriteHandle can still be nullptr if buffer has been destroyed before unmap
+ if ((mMapStateData->mapState == MapState::MappedForWrite ||
+ mMapStateData->mapState == MapState::MappedAtCreation) &&
+ mWriteHandle != nullptr) {
// Writes need to be flushed before Unmap is sent. Unmap calls all associated
// in-flight callbacks which may read the updated data.
// Get the serialization size of data update writes.
size_t writeDataUpdateInfoLength =
- mWriteHandle->SizeOfSerializeDataUpdate(mMappedOffset, mMappedSize);
+ mWriteHandle->SizeOfSerializeDataUpdate(mMapOffset, mMapSize);
BufferUpdateMappedDataCmd cmd;
cmd.bufferId = GetWireId();
cmd.writeDataUpdateInfoLength = writeDataUpdateInfoLength;
cmd.writeDataUpdateInfo = nullptr;
- cmd.offset = mMappedOffset;
- cmd.size = mMappedSize;
+ cmd.offset = mMapOffset;
+ cmd.size = mMapSize;
client->SerializeCommand(
cmd, CommandExtension{writeDataUpdateInfoLength, [&](char* writeHandleBuffer) {
@@ -446,11 +412,11 @@
// If mDestructWriteHandleOnUnmap is true, that means the write handle is merely
// for mappedAtCreation usage. It is destroyed on unmap after flush to server
// instead of at buffer destruction.
- if (mDestructWriteHandleOnUnmap) {
+ if (mMapStateData->mapState == MapState::MappedAtCreation && mDestructWriteHandleOnUnmap) {
mWriteHandle = nullptr;
if (mReadHandle) {
// If it's both mappedAtCreation and MapRead we need to reset
- // mData to readHandle's GetData(). This could be changed to
+ // mMappedData to readHandle's GetData(). This could be changed to
// merging read/write handle in future
mMappedData = const_cast<void*>(mReadHandle->GetData());
}
@@ -458,15 +424,15 @@
}
// Free map access tokens
- mMappedState = MapState::Unmapped;
- mMappedOffset = 0;
- mMappedSize = 0;
+ mMapStateData->mapState = MapState::Unmapped;
+ mMapOffset = 0;
+ mMapSize = 0;
BufferUnmapCmd cmd;
cmd.self = ToAPI(this);
client->SerializeCommand(cmd);
- SetFutureStatus(WGPUBufferMapAsyncStatus_UnmappedBeforeCallback);
+ SetFutureStatusAndClearPending(WGPUBufferMapAsyncStatus_UnmappedBeforeCallback);
}
void Buffer::Destroy() {
@@ -474,12 +440,13 @@
// Remove the current mapping and destroy Read/WriteHandles.
FreeMappedData();
+ mMapStateData->mapState = MapState::Unmapped;
BufferDestroyCmd cmd;
cmd.self = ToAPI(this);
client->SerializeCommand(cmd);
- SetFutureStatus(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback);
+ SetFutureStatusAndClearPending(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback);
}
WGPUBufferUsage Buffer::GetUsage() const {
@@ -491,13 +458,13 @@
}
WGPUBufferMapState Buffer::GetMapState() const {
- switch (mMappedState) {
+ switch (mMapStateData->mapState) {
case MapState::MappedForRead:
case MapState::MappedForWrite:
case MapState::MappedAtCreation:
return WGPUBufferMapState_Mapped;
case MapState::Unmapped:
- if (mPendingMapRequest) {
+ if (mMapStateData->pendingRequest) {
return WGPUBufferMapState_Pending;
} else {
return WGPUBufferMapState_Unmapped;
@@ -507,26 +474,27 @@
}
bool Buffer::IsMappedForReading() const {
- return mMappedState == MapState::MappedForRead;
+ return mMapStateData->mapState == MapState::MappedForRead;
}
bool Buffer::IsMappedForWriting() const {
- return mMappedState == MapState::MappedForWrite || mMappedState == MapState::MappedAtCreation;
+ return mMapStateData->mapState == MapState::MappedForWrite ||
+ mMapStateData->mapState == MapState::MappedAtCreation;
}
bool Buffer::CheckGetMappedRangeOffsetSize(size_t offset, size_t size) const {
- if (offset % 8 != 0 || offset < mMappedOffset || offset > mSize) {
+ if (offset % 8 != 0 || offset < mMapOffset || offset > mSize) {
return false;
}
size_t rangeSize = size == WGPU_WHOLE_MAP_SIZE ? mSize - offset : size;
- if (rangeSize % 4 != 0 || rangeSize > mMappedSize) {
+ if (rangeSize % 4 != 0 || rangeSize > mMapSize) {
return false;
}
- size_t offsetInMappedRange = offset - mMappedOffset;
- return offsetInMappedRange <= mMappedSize - rangeSize;
+ size_t offsetInMappedRange = offset - mMapOffset;
+ return offsetInMappedRange <= mMapSize - rangeSize;
}
void Buffer::FreeMappedData() {
@@ -535,16 +503,15 @@
// use-after-free of the mapped data. This is particularly useful for WebGPU test about the
// interaction of mapping and GC.
if (mMappedData) {
- memset(static_cast<uint8_t*>(mMappedData) + mMappedOffset, 0xCA, mMappedSize);
+ memset(static_cast<uint8_t*>(mMappedData) + mMapOffset, 0xCA, mMapSize);
}
#endif // defined(DAWN_ENABLE_ASSERTS)
- mMappedOffset = 0;
- mMappedSize = 0;
+ mMapOffset = 0;
+ mMapSize = 0;
mReadHandle = nullptr;
mWriteHandle = nullptr;
mMappedData = nullptr;
- mMappedState = MapState::Unmapped;
}
} // namespace dawn::wire::client
diff --git a/src/dawn/wire/client/Buffer.h b/src/dawn/wire/client/Buffer.h
index 7747790..1c52d57 100644
--- a/src/dawn/wire/client/Buffer.h
+++ b/src/dawn/wire/client/Buffer.h
@@ -43,6 +43,28 @@
class Device;
+enum class MapRequestType { None, Read, Write };
+
+enum class MapState {
+ Unmapped,
+ MappedForRead,
+ MappedForWrite,
+ MappedAtCreation,
+};
+
+struct MapRequestData {
+ FutureID futureID = kNullFutureID;
+ size_t offset = 0;
+ size_t size = 0;
+ MapRequestType type = MapRequestType::None;
+};
+
+struct MapStateData : public RefCounted {
+ // Up to only one request can exist at a single time. Other requests are rejected.
+ std::optional<MapRequestData> pendingRequest = std::nullopt;
+ MapState mapState = MapState::Unmapped;
+};
+
class Buffer final : public ObjectWithEventsBase {
public:
static WGPUBuffer Create(Device* device, const WGPUBufferDescriptor* descriptor);
@@ -52,6 +74,10 @@
const WGPUBufferDescriptor* descriptor);
~Buffer() override;
+ bool OnMapAsyncCallback(WGPUFuture future,
+ uint32_t status,
+ uint64_t readDataUpdateInfoLength,
+ const uint8_t* readDataUpdateInfo);
void MapAsync(WGPUMapModeFlags mode,
size_t offset,
size_t size,
@@ -74,11 +100,9 @@
WGPUBufferMapState GetMapState() const;
private:
- friend class Client;
- class MapAsyncEvent;
-
// Prepares the callbacks to be called and potentially calls them
- void SetFutureStatus(WGPUBufferMapAsyncStatus status);
+ bool SetFutureStatus(WGPUBufferMapAsyncStatus status);
+ void SetFutureStatusAndClearPending(WGPUBufferMapAsyncStatus status);
bool IsMappedForReading() const;
bool IsMappedForWriting() const;
@@ -86,39 +110,25 @@
void FreeMappedData();
- const uint64_t mSize = 0;
- const WGPUBufferUsage mUsage;
- const bool mDestructWriteHandleOnUnmap;
+ // The map state is a shared resource with the TrackedEvent so that it is updated only when the
+ // callback is actually called. This is important for WaitAny and ProcessEvents cases where the
+ // server may have responded, but due to an early Unmap or Destroy before the corresponding
+ // WaitAny or ProcessEvents call, we need to update the callback result.
+ Ref<MapStateData> mMapStateData;
- std::weak_ptr<bool> mIsDeviceAlive;
-
- // Mapping members are mutable depending on the current map state.
- enum class MapRequestType { Read, Write };
- struct MapRequest {
- FutureID futureID = kNullFutureID;
- size_t offset = 0;
- size_t size = 0;
- // Because validation for request type is validated via the backend, we use an optional type
- // here. This is nullopt when an invalid request type is passed to the wire.
- std::optional<MapRequestType> type;
- };
- enum class MapState {
- Unmapped,
- MappedForRead,
- MappedForWrite,
- MappedAtCreation,
- };
- std::optional<MapRequest> mPendingMapRequest = std::nullopt;
- MapState mMappedState = MapState::Unmapped;
- // TODO(https://crbug.com/dawn/2345): Investigate `DanglingUntriaged` in dawn/wire.
- raw_ptr<void, DanglingUntriaged> mMappedData = nullptr;
- size_t mMappedOffset = 0;
- size_t mMappedSize = 0;
+ uint64_t mSize = 0;
+ WGPUBufferUsage mUsage;
// Only one mapped pointer can be active at a time
// TODO(enga): Use a tagged pointer to save space.
std::unique_ptr<MemoryTransferService::ReadHandle> mReadHandle = nullptr;
std::unique_ptr<MemoryTransferService::WriteHandle> mWriteHandle = nullptr;
+ bool mDestructWriteHandleOnUnmap = false;
+
+ // TODO(https://crbug.com/dawn/2345): Investigate `DanglingUntriaged` in dawn/wire.
+ raw_ptr<void, DanglingUntriaged> mMappedData = nullptr;
+ size_t mMapOffset = 0;
+ size_t mMapSize = 0;
};
} // namespace dawn::wire::client
diff --git a/src/dawn/wire/client/ClientDoers.cpp b/src/dawn/wire/client/ClientDoers.cpp
index 2ed46d8..210cc53 100644
--- a/src/dawn/wire/client/ClientDoers.cpp
+++ b/src/dawn/wire/client/ClientDoers.cpp
@@ -88,6 +88,20 @@
return device->OnPopErrorScopeCallback(requestSerial, errorType, message);
}
+// TODO(dawn:2061) May be able to move this to Buffer.cpp once we move all mapping logic.
+bool Client::DoBufferMapAsyncCallback(Buffer* buffer,
+ ObjectHandle eventManager,
+ WGPUFuture future,
+ uint32_t status,
+ uint64_t readDataUpdateInfoLength,
+ const uint8_t* readDataUpdateInfo) {
+ // The buffer might have been deleted or recreated so this isn't an error.
+ if (buffer == nullptr) {
+ return true;
+ }
+ return buffer->OnMapAsyncCallback(future, status, readDataUpdateInfoLength, readDataUpdateInfo);
+}
+
bool Client::DoDeviceCreateComputePipelineAsyncCallback(Device* device,
uint64_t requestSerial,
WGPUCreatePipelineAsyncStatus status,
diff --git a/src/dawn/wire/client/EventManager.h b/src/dawn/wire/client/EventManager.h
index 8ea33ee..071b825 100644
--- a/src/dawn/wire/client/EventManager.h
+++ b/src/dawn/wire/client/EventManager.h
@@ -114,7 +114,7 @@
}
std::unique_ptr<TrackedEvent> spontaneousEvent;
- WireResult result = mTrackedEvents.Use([&](auto trackedEvents) {
+ WIRE_TRY(mTrackedEvents.Use([&](auto trackedEvents) {
auto it = trackedEvents->find(futureID);
if (it == trackedEvents->end()) {
// If the future is not found, it must've already been completed.
@@ -128,9 +128,8 @@
DAWN_ASSERT(trackedEvent->GetType() == Event::kType);
return WireResult::FatalError;
}
-
- WireResult result = static_cast<Event*>(trackedEvent.get())
- ->ReadyHook(futureID, std::forward<ReadyArgs>(readyArgs)...);
+ static_cast<Event*>(trackedEvent.get())
+ ->ReadyHook(std::forward<ReadyArgs>(readyArgs)...);
trackedEvent->SetReady();
// If the event can be spontaneously completed, prepare to do so now.
@@ -138,14 +137,14 @@
spontaneousEvent = std::move(trackedEvent);
trackedEvents->erase(futureID);
}
- return result;
- });
+ return WireResult::Success;
+ }));
// Handle spontaneous completions.
if (spontaneousEvent) {
spontaneousEvent->Complete(futureID, EventCompletionType::Ready);
}
- return result;
+ return WireResult::Success;
}
void ProcessPollEvents();
diff --git a/src/dawn/wire/client/Instance.cpp b/src/dawn/wire/client/Instance.cpp
index 95718ad..e67fc83 100644
--- a/src/dawn/wire/client/Instance.cpp
+++ b/src/dawn/wire/client/Instance.cpp
@@ -54,13 +54,12 @@
EventType GetType() override { return kType; }
- WireResult ReadyHook(FutureID futureID,
- WGPURequestAdapterStatus status,
- const char* message,
- const WGPUAdapterProperties* properties,
- const WGPUSupportedLimits* limits,
- uint32_t featuresCount,
- const WGPUFeatureName* features) {
+ void ReadyHook(WGPURequestAdapterStatus status,
+ const char* message,
+ const WGPUAdapterProperties* properties,
+ const WGPUSupportedLimits* limits,
+ uint32_t featuresCount,
+ const WGPUFeatureName* features) {
DAWN_ASSERT(mAdapter != nullptr);
mStatus = status;
if (message != nullptr) {
@@ -71,7 +70,6 @@
mAdapter->SetLimits(limits);
mAdapter->SetFeatures(features, featuresCount);
}
- return WireResult::Success;
}
private:
diff --git a/src/dawn/wire/client/Queue.cpp b/src/dawn/wire/client/Queue.cpp
index d897ba7..5337853 100644
--- a/src/dawn/wire/client/Queue.cpp
+++ b/src/dawn/wire/client/Queue.cpp
@@ -48,10 +48,7 @@
EventType GetType() override { return kType; }
- WireResult ReadyHook(FutureID futureID, WGPUQueueWorkDoneStatus status) {
- mStatus = status;
- return WireResult::Success;
- }
+ void ReadyHook(WGPUQueueWorkDoneStatus status) { mStatus = status; }
private:
void CompleteImpl(FutureID futureID, EventCompletionType completionType) override {
diff --git a/src/dawn/wire/server/ServerBuffer.cpp b/src/dawn/wire/server/ServerBuffer.cpp
index 22e2769..ca1471c 100644
--- a/src/dawn/wire/server/ServerBuffer.cpp
+++ b/src/dawn/wire/server/ServerBuffer.cpp
@@ -225,6 +225,8 @@
bool isSuccess = status == WGPUBufferMapAsyncStatus_Success;
ReturnBufferMapAsyncCallbackCmd cmd;
+ // TODO(dawn:2061) Should be able to remove buffer once mapping is updated.
+ cmd.buffer = data->buffer;
cmd.eventManager = data->eventManager;
cmd.future = data->future;
cmd.status = status;
