1berry/nodejs
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

async_hooks: remove promise object from resource

Browse Source 
While it doesn't make any difference now. In the future PromiseHooks
could be refactored to provide an asyncId instead of the promise object.
That would make escape analysis on promises possible.

Escape analysis on promises could lead to a more efficient destroy hook,
if provide by PromiseHooks as well. But at the very least would allow
the destroy hook to be emitted earlier. The destroy hook not being
emitted on promises frequent enough is a known and reported issue.
See https://github.com/nodejs/node/issues/14446 and
https://github.com/Jeff-Lewis/cls-hooked/issues/11.

While all this is speculation for now, it all depends on the promise
object not being a part of the PromiseWrap resource object.

Ref: https://github.com/nodejs/node/issues/14446
Ref: https://github.com/nodejs/diagnostics/issues/188

PR-URL: https://github.com/nodejs/node/pull/23443
Refs: https://github.com/nodejs/node/issues/14446
Refs: https://github.com/nodejs/diagnostics/issues/188
Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: George Adams <george.adams@uk.ibm.com>
This commit is contained in:
Andreas Madsen 2018-10-12 08:56:26 +02:00 committed by George Adams
parent 561e30d9ef
commit 1a2cf6696f
No known key found for this signature in database
GPG Key ID: 7B8D7E4421A0916D
6 changed files with 5 additions and 34 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
doc/api/async_hooks.md
View File

@ -301,8 +301,7 @@ 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 `promise` property
that refers to the `Promise` that is being initialized, and an
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.

7
lib/domain.js
View File

@ -55,13 +55,6 @@ const asyncHook = createHook({
// if this operation is created while in a domain, let's mark it
pairing.set(asyncId, process.domain);
resource.domain = process.domain;
if (resource.promise !== undefined &&
resource.promise instanceof Promise) {
// resource.promise instanceof Promise make sure that the
// promise comes from the same context
// see https://github.com/nodejs/node/issues/15673
resource.promise.domain = process.domain;
}
}
},
before(asyncId) {

16
src/async_wrap.cc
View File

@ -185,16 +185,13 @@ class PromiseWrap : public AsyncWrap {
SET_MEMORY_INFO_NAME(PromiseWrap)
SET_SELF_SIZE(PromiseWrap)
static constexpr int kPromiseField = 1;
static constexpr int kIsChainedPromiseField = 2;
static constexpr int kInternalFieldCount = 3;
static constexpr int kIsChainedPromiseField = 1;
static constexpr int kInternalFieldCount = 2;
static PromiseWrap* New(Environment* env,
Local<Promise> promise,
PromiseWrap* parent_wrap,
bool silent);
static void GetPromise(Local<String> property,
const PropertyCallbackInfo<Value>& info);
static void getIsChainedPromise(Local<String> property,
const PropertyCallbackInfo<Value>& info);
};
@ -205,7 +202,6 @@ PromiseWrap* PromiseWrap::New(Environment* env,
bool silent) {
Local<Object> object = env->promise_wrap_template()
->NewInstance(env->context()).ToLocalChecked();
object->SetInternalField(PromiseWrap::kPromiseField, promise);
object->SetInternalField(PromiseWrap::kIsChainedPromiseField,
parent_wrap != nullptr ?
v8::True(env->isolate()) :
@ -215,11 +211,6 @@ PromiseWrap* PromiseWrap::New(Environment* env,
return new PromiseWrap(env, object, silent);
}
void PromiseWrap::GetPromise(Local<String> property,
const PropertyCallbackInfo<Value>& info) {
info.GetReturnValue().Set(info.Holder()->GetInternalField(kPromiseField));
}
void PromiseWrap::getIsChainedPromise(Local<String> property,
const PropertyCallbackInfo<Value>& info) {
info.GetReturnValue().Set(
@ -315,9 +306,6 @@ static void SetupHooks(const FunctionCallbackInfo<Value>& args) {
Local<ObjectTemplate> promise_wrap_template = ctor->InstanceTemplate();
promise_wrap_template->SetInternalFieldCount(
PromiseWrap::kInternalFieldCount);
promise_wrap_template->SetAccessor(
FIXED_ONE_BYTE_STRING(env->isolate(), "promise"),
PromiseWrap::GetPromise);
promise_wrap_template->SetAccessor(
FIXED_ONE_BYTE_STRING(env->isolate(), "isChainedPromise"),
PromiseWrap::getIsChainedPromise);

3
test/parallel/test-async-hooks-promise-enable-disable.js
View File

@ -4,7 +4,6 @@ const common = require('../common');
const assert = require('assert');
const async_hooks = require('async_hooks');
const EXPECTED_INITS = 2;
let p_resource = null;
let p_er = null;
let p_inits = 0;
@ -23,7 +22,6 @@ const mustCallInit = common.mustCall(function init(id, type, tid, resource) {
if (type !== 'PROMISE')
return;
p_inits++;
p_resource = resource.promise;
}, EXPECTED_INITS);
const hook = async_hooks.createHook({
@ -36,7 +34,6 @@ new Promise(common.mustCall((res) => {
})).then(common.mustCall((val) => {
hook.enable().enable();
const p = new Promise((res) => res(val));
assert.strictEqual(p, p_resource);
hook.disable();
return p;
})).then(common.mustCall((val2) => {

4
test/parallel/test-async-hooks-promise.js
View File

@ -21,11 +21,9 @@ async_hooks.createHook({
}).enable();
const a = Promise.resolve(42);
const b = a.then(common.mustCall());
a.then(common.mustCall());
assert.strictEqual(initCalls[0].triggerId, 1);
assert.strictEqual(initCalls[0].resource.isChainedPromise, false);
assert.strictEqual(initCalls[0].resource.promise, a);
assert.strictEqual(initCalls[1].triggerId, initCalls[0].id);
assert.strictEqual(initCalls[1].resource.isChainedPromise, true);
assert.strictEqual(initCalls[1].resource.promise, b);

6
test/parallel/test-domain-promise.js
View File

@ -50,7 +50,6 @@ const vm = require('vm');
d2.run(common.mustCall(() => {
p.then(common.mustCall((v) => {
assert.strictEqual(process.domain, d2);
assert.strictEqual(p.domain, d1);
}));
}));
}
@ -64,9 +63,8 @@ const vm = require('vm');
}));
d2.run(common.mustCall(() => {
p.then(p.domain.bind(common.mustCall((v) => {
p.then(d1.bind(common.mustCall((v) => {
assert.strictEqual(process.domain, d1);
assert.strictEqual(p.domain, d1);
})));
}));
}
@ -83,7 +81,6 @@ const vm = require('vm');
d2.run(common.mustCall(() => {
p.then(common.mustCall((v) => {
assert.strictEqual(process.domain, d2);
assert.strictEqual(p.domain, d1);
}));
}));
}));
@ -100,7 +97,6 @@ const vm = require('vm');
d2.run(common.mustCall(() => {
p.catch(common.mustCall((v) => {
assert.strictEqual(process.domain, d2);
assert.strictEqual(p.domain, d1);
}));
}));
}