[async_wrap] enable/disable `PromiseHook` depending on `kTotals` #13509

trevnorris · 2017-06-06T20:11:43Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

async_wrap, async_hooks

Description

First revert 410b141. There's no need to delay setting the callbacks. No native logic exists that checks if these are empty or not. Except the check that occurs just before setting them.

Then use a portion of #13416 to allow removing a PromiseHook from Environment::promise_hooks_, and add a CHECK() to Environment::AddPromiseHook() to make sure the same fn + arg isn't added twice. (@addaleax I kept you as the author)

Finally track the number of enabled hooks via kTotals and use that to enable/disable AsyncWrap's PromiseHook dynamically.

CI: https://ci.nodejs.org/job/node-test-commit/10405/

trevnorris · 2017-06-06T20:56:54Z

@addaleax Thanks much for the code to remove a promise_hook_func from Environment::promise_hooks_. I hadn't thought to compare both the PromiseHookCallback and the arg. Do you think my additional CHECK_EQ() in Environment::AddPromiseHook() is appropriate?

addaleax · 2017-06-06T20:58:42Z

Do you think my additional CHECK_EQ() in Environment::AddPromiseHook() is appropriate?

Yeah, I think it’s a good idea until we know it’s not – if somebody actually has a use case for attaching identical handlers, we can always drop the check again.

AndreasMadsen

Thanks, I just have a few comments.

edit: Maybe add a test where a promise is created and resolved after the promise hooks are disabled. Just to check that disablePromiseHook works as expected.

AndreasMadsen · 2017-06-06T22:04:21Z

lib/async_hooks.js

    // createHook() has already enforced that the callbacks are all functions,
    // so here simply increment the count of whether each callbacks exists or
    // not.
    hook_fields[kInit] += +!!this[init_symbol];
    hook_fields[kBefore] += +!!this[before_symbol];
    hook_fields[kAfter] += +!!this[after_symbol];
    hook_fields[kDestroy] += +!!this[destroy_symbol];
+    hook_fields[kTotals]++;


It is a silly edge case, but what if there aren't provided any hooks then we don't need to enable promises.

I asked @matthewloring about this on IRC:

<trevnorris> if an async hook is enabled that has no callbacks should we go through the trouble of setting up the PromiseWrap in async_wrap.cc PromiseHook? <mattloring> Is it expected to be a common case to have async hooks enabled with no listeners? If listeners are added later then it might be good to have PromiseWraps available to resolve parent promise ids <trevnorris> it isn't a common case, but i can see your point. so continue to add PromiseWraps to Promises even if there are no callbacks to be called.

@AndreasMadsen My original PR tracked the number of callbacks to be called, and only called enablePromiseHook() if the number of callbacks was > 0. If you think it should be difference feel free to chime in.

I don't understand what @matthewloring mean with:

If listeners are added later then it might be good to have PromiseWraps available to resolve parent promise ids.

Is this related to #13427?

Sorry, reading back over it the comment wasn't correct. I was thinking about the case where async hooks are enabled in the middle of executing a promise chain and the parent promise is not wrapped but I forgot this was handled by adding a silent mode wrap when the child promise is created (https://github.com/trevnorris/node/blob/5a61e784c9b4f03a1341970a0b81819a52a60dde/src/async-wrap.cc#L309). However, the first part of the comment still holds. I was curious whether having async hooks enabled with no callbacks registered was expected to occur frequently enough to make it worth optimizing?

@matthewloring

I was curious whether having async hooks enabled with no callbacks registered was expected to occur frequently enough to make it worth optimizing?

I doubt it's a common case. Honestly I'm looking for what is the most expected behavior, but since creating a new hook w/o any callbacks isn't an expected behavior not sure what to say. Which behavior would you expect?

@AndreasMadsen Personally I like this solution more because the logic is much simpler. Though I can also keep track of number of callbacks as well. I'll leave the call as to what should be done up to others.

I doubt it's a common case. Honestly I'm looking for what is the most expected behavior, but since creating a new hook w/o any callbacks isn't an expected behavior not sure what to say. Which behavior would you expect?

I will expect async_hooks to have the smallest performance impact possible. This includes not enabling promise hooks when there are no hooks.

Personally I like this solution more because the logic is much simpler.

Yeah, that is the one reason why I don't like the additional checks. But I think especially in async_hooks performance is preferred over simplicity. It is also not that difficult/complex to check:

hook_fields[kTotal] += +( !!this[init_symbol] || !!this[before_symbol] || !!this[after_symbol] || !!this[destroy_symbol] )

@AndreasMadsen

It is also not that difficult/complex to check:

You're correct, and that's similar to the check I had before changing it to what's here now.

@matthewloring You agree that Promise hooks should only be enabled if there's actually a callback to call?

Yup, I don't see any other reason they need to be active.

AndreasMadsen · 2017-06-07T15:55:50Z

lib/async_hooks.js

-let setupHooksCalled = false;
+// Setup the callbacks that node::AsyncWrap will call when there are hooks to
+// process. They use the same functions as the JS embedder API.
+async_wrap.setupHooks({ init,


Could you add a comment explaining why we do this immediately instead of waiting for the AsyncHook.enable() call.

trevnorris · 2017-06-07T22:16:00Z

@AndreasMadsen

Maybe add a test where a promise is created and resolved after the promise hooks are disabled. Just to check that disablePromiseHook works as expected.

Added test. Please double check that it's what you were expecting.

This reverts commit 410b141. PR-URL: nodejs#13509

Allow node::PromiseHook (src/async-wrap.cc) to be enabled/disabled from the JavaScript API. PR-URL: nodejs#13509

trevnorris · 2017-06-10T06:23:10Z

@addaleax @AndreasMadsen Needed to include an addon test so I could check the internal field. So running tests one more time

CI: https://ci.nodejs.org/job/node-test-commit/10482/

Keep a total of enabled hook callbacks in kTotals. This value is used to track whether node::PromiseHook (src/async-wrap.cc) should be enabled or disabled. Don't enable node::PromiseHook, using enablePromiseHook(), until a hook has been added. Then, using disablePromiseHook(), disable node::PromiseHook when all hooks have been disabled. Need to use a native test in order to check the internal field of the Promise and check for a PromiseWrap. PR-URL: nodejs#13509

trevnorris · 2017-06-10T08:06:52Z

Test failures are unrelated. I think this is ready to go but would like one more sign-off before landing.

AndreasMadsen

LGTM.

The kTotals setup is a little hard to read. You could just do:

hook_fields[kTotals] = hook_fields[kInit] + hook_fields[kBefore] + hook_fields[kAfter] + hook_fields[kDestroy]

but it is not important to me.

addaleax · 2017-06-10T09:42:21Z

The CI failures are unrelated or infrastructure-related.

Landed in dde4f0f...a2fdb76