Skip to content

async_wrap,src: wrap promises directly #13224

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions configure
Original file line number Diff line number Diff line change
Expand Up @@ -954,6 +954,7 @@ def configure_v8(o):
o['variables']['v8_no_strict_aliasing'] = 1 # Work around compiler bugs.
o['variables']['v8_optimized_debug'] = 0 # Compile with -O0 in debug builds.
o['variables']['v8_random_seed'] = 0 # Use a random seed for hash tables.
o['variables']['v8_promise_internal_field_count'] = 1 # Add internal field to promises for async hooks.
o['variables']['v8_use_snapshot'] = 'false' if options.without_snapshot else 'true'
o['variables']['node_use_v8_platform'] = b(not options.without_v8_platform)
o['variables']['node_use_bundled_v8'] = b(not options.without_bundled_v8)
Expand Down
7 changes: 7 additions & 0 deletions deps/v8/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ declare_args() {
# Sets -dENABLE_DISASSEMBLER.
v8_enable_disassembler = ""

# Sets the number of internal fields on promise objects.
v8_promise_internal_field_count = 0

# Sets -dENABLE_GDB_JIT_INTERFACE.
v8_enable_gdbjit = ""

Expand Down Expand Up @@ -197,6 +200,10 @@ config("features") {
if (v8_enable_disassembler) {
defines += [ "ENABLE_DISASSEMBLER" ]
}
if (v8_promise_internal_field_count != 0) {
defines +=
[ "V8_PROMISE_INTERNAL_FIELD_COUNT=${v8_promise_internal_field_count}" ]
}
if (v8_enable_gdbjit) {
defines += [ "ENABLE_GDB_JIT_INTERFACE" ]
}
Expand Down
5 changes: 5 additions & 0 deletions deps/v8/gypfiles/features.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
'variables': {
'v8_enable_disassembler%': 0,

'v8_promise_internal_field_count%': 0,

'v8_enable_gdbjit%': 0,

'v8_enable_verify_csa%': 0,
Expand Down Expand Up @@ -77,6 +79,9 @@
['v8_enable_disassembler==1', {
'defines': ['ENABLE_DISASSEMBLER',],
}],
['v8_promise_internal_field_count!=0', {
'defines': ['V8_PROMISE_INTERNAL_FIELD_COUNT','v8_promise_internal_field_count'],
}],
['v8_enable_gdbjit==1', {
'defines': ['ENABLE_GDB_JIT_INTERFACE',],
}],
Expand Down
2 changes: 1 addition & 1 deletion deps/v8/include/v8-version.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
#define V8_MAJOR_VERSION 5
#define V8_MINOR_VERSION 8
#define V8_BUILD_NUMBER 283
#define V8_PATCH_LEVEL 40
#define V8_PATCH_LEVEL 41

// Use 1 for candidates and 0 otherwise.
// (Boolean macro values are not supported by all preprocessors.)
Expand Down
6 changes: 6 additions & 0 deletions deps/v8/include/v8.h
Original file line number Diff line number Diff line change
Expand Up @@ -3741,6 +3741,10 @@ class V8_EXPORT Function : public Object {
static void CheckCast(Value* obj);
};

#ifndef V8_PROMISE_INTERNAL_FIELD_COUNT
// The number of required internal fields can be defined by embedder.
#define V8_PROMISE_INTERNAL_FIELD_COUNT 0
#endif

/**
* An instance of the built-in Promise constructor (ES6 draft).
Expand Down Expand Up @@ -3822,6 +3826,8 @@ class V8_EXPORT Promise : public Object {

V8_INLINE static Promise* Cast(Value* obj);

static const int kEmbedderFieldCount = V8_PROMISE_INTERNAL_FIELD_COUNT;

private:
Promise();
static void CheckCast(Value* obj);
Expand Down
6 changes: 3 additions & 3 deletions deps/v8/src/bootstrapper.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1945,9 +1945,9 @@ void Genesis::InitializeGlobal(Handle<JSGlobalObject> global_object,

Handle<JSObject> prototype =
factory->NewJSObject(isolate->object_function(), TENURED);
Handle<JSFunction> promise_fun =
InstallFunction(global, "Promise", JS_PROMISE_TYPE, JSPromise::kSize,
prototype, Builtins::kPromiseConstructor);
Handle<JSFunction> promise_fun = InstallFunction(
global, "Promise", JS_PROMISE_TYPE, JSPromise::kSizeWithEmbedderFields,
prototype, Builtins::kPromiseConstructor);
InstallWithIntrinsicDefaultProto(isolate, promise_fun,
Context::PROMISE_FUNCTION_INDEX);

Expand Down
8 changes: 8 additions & 0 deletions deps/v8/src/builtins/builtins-promise.cc
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,10 @@ void PromiseBuiltinsAssembler::PromiseInit(Node* promise) {
StoreObjectField(promise, JSPromise::kStatusOffset,
SmiConstant(v8::Promise::kPending));
StoreObjectField(promise, JSPromise::kFlagsOffset, SmiConstant(0));
for (int i = 0; i < v8::Promise::kEmbedderFieldCount; i++) {
int offset = JSPromise::kSize + i * kPointerSize;
StoreObjectFieldNoWriteBarrier(promise, offset, SmiConstant(Smi::kZero));
}
}

Node* PromiseBuiltinsAssembler::AllocateAndInitJSPromise(Node* context) {
Expand Down Expand Up @@ -62,6 +66,10 @@ Node* PromiseBuiltinsAssembler::AllocateAndSetJSPromise(Node* context,
StoreObjectFieldNoWriteBarrier(instance, JSPromise::kResultOffset, result);
StoreObjectFieldNoWriteBarrier(instance, JSPromise::kFlagsOffset,
SmiConstant(0));
for (int i = 0; i < v8::Promise::kEmbedderFieldCount; i++) {
int offset = JSPromise::kSize + i * kPointerSize;
StoreObjectFieldNoWriteBarrier(instance, offset, SmiConstant(Smi::kZero));
}

Label out(this);
GotoIfNot(IsPromiseHookEnabledOrDebugIsActive(), &out);
Expand Down
2 changes: 2 additions & 0 deletions deps/v8/src/objects.h
Original file line number Diff line number Diff line change
Expand Up @@ -8443,6 +8443,8 @@ class JSPromise : public JSObject {
kFulfillReactionsOffset + kPointerSize;
static const int kFlagsOffset = kRejectReactionsOffset + kPointerSize;
static const int kSize = kFlagsOffset + kPointerSize;
static const int kSizeWithEmbedderFields =
kSize + v8::Promise::kEmbedderFieldCount * kPointerSize;

// Flags layout.
static const int kHasHandlerBit = 0;
Expand Down
30 changes: 3 additions & 27 deletions src/async-wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -281,32 +281,13 @@ static void PromiseHook(PromiseHookType type, Local<Promise> promise,
Local<Context> context = promise->CreationContext();
Environment* env = Environment::GetCurrent(context);
if (type == PromiseHookType::kInit) {
// Unfortunately, promises don't have internal fields. Need a surrogate that
// async wrap can wrap.
Local<Object> obj =
env->async_hooks_promise_object()->NewInstance(context).ToLocalChecked();
PromiseWrap* wrap = new PromiseWrap(env, obj);
v8::PropertyAttribute hidden =
static_cast<v8::PropertyAttribute>(v8::ReadOnly
| v8::DontDelete
| v8::DontEnum);
promise->DefineOwnProperty(context,
env->promise_wrap(),
v8::External::New(env->isolate(), wrap),
hidden).FromJust();
// The async tag will be destroyed at the same time as the promise as the
// only reference to it is held by the promise. This allows the promise
// wrap instance to be notified when the promise is destroyed.
promise->DefineOwnProperty(context,
env->promise_async_tag(),
obj, hidden).FromJust();
PromiseWrap* wrap = new PromiseWrap(env, promise);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes the resource that the init callback sees the Promise itself, right? /cc @Fishrock123

promise->SetAlignedPointerInInternalField(0, wrap);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait… doesn’t this leak the PromiseWrap memory? V8 doesn’t know that this is a pointer to something it can/should garbage collect. I think promise->SetInternalField(0, wrap->object()); should work?

} else if (type == PromiseHookType::kResolve) {
// TODO(matthewloring): need to expose this through the async hooks api.
}
Local<v8::Value> external_wrap =
promise->Get(context, env->promise_wrap()).ToLocalChecked();
PromiseWrap* wrap =
static_cast<PromiseWrap*>(external_wrap.As<v8::External>()->Value());
static_cast<PromiseWrap*>(promise->GetAlignedPointerFromInternalField(0));
CHECK_NE(wrap, nullptr);
if (type == PromiseHookType::kBefore) {
PreCallbackExecution(wrap, false);
Expand Down Expand Up @@ -402,11 +383,6 @@ void AsyncWrap::Initialize(Local<Object> target,
env->SetMethod(target, "clearIdStack", ClearIdStack);
env->SetMethod(target, "addIdToDestroyList", QueueDestroyId);

Local<v8::ObjectTemplate> promise_object_template =
v8::ObjectTemplate::New(env->isolate());
promise_object_template->SetInternalFieldCount(1);
env->set_async_hooks_promise_object(promise_object_template);

v8::PropertyAttribute ReadOnlyDontDelete =
static_cast<v8::PropertyAttribute>(v8::ReadOnly | v8::DontDelete);

Expand Down
3 changes: 0 additions & 3 deletions src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -195,8 +195,6 @@ namespace node {
V(preference_string, "preference") \
V(priority_string, "priority") \
V(produce_cached_data_string, "produceCachedData") \
V(promise_wrap, "_promise_async_wrap") \
V(promise_async_tag, "_promise_async_wrap_tag") \
V(raw_string, "raw") \
V(read_host_object_string, "_readHostObject") \
V(readable_string, "readable") \
Expand Down Expand Up @@ -258,7 +256,6 @@ namespace node {
V(async_hooks_init_function, v8::Function) \
V(async_hooks_before_function, v8::Function) \
V(async_hooks_after_function, v8::Function) \
V(async_hooks_promise_object, v8::ObjectTemplate) \
V(binding_cache_object, v8::Object) \
V(buffer_constructor_function, v8::Function) \
V(buffer_prototype_object, v8::Object) \
Expand Down