Proposal for Promise hooks to improve performance.

[hashseed]

@bmeurer, @ofrobots and I discussed and put some thoughts together. Comments welcome!

[bnoordhuis]

"You need permission" - is that intentional?

[AndreasMadsen]

@hashseed I think you need to make the document public.

[hashseed]

Ah yeah. Totally forgot. Thanks.

[hashseed]

As discussed during the WG meeting, @mcollina is going to reach out to find out why async hooks were designed around async IDs and whether adding the resource would be ok.

[Qard]

Definitely a 👍 from me. There's no reason for all this work to be done on the native side. No one uses it there.

One thing I'd like to bring up, which I think is relevant, and I have commented on several times in the past is that async_hooks and the previous iteration in AsyncListener both made the mistake of conflating resource lifecycle with asynchronous boundary linkage. I think that has long been a major failing, preventing the feature from working optimally.

Some want to track the resource lifecycle, which makes sense for exhaustible resources like sockets and file descriptors. However, that really doesn't make sense for something like promise hooks. Promises are not an exhaustible resource, the object itself is not actually relevant. For asynchronous continuity, it's actually the callback that we care about--namely, linking where it was declared to where it was executed.

I feel like it would have made a lot more sense to have one system which tracks creation and destruction of true resource handles, and then a completely separate feature used exclusively for connecting async task init points to the callback start and end. Resources would generally need native code to track those lifecycle points, but the barrier tracking can be implemented 100% in JS. (I actually wrote exactly this as an experiment several years ago. https://github.com/qard/stacks-concept)

[AndreasMadsen]

I'm -1. Maybe this is the correct approach, but it completly skips over the memory issues that have been reported. I think the memory issue is actually what we have gotten most uninfluced feedback about (feedback where we didn't ask directly, "what is the performance overhead").

In fact two companies have reached out to me personally about memory issues, non about computationally issues. There are also some issues:

• async hooks promise not destroyed node#14446
• Memory leak in _contexts Jeff-Lewis/cls-hooked#11

I remember there being more, but perhaps they are not in nodejs/node.

As discussed during the WG meeting, @mcollina is going to reach out to find out why async hooks were designed around async IDs and whether adding the resource would be ok.

Two reasons:

Reason 1:

The destoy hook was added because the rest isn't always enogth information. It would allow you to:

• Find unresolved promises
• Know when a resource will no longer emit (setInterval, http.createServer)
• Track active handles

If you want to get meta information from the init hook (such as the stack-trace) in the destroy hook, then you can't use a WeekMap. Because when the destroy hook is emitted, the resource object is GC'ed and thus you have lost your WeekMap key. That is why we use the asyncId.

Reason 2:

WeekMap have a very bad memory profile.

In AndreasMadsen/trace#17 someone reported that trace used an extream amout of memory on something as simple as:

require('trace')
var stream = require('fs').createReadStream('huge file here')
stream.resume()

Turns out that WeakMap are really bad for memory consumtion, especially in cases where you get a very determinstic destroy hook, such as the nextTick case.

WeakMap using handle object

Map using uid and destructor event

Removing the destroy hook and using WeakMaps means that we can't efficiently cleanup after nextTick, an async resource that is much more common than promises.

I also understand that to more efficiently garbage collect promises in the future, using ecape-analysis, we can't depend on the promise object in the init hook. Something that this proposal makes us very depedent on. And inefficient garbage collection for promises is still going to be an issue with WeakMaps, as the value can't be GC'ed before the key is GC'ed and for that we would need ecape-analysis.

[hashseed]

I haven't had time to prepare response for your entire answer, which btw is very detailed, thanks.

Just some thoughts:

• Escape analysis is already not possible due to exposing the promise to the init hook.
• We also cannot perform escape analysis with the current PromiseHook API that goes through C++, since we cannot inline across calls into C++.
• What do you think about the other two parts of the proposal (skip hooks for uninteresting Promises and move the API to JS)?

[AndreasMadsen]

• Escape analysis is already not possible due to exposing the promise to the init hook.

We don't need to expose the promise object in the init hook. The only use case is to call promise.then((value) => {}) and the https://github.com/angular/zone.js team would actually prefer to get the value in resolve(value) through the promiseResolve hook.

• We also cannot perform escape analysis with the current PromiseHook API that goes through C++, since we cannot inline across calls into C++.

In C++ we only use the promise object for assigning asyncId and emitting the destroy hook. If V8 provided an API for that we wouldn't need the promise object in PromiseHook at all!.

I have said this quite a few times, I think most details is in here: nodejs/benchmarking#181 (comment) and comments futher down.

What do you think about the other two parts of the proposal (skip hooks for uninteresting Promises and move the API to JS)?

Should be fine. The WeakRef would also allow us to replace https://github.com/nodejs/node/blob/master/lib/async_hooks.js#L169 with is currently a big performance penalty in AsyncHooks integration for bluebird promises.

[github-actions]

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

[mmarchini]

@Qard do you want to keep this issue opened?

[Qard]

Everything that can be done from Node.js perspective has already been done, so I think it can be closed.

It would still be nice to have V8 supply a version of the API which takes v8::Function though.

It may also be worth looking into skipping non-observable promises, though it seems like the V8 folks have actually just tried to eliminate some of those altogether since the doc for that issue was originally written. Not sure how relevant that last idea is at this point.