[async_wrap] enable/disable PromiseHook depending on kTotals

[trevnorris]

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]

@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]

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]

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

[trevnorris]

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.

[AndreasMadsen]

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?

[matthewloring]

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?

[trevnorris]

@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.

[AndreasMadsen]

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])

[trevnorris]

@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?

[matthewloring]

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

[AndreasMadsen]

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

[trevnorris]

@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.

[trevnorris]

@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/

[trevnorris]

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]

The CI failures are unrelated or infrastructure-related.

Landed in dde4f0f...a2fdb76

[trevnorris]

@AndreasMadsen@addaleax Thanks much for the reviews and for getting it landed.