From e9cbd4896acc6f8266f336cf43feab80b434e3a1 Mon Sep 17 00:00:00 2001 From: Ben Clayton Date: Tue, 19 Oct 2021 19:18:42 +0000 Subject: [PATCH] dawn_node: Track promises These should always be resolved or rejected. The Fatal() call, when a promise is not resolved or rejected, is currently disabled due to https://github.com/gpuweb/cts/issues/784. Bug: dawn:1123 Change-Id: Ie0e8ac187ad70be0fea41cd66956d0bfd9c53212 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/66821 Reviewed-by: Antonio Maiorano Commit-Queue: Ben Clayton --- src/dawn_node/binding/GPU.cpp | 4 +- src/dawn_node/binding/GPUAdapter.cpp | 2 +- src/dawn_node/binding/GPUBuffer.cpp | 8 +- src/dawn_node/binding/GPUDevice.cpp | 38 +++--- src/dawn_node/binding/GPUQueue.cpp | 2 +- src/dawn_node/binding/GPUShaderModule.cpp | 2 +- src/dawn_node/interop/Core.h | 142 +++++++++++++--------- 7 files changed, 119 insertions(+), 79 deletions(-) diff --git a/src/dawn_node/binding/GPU.cpp b/src/dawn_node/binding/GPU.cpp index 2f649786af..61e4d6f948 100644 --- a/src/dawn_node/binding/GPU.cpp +++ b/src/dawn_node/binding/GPU.cpp @@ -55,8 +55,8 @@ namespace wgpu { namespace binding { interop::Promise>> GPU::requestAdapter( Napi::Env env, interop::GPURequestAdapterOptions options) { - auto promise = - interop::Promise>>(env); + auto promise = interop::Promise>>( + env, PROMISE_INFO); if (options.forceFallbackAdapter) { // Software adapters are not currently supported. diff --git a/src/dawn_node/binding/GPUAdapter.cpp b/src/dawn_node/binding/GPUAdapter.cpp index 82378d8515..c49e442537 100644 --- a/src/dawn_node/binding/GPUAdapter.cpp +++ b/src/dawn_node/binding/GPUAdapter.cpp @@ -139,7 +139,7 @@ namespace wgpu { namespace binding { Napi::Env env, interop::GPUDeviceDescriptor descriptor) { dawn_native::DeviceDescriptor desc{}; // TODO(crbug.com/dawn/1133): Fill in. - interop::Promise> promise(env); + interop::Promise> promise(env, PROMISE_INFO); // See src/dawn_native/Features.cpp for enum <-> string mappings. for (auto required : descriptor.requiredFeatures) { diff --git a/src/dawn_node/binding/GPUBuffer.cpp b/src/dawn_node/binding/GPUBuffer.cpp index ac5ae544b4..2fca5d5144 100644 --- a/src/dawn_node/binding/GPUBuffer.cpp +++ b/src/dawn_node/binding/GPUBuffer.cpp @@ -47,11 +47,13 @@ namespace wgpu { namespace binding { wgpu::MapMode md{}; Converter conv(env); if (!conv(md, mode)) { - return {env}; + interop::Promise promise(env, PROMISE_INFO); + promise.Reject(Errors::OperationError(env)); + return promise; } if (state_ != State::Unmapped) { - interop::Promise promise(env); + interop::Promise 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"); @@ -64,7 +66,7 @@ namespace wgpu { namespace binding { AsyncTask task; State& state; }; - auto ctx = new Context{env, interop::Promise(env), async_, state_}; + auto ctx = new Context{env, interop::Promise(env, PROMISE_INFO), async_, state_}; auto promise = ctx->promise; uint64_t s = size.has_value() ? size.value() : (desc_.size - offset); diff --git a/src/dawn_node/binding/GPUDevice.cpp b/src/dawn_node/binding/GPUDevice.cpp index 0870307598..2f928e5dc1 100644 --- a/src/dawn_node/binding/GPUDevice.cpp +++ b/src/dawn_node/binding/GPUDevice.cpp @@ -138,7 +138,12 @@ namespace wgpu { namespace binding { return interop::GPUQueue::Create(env, device_.GetQueue(), async_); } - void GPUDevice::destroy(Napi::Env) { + void GPUDevice::destroy(Napi::Env env) { + for (auto promise : lost_promises_) { + promise.Resolve(interop::GPUDeviceLostInfo::Create( + env_, interop::GPUDeviceLostReason::kDestroyed, "device was destroyed")); + } + lost_promises_.clear(); device_.Release(); } @@ -293,21 +298,23 @@ namespace wgpu { namespace binding { interop::Promise> GPUDevice::createComputePipelineAsync(Napi::Env env, interop::GPUComputePipelineDescriptor descriptor) { + using Promise = interop::Promise>; + Converter conv(env); wgpu::ComputePipelineDescriptor desc{}; if (!conv(desc, descriptor)) { - return {env}; + Promise promise(env, PROMISE_INFO); + promise.Reject(Errors::OperationError(env)); + return promise; } - using Promise = interop::Promise>; - struct Context { Napi::Env env; Promise promise; AsyncTask task; }; - auto ctx = new Context{env, env, async_}; + auto ctx = new Context{env, Promise(env, PROMISE_INFO), async_}; auto promise = ctx->promise; device_.CreateComputePipelineAsync( @@ -334,21 +341,23 @@ namespace wgpu { namespace binding { interop::Promise> GPUDevice::createRenderPipelineAsync(Napi::Env env, interop::GPURenderPipelineDescriptor descriptor) { + using Promise = interop::Promise>; + Converter conv(env); wgpu::RenderPipelineDescriptor desc{}; if (!conv(desc, descriptor)) { - return {env}; + Promise promise(env, PROMISE_INFO); + promise.Reject(Errors::OperationError(env)); + return promise; } - using Promise = interop::Promise>; - struct Context { Napi::Env env; Promise promise; AsyncTask task; }; - auto ctx = new Context{env, env, async_}; + auto ctx = new Context{env, Promise(env, PROMISE_INFO), async_}; auto promise = ctx->promise; device_.CreateRenderPipelineAsync( @@ -415,7 +424,8 @@ namespace wgpu { namespace binding { interop::Promise> GPUDevice::getLost( Napi::Env env) { - auto promise = interop::Promise>(env); + auto promise = + interop::Promise>(env, PROMISE_INFO); lost_promises_.emplace_back(promise); return promise; } @@ -444,7 +454,7 @@ namespace wgpu { namespace binding { Promise promise; AsyncTask task; }; - auto* ctx = new Context{env, env, async_}; + auto* ctx = new Context{env, Promise(env, PROMISE_INFO), async_}; auto promise = ctx->promise; bool ok = device_.PopErrorScope( @@ -476,10 +486,8 @@ namespace wgpu { namespace binding { } delete ctx; - Promise p(env); - p.Resolve( - interop::GPUValidationError::Create(env, "failed to pop error scope")); - return p; + promise.Reject(Errors::OperationError(env)); + return promise; } std::optional GPUDevice::getLabel(Napi::Env) { diff --git a/src/dawn_node/binding/GPUQueue.cpp b/src/dawn_node/binding/GPUQueue.cpp index c8e39fe23e..e1c0413d70 100644 --- a/src/dawn_node/binding/GPUQueue.cpp +++ b/src/dawn_node/binding/GPUQueue.cpp @@ -51,7 +51,7 @@ namespace wgpu { namespace binding { interop::Promise promise; AsyncTask task; }; - auto ctx = new Context{env, interop::Promise(env), async_}; + auto ctx = new Context{env, interop::Promise(env, PROMISE_INFO), async_}; auto promise = ctx->promise; queue_.OnSubmittedWorkDone( diff --git a/src/dawn_node/binding/GPUShaderModule.cpp b/src/dawn_node/binding/GPUShaderModule.cpp index 3323ba5c83..52efabd3ea 100644 --- a/src/dawn_node/binding/GPUShaderModule.cpp +++ b/src/dawn_node/binding/GPUShaderModule.cpp @@ -91,7 +91,7 @@ namespace wgpu { namespace binding { Promise promise; AsyncTask task; }; - auto ctx = new Context{env, env, async_}; + auto ctx = new Context{env, Promise(env, PROMISE_INFO), async_}; auto promise = ctx->promise; shader_.GetCompilationInfo( diff --git a/src/dawn_node/interop/Core.h b/src/dawn_node/interop/Core.h index ff26930959..dba1ad02ba 100644 --- a/src/dawn_node/interop/Core.h +++ b/src/dawn_node/interop/Core.h @@ -38,6 +38,13 @@ # define INTEROP_LOG(...) #endif +// A helper macro for constructing a PromiseInfo with the current file, function and line. +// See PromiseInfo +#define PROMISE_INFO \ + ::wgpu::interop::PromiseInfo { \ + __FILE__, __FUNCTION__, __LINE__ \ + } + namespace wgpu { namespace interop { //////////////////////////////////////////////////////////////////////////////// @@ -147,83 +154,106 @@ namespace wgpu { namespace interop { // Promise //////////////////////////////////////////////////////////////////////////////// + // Info holds details about where the promise was constructed. + // Used for printing debug messages when a promise is finalized without being resolved + // or rejected. + // Use the PROMISE_INFO macro to populate this structure. + struct PromiseInfo { + const char* file = nullptr; + const char* function = nullptr; + int line = 0; + }; + + namespace detail { + // Base class for Promise specializations. + class PromiseBase { + public: + // Implicit conversion operators to Napi promises. + inline operator napi_value() const { + return state->deferred.Promise(); + } + inline operator Napi::Value() const { + return state->deferred.Promise(); + } + inline operator Napi::Promise() const { + return state->deferred.Promise(); + } + + // Reject() rejects the promise with the given failure value. + void Reject(Napi::Value value) const { + state->deferred.Reject(value); + state->resolved_or_rejected = true; + } + void Reject(Napi::Error err) const { + Reject(err.Value()); + } + void Reject(std::string err) const { + Reject(Napi::Error::New(state->deferred.Env(), err)); + } + + protected: + void Resolve(Napi::Value value) const { + state->deferred.Resolve(value); + state->resolved_or_rejected = true; + } + + struct State { + Napi::Promise::Deferred deferred; + PromiseInfo info; + bool resolved_or_rejected = false; + }; + + PromiseBase(Napi::Env env, const PromiseInfo& info) + : state(new State{Napi::Promise::Deferred::New(env), info}) { + state->deferred.Promise().AddFinalizer( + [](Napi::Env, State* state) { + // TODO(https://github.com/gpuweb/cts/issues/784): + // Devices are never destroyed, so we always end up + // leaking the Device.lost promise. Enable this once + // fixed. + if ((false)) { + if (!state->resolved_or_rejected) { + ::wgpu::utils::Fatal("Promise not resolved or rejected", + state->info.file, state->info.line, + state->info.function); + } + } + delete state; + }, + state); + } + + State* const state; + }; + } // namespace detail + // Promise is a templated wrapper around a JavaScript promise, which can // resolve to the template type T. template - class Promise { + class Promise : public detail::PromiseBase { public: // Constructor - Promise(Napi::Env env) : deferred(Napi::Promise::Deferred::New(env)) { - } - - // Implicit conversion operators to Napi promises. - inline operator napi_value() const { - return deferred.Promise(); - } - inline operator Napi::Value() const { - return deferred.Promise(); - } - inline operator Napi::Promise() const { - return deferred.Promise(); + Promise(Napi::Env env, const PromiseInfo& info) : PromiseBase(env, info) { } // Resolve() fulfills the promise with the given value. void Resolve(T&& value) const { - deferred.Resolve(ToJS(deferred.Env(), std::forward(value))); + PromiseBase::Resolve(ToJS(state->deferred.Env(), std::forward(value))); } - - // Reject() rejects the promise with the given failure value. - void Reject(Napi::Object obj) const { - deferred.Reject(obj); - } - void Reject(Napi::Error err) const { - deferred.Reject(err.Value()); - } - void Reject(std::string err) const { - Reject(Napi::Error::New(deferred.Env(), err)); - } - - private: - Napi::Promise::Deferred deferred; }; // Specialization for Promises that resolve with no value template <> - class Promise { + class Promise : public detail::PromiseBase { public: // Constructor - Promise(Napi::Env env) : deferred(Napi::Promise::Deferred::New(env)) { - } - - // Implicit conversion operators to Napi promises. - inline operator napi_value() const { - return deferred.Promise(); - } - inline operator Napi::Value() const { - return deferred.Promise(); - } - inline operator Napi::Promise() const { - return deferred.Promise(); + Promise(Napi::Env env, const PromiseInfo& info) : PromiseBase(env, info) { } // Resolve() fulfills the promise. void Resolve() const { - deferred.Resolve(deferred.Env().Undefined()); + PromiseBase::Resolve(state->deferred.Env().Undefined()); } - - // Reject() rejects the promise with the given failure value. - void Reject(Napi::Object obj) const { - deferred.Reject(obj); - } - void Reject(Napi::Error err) const { - deferred.Reject(err.Value()); - } - void Reject(std::string err) const { - Reject(Napi::Error::New(deferred.Env(), err)); - } - - private: - Napi::Promise::Deferred deferred; }; ////////////////////////////////////////////////////////////////////////////////