dawn.node: Bring the GPUBuffer.mapAsync implementation up to spec.
The semantic of early rejection changed just before v1 and we never
caught up with it. This CL makes all the buffer mapping validation tests
pass.
Bug: dawn:1134
Change-Id: I2d95a97da6ad205a5dde35a32086974f3d2f1f2f
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/159063
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Ben Clayton <bclayton@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
diff --git a/src/dawn/node/binding/AsyncRunner.cpp b/src/dawn/node/binding/AsyncRunner.cpp
index 8aa5dae..bd8d17d 100644
--- a/src/dawn/node/binding/AsyncRunner.cpp
+++ b/src/dawn/node/binding/AsyncRunner.cpp
@@ -69,4 +69,12 @@
});
}
+void AsyncRunner::Reject(interop::Promise<void> promise, Napi::Error error) {
+ env_.Global()
+ .Get("setImmediate")
+ .As<Napi::Function>()
+ .Call({Napi::Function::New(
+ env_, [promise, error](const Napi::CallbackInfo&) { promise.Reject(error); })});
+}
+
} // namespace wgpu::binding
diff --git a/src/dawn/node/binding/AsyncRunner.h b/src/dawn/node/binding/AsyncRunner.h
index 8162fd6..3b98875 100644
--- a/src/dawn/node/binding/AsyncRunner.h
+++ b/src/dawn/node/binding/AsyncRunner.h
@@ -33,6 +33,7 @@
#include <utility>
#include "dawn/webgpu_cpp.h"
+#include "src/dawn/node/interop/Core.h"
#include "src/dawn/node/interop/NodeAPI.h"
namespace wgpu::binding {
@@ -54,6 +55,12 @@
// Every call to Begin() should eventually result in a call to End().
void End();
+ // Rejects the promise after the current task in the event loop. This is useful to preserve
+ // some of the semantics of WebGPU w.r.t. the JavaScript event loop. Reject() can be called
+ // any time, but callers need to make sure that the Promise is (rejected or resolved) only
+ // once.
+ void Reject(interop::Promise<void> promise, Napi::Error error);
+
private:
void QueueTick();
Napi::Env env_;
diff --git a/src/dawn/node/binding/GPUBuffer.cpp b/src/dawn/node/binding/GPUBuffer.cpp
index afb81db..add9700 100644
--- a/src/dawn/node/binding/GPUBuffer.cpp
+++ b/src/dawn/node/binding/GPUBuffer.cpp
@@ -27,6 +27,7 @@
#include "src/dawn/node/binding/GPUBuffer.h"
+#include <cassert>
#include <memory>
#include <utility>
@@ -38,8 +39,6 @@
////////////////////////////////////////////////////////////////////////////////
// wgpu::bindings::GPUBuffer
-// TODO(crbug.com/dawn/1134): We may be doing more validation here than necessary. Once CTS is
-// robustly passing, pull out validation and see what / if breaks.
////////////////////////////////////////////////////////////////////////////////
GPUBuffer::GPUBuffer(wgpu::Buffer buffer,
wgpu::BufferDescriptor desc,
@@ -49,95 +48,90 @@
desc_(desc),
device_(std::move(device)),
async_(std::move(async)),
- label_(desc.label ? desc.label : "") {
- if (desc.mappedAtCreation) {
- state_ = State::MappedAtCreation;
- }
-}
+ mapped_(desc.mappedAtCreation),
+ label_(desc.label ? desc.label : "") {}
interop::Promise<void> GPUBuffer::mapAsync(Napi::Env env,
- interop::GPUMapModeFlags mode,
+ interop::GPUMapModeFlags modeIn,
interop::GPUSize64 offset,
std::optional<interop::GPUSize64> size) {
- wgpu::MapMode md{};
- Converter conv(env);
- if (!conv(md, mode)) {
- interop::Promise<void> promise(env, PROMISE_INFO);
+ // Convert the mapMode and reject with the TypeError if it happens.
+ Converter conv(env, device_, true);
+ wgpu::MapMode mode;
+ if (!conv(mode, modeIn)) {
+ auto promise = interop::Promise<void>(env, PROMISE_INFO);
+ promise.Reject(conv.AcquireException());
+ return promise;
+ }
+
+ // Early rejection when there is already a mapping pending.
+ if (pending_map_) {
+ auto promise = interop::Promise<void>(env, PROMISE_INFO);
promise.Reject(Errors::OperationError(env));
return promise;
}
- if (state_ != State::Unmapped) {
- interop::Promise<void> promise(env, PROMISE_INFO);
- promise.Reject(Errors::OperationError(env));
- device_.InjectError(wgpu::ErrorType::Validation,
- "mapAsync called on buffer that is not in the unmapped state");
- return promise;
- }
+ pending_map_.emplace(env, PROMISE_INFO);
+ uint64_t rangeSize = size.has_value() ? size.value().value : (desc_.size - offset);
struct Context {
Napi::Env env;
- interop::Promise<void> promise;
+ GPUBuffer* self;
AsyncTask task;
- State& state;
+ interop::Promise<void> promise;
};
- auto ctx =
- new Context{env, interop::Promise<void>(env, PROMISE_INFO), AsyncTask(async_), state_};
- auto promise = ctx->promise;
-
- uint64_t s = size.has_value() ? size.value().value : (desc_.size - offset);
-
- state_ = State::MappingPending;
+ auto ctx = new Context{env, this, AsyncTask(async_), *pending_map_};
buffer_.MapAsync(
- md, offset, s,
+ mode, offset, rangeSize,
[](WGPUBufferMapAsyncStatus status, void* userdata) {
auto c = std::unique_ptr<Context>(static_cast<Context*>(userdata));
- c->state = State::Unmapped;
+
+ // The promise may already have been resolved with an AbortError if there was an early
+ // destroy() or early unmap().
+ if (c->promise.GetState() != interop::PromiseState::Pending) {
+ assert(c->promise.GetState() == interop::PromiseState::Rejected);
+ return;
+ }
+
switch (status) {
case WGPUBufferMapAsyncStatus_Force32:
UNREACHABLE("WGPUBufferMapAsyncStatus_Force32");
break;
case WGPUBufferMapAsyncStatus_Success:
c->promise.Resolve();
- c->state = State::Mapped;
+ c->self->mapped_ = true;
break;
case WGPUBufferMapAsyncStatus_ValidationError:
- c->promise.Reject(Errors::OperationError(c->env));
- break;
case WGPUBufferMapAsyncStatus_UnmappedBeforeCallback:
case WGPUBufferMapAsyncStatus_DestroyedBeforeCallback:
case WGPUBufferMapAsyncStatus_MappingAlreadyPending:
case WGPUBufferMapAsyncStatus_OffsetOutOfRange:
case WGPUBufferMapAsyncStatus_SizeOutOfRange:
- c->promise.Reject(Errors::AbortError(c->env));
- break;
- case WGPUBufferMapAsyncStatus_Unknown:
case WGPUBufferMapAsyncStatus_DeviceLost:
- // TODO(dawn:1123): The spec is a bit vague around what the promise should
- // do here.
- c->promise.Reject(Errors::UnknownError(c->env));
+ case WGPUBufferMapAsyncStatus_Unknown:
+ c->self->async_->Reject(c->promise, Errors::OperationError(c->env));
break;
}
+
+ // This captured promise is the currently pending mapping, reset it so we can start new
+ // mappings.
+ assert(*c->self->pending_map_ == c->promise);
+ c->self->pending_map_.reset();
},
ctx);
- return promise;
+ return pending_map_.value();
}
interop::ArrayBuffer GPUBuffer::getMappedRange(Napi::Env env,
interop::GPUSize64 offset,
std::optional<interop::GPUSize64> size) {
- if (state_ != State::Mapped && state_ != State::MappedAtCreation) {
- Errors::OperationError(env).ThrowAsJavaScriptException();
- return {};
- }
-
uint64_t s = size.has_value() ? size.value().value : (desc_.size - offset);
uint64_t start = offset;
uint64_t end = offset + s;
- for (auto& mapping : mapped_) {
+ for (auto& mapping : mappings_) {
if (mapping.Intersects(start, end)) {
Errors::OperationError(env).ThrowAsJavaScriptException();
return {};
@@ -153,30 +147,18 @@
}
auto array_buffer = Napi::ArrayBuffer::New(env, ptr, s);
// TODO(crbug.com/dawn/1135): Ownership here is the wrong way around.
- mapped_.emplace_back(Mapping{start, end, Napi::Persistent(array_buffer)});
+ mappings_.emplace_back(Mapping{start, end, Napi::Persistent(array_buffer)});
return array_buffer;
}
void GPUBuffer::unmap(Napi::Env env) {
+ DetachMappings(env);
buffer_.Unmap();
-
- if (state_ != State::Destroyed && state_ != State::Unmapped) {
- DetachMappings();
- state_ = State::Unmapped;
- }
}
-void GPUBuffer::destroy(Napi::Env) {
- if (state_ == State::Destroyed) {
- return;
- }
-
- if (state_ != State::Unmapped) {
- DetachMappings();
- }
-
+void GPUBuffer::destroy(Napi::Env env) {
+ DetachMappings(env);
buffer_.Destroy();
- state_ = State::Destroyed;
}
interop::GPUSize64Out GPUBuffer::getSize(Napi::Env) {
@@ -184,16 +166,16 @@
}
interop::GPUBufferMapState GPUBuffer::getMapState(Napi::Env env) {
- interop::GPUBufferMapState result;
-
- Converter conv(env);
- if (!conv(result, buffer_.GetMapState())) {
- Napi::Error::New(env, "Couldn't convert usage to a JavaScript value.")
- .ThrowAsJavaScriptException();
- return interop::GPUBufferMapState::kUnmapped;
+ if (mapped_) {
+ return interop::GPUBufferMapState::kMapped;
}
- return result;
+ if (pending_map_) {
+ assert(pending_map_->GetState() == interop::PromiseState::Pending);
+ return interop::GPUBufferMapState::kPending;
+ }
+
+ return interop::GPUBufferMapState::kUnmapped;
}
interop::GPUFlagsConstant GPUBuffer::getUsage(Napi::Env env) {
@@ -209,11 +191,18 @@
return result;
}
-void GPUBuffer::DetachMappings() {
- for (auto& mapping : mapped_) {
+void GPUBuffer::DetachMappings(Napi::Env env) {
+ mapped_ = false;
+
+ if (pending_map_) {
+ pending_map_->Reject(Errors::AbortError(env));
+ pending_map_.reset();
+ }
+
+ for (auto& mapping : mappings_) {
mapping.buffer.Value().Detach();
}
- mapped_.clear();
+ mappings_.clear();
}
std::string GPUBuffer::getLabel(Napi::Env) {
diff --git a/src/dawn/node/binding/GPUBuffer.h b/src/dawn/node/binding/GPUBuffer.h
index 53cdd1f..24f4a93 100644
--- a/src/dawn/node/binding/GPUBuffer.h
+++ b/src/dawn/node/binding/GPUBuffer.h
@@ -71,7 +71,7 @@
void setLabel(Napi::Env, std::string value) override;
private:
- void DetachMappings();
+ void DetachMappings(Napi::Env env);
struct Mapping {
uint64_t start;
@@ -93,8 +93,9 @@
wgpu::BufferDescriptor const desc_;
wgpu::Device const device_;
std::shared_ptr<AsyncRunner> async_;
- State state_ = State::Unmapped;
- std::vector<Mapping> mapped_;
+ std::vector<Mapping> mappings_;
+ bool mapped_;
+ std::optional<interop::Promise<void>> pending_map_;
std::string label_;
};
diff --git a/src/dawn/node/interop/Core.h b/src/dawn/node/interop/Core.h
index 93997d6..4e758be 100644
--- a/src/dawn/node/interop/Core.h
+++ b/src/dawn/node/interop/Core.h
@@ -212,6 +212,10 @@
inline operator Napi::Value() const { return state_->deferred.Promise(); }
inline operator Napi::Promise() const { return state_->deferred.Promise(); }
+ // Comparison operator between promises
+ bool operator==(const PromiseBase& other) { return state_ == other.state_; }
+ bool operator!=(const PromiseBase& other) { return !(*this == other); }
+
// Reject() rejects the promise with the given failure value.
void Reject(Napi::Value value) const {
state_->deferred.Reject(value);
