From 13c5a1629cd025ba560f34f6d3190b2f38d184d4 Mon Sep 17 00:00:00 2001 From: Stephen Belanger Date: Thu, 16 Apr 2020 15:23:57 -0700 Subject: [PATCH] async_hooks: move PromiseHook handler to JS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This avoids the need to wrap every promise in an AsyncWrap and also makes it easier to skip the machinery to track destroy events when there's no destroy listener. Co-authored-by: Andrey Pechkurov PR-URL: https://github.com/nodejs/node/pull/32891 Reviewed-By: Matteo Collina Reviewed-By: Andrey Pechkurov Reviewed-By: Anna Henningsen Reviewed-By: Gerhard Stöbich Reviewed-By: Chengzhong Wu --- benchmark/async_hooks/promises.js | 27 +++- doc/api/async_hooks.md | 5 - lib/async_hooks.js | 3 + lib/internal/async_hooks.js | 76 +++++++++- src/async_wrap.cc | 173 +++++++++++++++++----- src/async_wrap.h | 5 + src/env.h | 3 + test/addons/async-hooks-promise/test.js | 76 ++++++++-- test/parallel/test-async-hooks-promise.js | 2 - test/parallel/test-bootstrap-modules.js | 1 + 10 files changed, 306 insertions(+), 65 deletions(-) diff --git a/benchmark/async_hooks/promises.js b/benchmark/async_hooks/promises.js index eb90ca0368e079..5632a6901d611b 100644 --- a/benchmark/async_hooks/promises.js +++ b/benchmark/async_hooks/promises.js @@ -2,10 +2,31 @@ const common = require('../common.js'); const { createHook } = require('async_hooks'); +let hook; +const tests = { + disabled() { + hook = createHook({ + promiseResolve() {} + }); + }, + enabled() { + hook = createHook({ + promiseResolve() {} + }).enable(); + }, + enabledWithDestroy() { + hook = createHook({ + promiseResolve() {}, + destroy() {} + }).enable(); + } +}; + const bench = common.createBenchmark(main, { n: [1e6], asyncHooks: [ 'enabled', + 'enabledWithDestroy', 'disabled', ] }); @@ -19,10 +40,8 @@ async function run(n) { } function main({ n, asyncHooks }) { - const hook = createHook({ promiseResolve() {} }); - if (asyncHooks !== 'disabled') { - hook.enable(); - } + if (hook) hook.disable(); + tests[asyncHooks](); bench.start(); run(n).then(() => { bench.end(n); diff --git a/doc/api/async_hooks.md b/doc/api/async_hooks.md index 698c8f2527789e..1534ca853611f9 100644 --- a/doc/api/async_hooks.md +++ b/doc/api/async_hooks.md @@ -306,11 +306,6 @@ currently not considered public, but using the Embedder API, users can provide and document their own resource objects. For example, such a resource object could contain the SQL query being executed. -In the case of Promises, the `resource` object will have an -`isChainedPromise` property, set to `true` if the promise has a parent promise, -and `false` otherwise. For example, in the case of `b = a.then(handler)`, `a` is -considered a parent `Promise` of `b`. Here, `b` is considered a chained promise. - In some cases the resource object is reused for performance reasons, it is thus not safe to use it as a key in a `WeakMap` or add properties to it. diff --git a/lib/async_hooks.js b/lib/async_hooks.js index 0943534790550c..9e287405f8af0b 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -26,6 +26,7 @@ const { getHookArrays, enableHooks, disableHooks, + updatePromiseHookMode, executionAsyncResource, // Internal Embedder API newAsyncId, @@ -101,6 +102,8 @@ class AsyncHook { enableHooks(); } + updatePromiseHookMode(); + return this; } diff --git a/lib/internal/async_hooks.js b/lib/internal/async_hooks.js index 8439260f1af1a6..b7a8c581cccd0b 100644 --- a/lib/internal/async_hooks.js +++ b/lib/internal/async_hooks.js @@ -3,7 +3,9 @@ const { Error, FunctionPrototypeBind, + ObjectPrototypeHasOwnProperty, ObjectDefineProperty, + Promise, Symbol, } = primordials; @@ -86,9 +88,10 @@ const { kInit, kBefore, kAfter, kDestroy, kTotals, kPromiseResolve, kCheck, kExecutionAsyncId, kAsyncIdCounter, kTriggerAsyncId, kDefaultTriggerAsyncId, kStackLength } = async_wrap.constants; +const { async_id_symbol, + trigger_async_id_symbol } = internalBinding('symbols'); + // Used in AsyncHook and AsyncResource. -const async_id_symbol = Symbol('asyncId'); -const trigger_async_id_symbol = Symbol('triggerAsyncId'); const init_symbol = Symbol('init'); const before_symbol = Symbol('before'); const after_symbol = Symbol('after'); @@ -243,27 +246,89 @@ function restoreActiveHooks() { active_hooks.tmp_fields = null; } +function trackPromise(promise, parent, silent) { + const asyncId = getOrSetAsyncId(promise); + + promise[trigger_async_id_symbol] = parent ? getOrSetAsyncId(parent) : + getDefaultTriggerAsyncId(); + + if (!silent && initHooksExist()) { + const triggerId = promise[trigger_async_id_symbol]; + emitInitScript(asyncId, 'PROMISE', triggerId, promise); + } +} + +function fastPromiseHook(type, promise, parent) { + if (type === kInit || !promise[async_id_symbol]) { + const silent = type !== kInit; + if (parent instanceof Promise) { + trackPromise(promise, parent, silent); + } else { + trackPromise(promise, null, silent); + } + + if (!silent) return; + } + + const asyncId = promise[async_id_symbol]; + switch (type) { + case kBefore: + const triggerId = promise[trigger_async_id_symbol]; + emitBeforeScript(asyncId, triggerId, promise); + break; + case kAfter: + if (hasHooks(kAfter)) { + emitAfterNative(asyncId); + } + if (asyncId === executionAsyncId()) { + // This condition might not be true if async_hooks was enabled during + // the promise callback execution. + // Popping it off the stack can be skipped in that case, because it is + // known that it would correspond to exactly one call with + // PromiseHookType::kBefore that was not witnessed by the PromiseHook. + popAsyncContext(asyncId); + } + break; + case kPromiseResolve: + emitPromiseResolveNative(asyncId); + break; + } +} let wantPromiseHook = false; function enableHooks() { async_hook_fields[kCheck] += 1; +} +let promiseHookMode = -1; +function updatePromiseHookMode() { wantPromiseHook = true; - enablePromiseHook(); + if (destroyHooksExist()) { + if (promiseHookMode !== 1) { + promiseHookMode = 1; + enablePromiseHook(); + } + } else if (promiseHookMode !== 0) { + promiseHookMode = 0; + enablePromiseHook(fastPromiseHook); + } } function disableHooks() { async_hook_fields[kCheck] -= 1; wantPromiseHook = false; + // Delay the call to `disablePromiseHook()` because we might currently be // between the `before` and `after` calls of a Promise. enqueueMicrotask(disablePromiseHookIfNecessary); } function disablePromiseHookIfNecessary() { - if (!wantPromiseHook) + if (!wantPromiseHook) { + promiseHookMode = -1; disablePromiseHook(); + } } // Internal Embedder API // @@ -276,7 +341,7 @@ function newAsyncId() { } function getOrSetAsyncId(object) { - if (object.hasOwnProperty(async_id_symbol)) { + if (ObjectPrototypeHasOwnProperty(object, async_id_symbol)) { return object[async_id_symbol]; } @@ -447,6 +512,7 @@ module.exports = { }, enableHooks, disableHooks, + updatePromiseHookMode, clearDefaultTriggerAsyncId, clearAsyncIdStack, hasAsyncIdStack, diff --git a/src/async_wrap.cc b/src/async_wrap.cc index 42837e09818ec2..75e34c95b44977 100644 --- a/src/async_wrap.cc +++ b/src/async_wrap.cc @@ -39,7 +39,9 @@ using v8::HandleScope; using v8::Integer; using v8::Isolate; using v8::Local; +using v8::Maybe; using v8::MaybeLocal; +using v8::Name; using v8::Number; using v8::Object; using v8::ObjectTemplate; @@ -189,46 +191,107 @@ void AsyncWrap::EmitAfter(Environment* env, double async_id) { class PromiseWrap : public AsyncWrap { public: - enum InternalFields { - kIsChainedPromiseField = AsyncWrap::kInternalFieldCount, - kInternalFieldCount - }; PromiseWrap(Environment* env, Local object, bool silent) : AsyncWrap(env, object, PROVIDER_PROMISE, kInvalidAsyncId, silent) { MakeWeak(); } + PromiseWrap(Environment* env, Local object, double asyncId, + double triggerAsyncId) + : AsyncWrap(env, object, PROVIDER_PROMISE, asyncId, triggerAsyncId) { + MakeWeak(); + } + SET_NO_MEMORY_INFO() SET_MEMORY_INFO_NAME(PromiseWrap) SET_SELF_SIZE(PromiseWrap) static PromiseWrap* New(Environment* env, Local promise, - PromiseWrap* parent_wrap, bool silent); - static void getIsChainedPromise(Local property, - const PropertyCallbackInfo& info); + static void GetAsyncId(Local property, + const PropertyCallbackInfo& args); + static void GetTriggerAsyncId(Local property, + const PropertyCallbackInfo& args); + + static void Initialize(Environment* env); }; PromiseWrap* PromiseWrap::New(Environment* env, Local promise, - PromiseWrap* parent_wrap, bool silent) { + Local context = env->context(); + Local obj; - if (!env->promise_wrap_template()->NewInstance(env->context()).ToLocal(&obj)) + if (!env->promise_wrap_template()->NewInstance(context).ToLocal(&obj)) return nullptr; - obj->SetInternalField(PromiseWrap::kIsChainedPromiseField, - parent_wrap != nullptr ? v8::True(env->isolate()) - : v8::False(env->isolate())); + CHECK_NULL(promise->GetAlignedPointerFromInternalField(0)); promise->SetInternalField(0, obj); + + // Skip for init events + if (silent) { + Local maybeAsyncId = promise + ->Get(context, env->async_id_symbol()) + .ToLocalChecked(); + + Local maybeTriggerAsyncId = promise + ->Get(context, env->trigger_async_id_symbol()) + .ToLocalChecked(); + + if (maybeAsyncId->IsNumber() && maybeTriggerAsyncId->IsNumber()) { + double asyncId = maybeAsyncId->NumberValue(context).ToChecked(); + double triggerAsyncId = maybeTriggerAsyncId->NumberValue(context) + .ToChecked(); + return new PromiseWrap(env, obj, asyncId, triggerAsyncId); + } + } + return new PromiseWrap(env, obj, silent); } -void PromiseWrap::getIsChainedPromise(Local property, - const PropertyCallbackInfo& info) { - info.GetReturnValue().Set( - info.Holder()->GetInternalField(PromiseWrap::kIsChainedPromiseField)); +void PromiseWrap::GetAsyncId(Local property, + const PropertyCallbackInfo& info) { + Isolate* isolate = info.GetIsolate(); + HandleScope scope(isolate); + + PromiseWrap* wrap = Unwrap(info.Holder()); + double value = wrap->get_async_id(); + + info.GetReturnValue().Set(Number::New(isolate, value)); +} + +void PromiseWrap::GetTriggerAsyncId(Local property, + const PropertyCallbackInfo& info) { + Isolate* isolate = info.GetIsolate(); + HandleScope scope(isolate); + + PromiseWrap* wrap = Unwrap(info.Holder()); + double value = wrap->get_trigger_async_id(); + + info.GetReturnValue().Set(Number::New(isolate, value)); +} + +void PromiseWrap::Initialize(Environment* env) { + Isolate* isolate = env->isolate(); + HandleScope scope(isolate); + + Local ctor = FunctionTemplate::New(isolate); + ctor->SetClassName(FIXED_ONE_BYTE_STRING(isolate, "PromiseWrap")); + + Local promise_wrap_template = ctor->InstanceTemplate(); + env->set_promise_wrap_template(promise_wrap_template); + + promise_wrap_template->SetInternalFieldCount( + PromiseWrap::kInternalFieldCount); + + promise_wrap_template->SetAccessor( + env->async_id_symbol(), + PromiseWrap::GetAsyncId); + + promise_wrap_template->SetAccessor( + env->trigger_async_id_symbol(), + PromiseWrap::GetTriggerAsyncId); } static PromiseWrap* extractPromiseWrap(Local promise) { @@ -240,8 +303,35 @@ static PromiseWrap* extractPromiseWrap(Local promise) { return obj->IsObject() ? Unwrap(obj.As()) : nullptr; } -static void PromiseHook(PromiseHookType type, Local promise, - Local parent) { +static uint16_t ToAsyncHooksType(PromiseHookType type) { + switch (type) { + case PromiseHookType::kInit: return AsyncHooks::kInit; + case PromiseHookType::kBefore: return AsyncHooks::kBefore; + case PromiseHookType::kAfter: return AsyncHooks::kAfter; + case PromiseHookType::kResolve: return AsyncHooks::kPromiseResolve; + } +} + +// Simplified JavaScript hook fast-path for when there is no destroy hook +static void FastPromiseHook(PromiseHookType type, Local promise, + Local parent) { + Local context = promise->CreationContext(); + Environment* env = Environment::GetCurrent(context); + if (env == nullptr) return; + + Local argv[] = { + Integer::New(env->isolate(), ToAsyncHooksType(type)), + promise, + parent + }; + + TryCatchScope try_catch(env, TryCatchScope::CatchMode::kFatal); + Local promise_hook = env->promise_hook_handler(); + USE(promise_hook->Call(context, Undefined(env->isolate()), 3, argv)); +} + +static void FullPromiseHook(PromiseHookType type, Local promise, + Local parent) { Local context = promise->CreationContext(); Environment* env = Environment::GetCurrent(context); @@ -261,14 +351,14 @@ static void PromiseHook(PromiseHookType type, Local promise, Local parent_promise = parent.As(); PromiseWrap* parent_wrap = extractPromiseWrap(parent_promise); if (parent_wrap == nullptr) { - parent_wrap = PromiseWrap::New(env, parent_promise, nullptr, true); + parent_wrap = PromiseWrap::New(env, parent_promise, true); if (parent_wrap == nullptr) return; } AsyncHooks::DefaultTriggerAsyncIdScope trigger_scope(parent_wrap); - wrap = PromiseWrap::New(env, promise, parent_wrap, silent); + wrap = PromiseWrap::New(env, promise, silent); } else { - wrap = PromiseWrap::New(env, promise, nullptr, silent); + wrap = PromiseWrap::New(env, promise, silent); } } @@ -295,7 +385,6 @@ static void PromiseHook(PromiseHookType type, Local promise, } } - static void SetupHooks(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -324,34 +413,28 @@ static void SetupHooks(const FunctionCallbackInfo& args) { SET_HOOK_FN(destroy); SET_HOOK_FN(promise_resolve); #undef SET_HOOK_FN - - { - Local ctor = - FunctionTemplate::New(env->isolate()); - ctor->SetClassName(FIXED_ONE_BYTE_STRING(env->isolate(), "PromiseWrap")); - Local promise_wrap_template = ctor->InstanceTemplate(); - promise_wrap_template->SetInternalFieldCount( - PromiseWrap::kInternalFieldCount); - promise_wrap_template->SetAccessor( - FIXED_ONE_BYTE_STRING(env->isolate(), "isChainedPromise"), - PromiseWrap::getIsChainedPromise); - env->set_promise_wrap_template(promise_wrap_template); - } } - static void EnablePromiseHook(const FunctionCallbackInfo& args) { - args.GetIsolate()->SetPromiseHook(PromiseHook); + Environment* env = Environment::GetCurrent(args); + + if (args[0]->IsFunction()) { + env->set_promise_hook_handler(args[0].As()); + args.GetIsolate()->SetPromiseHook(FastPromiseHook); + } else { + args.GetIsolate()->SetPromiseHook(FullPromiseHook); + } } static void DisablePromiseHook(const FunctionCallbackInfo& args) { - Isolate* isolate = args.GetIsolate(); + Environment* env = Environment::GetCurrent(args); + env->set_promise_hook_handler(Local()); // The per-Isolate API provides no way of knowing whether there are multiple // users of the PromiseHook. That hopefully goes away when V8 introduces // a per-context API. - isolate->SetPromiseHook(nullptr); + args.GetIsolate()->SetPromiseHook(nullptr); } @@ -577,6 +660,9 @@ void AsyncWrap::Initialize(Local target, FIXED_ONE_BYTE_STRING(env->isolate(), "AsyncWrap"), AsyncWrapObject::GetConstructorTemplate(env) ->GetFunction(env->context()).ToLocalChecked()).Check(); + + // TODO(qard): maybe this should be GetConstructorTemplate instead? + PromiseWrap::Initialize(env); } @@ -600,6 +686,15 @@ AsyncWrap::AsyncWrap(Environment* env, init_hook_ran_ = true; } +AsyncWrap::AsyncWrap(Environment* env, + Local object, + ProviderType provider, + double execution_async_id, + double trigger_async_id) + : AsyncWrap(env, object, provider, execution_async_id, true) { + trigger_async_id_ = trigger_async_id; +} + AsyncWrap::AsyncWrap(Environment* env, Local object) : BaseObject(env, object) { } diff --git a/src/async_wrap.h b/src/async_wrap.h index 34e84fde6d4823..dac86d694ac28e 100644 --- a/src/async_wrap.h +++ b/src/async_wrap.h @@ -209,6 +209,11 @@ class AsyncWrap : public BaseObject { ProviderType provider, double execution_async_id, bool silent); + AsyncWrap(Environment* env, + v8::Local promise, + ProviderType provider, + double execution_async_id, + double trigger_async_id); ProviderType provider_type_ = PROVIDER_NONE; bool init_hook_ran_ = false; // Because the values may be Reset(), cannot be made const. diff --git a/src/env.h b/src/env.h index 182ea4e4077b21..0f583fa6af0068 100644 --- a/src/env.h +++ b/src/env.h @@ -156,11 +156,13 @@ constexpr size_t kFsStatsBufferLength = // Symbols are per-isolate primitives but Environment proxies them // for the sake of convenience. #define PER_ISOLATE_SYMBOL_PROPERTIES(V) \ + V(async_id_symbol, "async_id_symbol") \ V(handle_onclose_symbol, "handle_onclose") \ V(no_message_symbol, "no_message_symbol") \ V(oninit_symbol, "oninit") \ V(owner_symbol, "owner") \ V(onpskexchange_symbol, "onpskexchange") \ + V(trigger_async_id_symbol, "trigger_async_id_symbol") \ // Strings are per-isolate primitives but Environment proxies them // for the sake of convenience. Strings should be ASCII-only. @@ -457,6 +459,7 @@ constexpr size_t kFsStatsBufferLength = V(prepare_stack_trace_callback, v8::Function) \ V(process_object, v8::Object) \ V(primordials, v8::Object) \ + V(promise_hook_handler, v8::Function) \ V(promise_reject_callback, v8::Function) \ V(script_data_constructor_function, v8::Function) \ V(source_map_cache_getter, v8::Function) \ diff --git a/test/addons/async-hooks-promise/test.js b/test/addons/async-hooks-promise/test.js index a6c48e94a34f07..d38bf9bd978103 100644 --- a/test/addons/async-hooks-promise/test.js +++ b/test/addons/async-hooks-promise/test.js @@ -1,8 +1,11 @@ 'use strict'; +// Flags: --expose-internals const common = require('../../common'); const assert = require('assert'); const async_hooks = require('async_hooks'); +const { async_id_symbol, + trigger_async_id_symbol } = require('internal/async_hooks').symbols; const binding = require(`./build/${common.buildType}/binding`); if (process.env.NODE_TEST_WITH_ASYNC_HOOKS) { @@ -15,28 +18,60 @@ assert.strictEqual( binding.getPromiseField(Promise.resolve(1)), 0); -const hook0 = async_hooks.createHook({}).enable(); +const emptyHook = async_hooks.createHook({}).enable(); // Check that no PromiseWrap is created when there are no hook callbacks. assert.strictEqual( binding.getPromiseField(Promise.resolve(1)), 0); -hook0.disable(); +emptyHook.disable(); + +let lastResource; +let lastAsyncId; +let lastTriggerAsyncId; +const initOnlyHook = async_hooks.createHook({ + init(asyncId, type, triggerAsyncId, resource) { + lastAsyncId = asyncId; + lastTriggerAsyncId = triggerAsyncId; + lastResource = resource; + } +}).enable(); + +// Check that no PromiseWrap is created when only using an init hook. +{ + const promise = Promise.resolve(1); + assert.strictEqual(binding.getPromiseField(promise), 0); + assert.strictEqual(lastResource, promise); + assert.strictEqual(lastAsyncId, promise[async_id_symbol]); + assert.strictEqual(lastTriggerAsyncId, promise[trigger_async_id_symbol]); +} + +initOnlyHook.disable(); + +lastResource = null; +const hookWithDestroy = async_hooks.createHook({ + init(asyncId, type, triggerAsyncId, resource) { + lastAsyncId = asyncId; + lastTriggerAsyncId = triggerAsyncId; + lastResource = resource; + }, + + destroy() { -let pwrap = null; -const hook1 = async_hooks.createHook({ - init(id, type, tid, resource) { - pwrap = resource; } }).enable(); // Check that the internal field returns the same PromiseWrap passed to init(). -assert.strictEqual( - binding.getPromiseField(Promise.resolve(1)), - pwrap); +{ + const promise = Promise.resolve(1); + const promiseWrap = binding.getPromiseField(promise); + assert.strictEqual(lastResource, promiseWrap); + assert.strictEqual(lastAsyncId, promiseWrap[async_id_symbol]); + assert.strictEqual(lastTriggerAsyncId, promiseWrap[trigger_async_id_symbol]); +} -hook1.disable(); +hookWithDestroy.disable(); // Check that internal fields are no longer being set. This needs to be delayed // a bit because the `disable()` call only schedules disabling the hook in a @@ -45,4 +80,25 @@ setImmediate(() => { assert.strictEqual( binding.getPromiseField(Promise.resolve(1)), 0); + + const noDestroyHook = async_hooks.createHook({ + init(asyncId, type, triggerAsyncId, resource) { + lastAsyncId = asyncId; + lastTriggerAsyncId = triggerAsyncId; + lastResource = resource; + }, + + before() {}, + after() {}, + resolve() {} + }).enable(); + + // Check that no PromiseWrap is created when there is no destroy hook. + const promise = Promise.resolve(1); + assert.strictEqual(binding.getPromiseField(promise), 0); + assert.strictEqual(lastResource, promise); + assert.strictEqual(lastAsyncId, promise[async_id_symbol]); + assert.strictEqual(lastTriggerAsyncId, promise[trigger_async_id_symbol]); + + noDestroyHook.disable(); }); diff --git a/test/parallel/test-async-hooks-promise.js b/test/parallel/test-async-hooks-promise.js index 637d287b506ba9..9db510e329ffad 100644 --- a/test/parallel/test-async-hooks-promise.js +++ b/test/parallel/test-async-hooks-promise.js @@ -24,6 +24,4 @@ const a = Promise.resolve(42); a.then(common.mustCall()); assert.strictEqual(initCalls[0].triggerId, 1); -assert.strictEqual(initCalls[0].resource.isChainedPromise, false); assert.strictEqual(initCalls[1].triggerId, initCalls[0].id); -assert.strictEqual(initCalls[1].resource.isChainedPromise, true); diff --git a/test/parallel/test-bootstrap-modules.js b/test/parallel/test-bootstrap-modules.js index e07ca03e807b5d..ee2a40c0a6cd79 100644 --- a/test/parallel/test-bootstrap-modules.js +++ b/test/parallel/test-bootstrap-modules.js @@ -24,6 +24,7 @@ const expectedModules = new Set([ 'Internal Binding process_methods', 'Internal Binding report', 'Internal Binding string_decoder', + 'Internal Binding symbols', 'Internal Binding task_queue', 'Internal Binding timers', 'Internal Binding trace_events',