async_hooks performance/stability improvement plans

[Qard]

Over the next several months I plan to work on async_hooks improvements full-time. I have several experiments I intend to try, so I will document my plans here and link to the PRs as I progress. I'm not sure how much of this I'll be able to finish in that time, but this is my rough plan and likely priority order. Let me know if you have any questions or suggestions for other things to look at.

Avoid binding all event handlers when only some are used

The native side of async_hooks was designed to only handle one set of hooks at a time and the JavaScript side abstracts that with a design which shares one native hook set across many JavaScript hook sets. It currently will bind all hook functions even if some are never used.

This is particularly a problem because the new AsyncLocalStorage class intended for context management aims to improve performance over traditional init/destroy-based context management by using only the init hook and the executionAsyncResource function. This allows it to avoid using the destroy hook, but the internals continue to bind all the other methods even though they are not being used.

Avoid wrapping promises with AsyncWrap if destroy is not used

A deeper issue of executionAsyncResource existing as an alternative to the destroy hook is that the primary performance penalty of the destroy hook is due to needing to wrap every single promise in an AsyncWrap instance in order to track its memory and trigger the destroy hook when it is garbage collected. If the destroy hook is not even being listened to, all that overhead of wrapping the promise, tracking the garbage collection, and queuing destroy events for the next tick (because you can't call JavaScript mid-gc) becomes completely unnecessary.

This issue specifically is currently the largest performance hit of turning on async_hooks in most apps. Eliminating this would be huge.

To deal with this issue, I'm considering a few options. The main one being to expose PromiseHook events directly to JavaScript in a separate API from async_hooks and then having async_hooks join those events into its own event stream purely on the JavaScript side, making use of WeakRef to trigger the destroy events only if there is a destroy hook listening for them. I'm also considering an additional option passed alongside the hooks object to indicate that destroy events are wanted for consumable resource but not for promises--possibly even a filter/subscription API of some sort to explicitly describe which things to listen to or not.

InternalCallbackScope function trampoline

status: on hold - C++ streams and promises make this complicated. Will re-evaluate later.

Currently "before" and "after" events of async_hooks are largely emitted on the C++ side from within the InternalCallbackScope class. It is currently designed in such a way that the "before" and "after" handlers of a hook are triggered by separate entrances into JavaScript. I want to try and create a function trampoline, similar to how timers works, which would involve passing the callback and arguments to a JavaScript function which would trigger the "before" event fully on the JS-side, then call the callback, then trigger the "after" event. This would reduce three separate entrances into the JavaScript layer down to just one. It also opens the door for the next two ideas...

Avoid hop to C++ from AsyncResource events

Currently AsyncResource, which expresses JavaScript side pseudo-asyncrony such as connection pooling mechanics, sends events into the C++ side of async_hooks despite that data only actually being useful to consume on the JavaScript side. That barrier hop is unnecessary and only exists because async_hooks was designed with the expectation that everything is wrapped in a C++ type which is passed through InternalCallbackScope to trigger the lifecycle emitter functions on the wrapper class.

Wrap non-handle request callbacks

In libuv, there are "handles" expressing long-term resources like a socket or file descriptor and there are "requests" to express single-use things. Some request objects are constructed within C++ from handle functions, however many are constructed in JavaScript where it would be possible to wrap the callback assigned to the oncomplete field to trigger the before and after hooks, enabling to bypass the native side of async_hooks entirely.

Bonus Round - Make PromiseHook support thenables

One major stumbling block with context management in Node.js is PromiseHook does not currently support thenables, an object which has a then(...) function on it but is not a real promise. Thenables are commonly used in database and cache libraries, among other things, which breaks every async_hooks user. There are some complex hacks to work around it, but they are complicated, fragile, and depend heavily on sequence timing/ordering of the microtask queue, which is quite risky. PromiseHook needs to support thenables too.

I don't have experience contributing to V8 myself, though I'm relatively familiar with the internals as a compilers enthusiast that enjoys spelunking in the codebase now and then. I could use some assistance here from anyone with experience contributing to V8 and making Node.js builds with custom V8 builds. 😅

[Qard]

The InternalCallbackScope function trampoline idea seems to get rather complicated with C++ streams and promises. I'm thinking I might rearrange things a bit to rely more on wrapping callbacks on the JS side, focusing on things like the non-handle requests callbacks, and seeing where I can go from there. As it is currently, InternalCallbackScope overlays too much functionality deeper into the stack in many places that changing it to receive a callback and arguments would need fairly far-reaching changes. I also think it would likely be rendered somewhat redundant by efforts to wrap the callbacks before even passing it into the C++ side. I think I'm going to skip that one for now. 🤔

[Flarna]

Thanks a lot for working on this with that a high focus!

In general I would prefer correctness over performance which would pull the last point to the first.
But I fully understand that it is done the other way around as the last item has a hard dependency to v8. Therefore it's difficult to progress and most likely hard to get it done within the time-frame you mentioned. If we could at least get some inputs from V8 team how to progress in this would be good.

[puzpuzpuz]

Many thanks for describing these improvements!

I'm specifically interested in improving AsyncLocalStorage + native promises performance (section "Avoid wrapping promises with AsyncWrap if destroy is not used"). Please, let me know if there is something that can be done to help you here.

[wentout]

Great initiative! Thank you very much for starting this!

I'm interested in wrapping argumets for callbacks, if this can help: if we start tracking from callback then all it's argumments can be wrapped too, on JS side, I can show examples if necessary. It is an easiest way to controll execution path. The same idea is chaining for .then and in deep. The hardiest thing to controll is when some function creates function inside and then pushes it somewhere, IDK can it be tracked or not. And the final step is Async Constructor, when we have await new Constuctor, though it is well tracked by promises, but there can be nested topics if this would be class instead of async function, because inheritance chain is not the same for classes.

I think it is connected to #375.

Also here is possible solution for synchronous callback tracking, thought it shouldn't be done this way, because it is solution for userland impl.

[addaleax]

@Qard As far as InternalCallbackScope is concerned, one possible idea might be to only optimize in the case that the (Internal)MakeCallback() utility is being used. We could pass kSkipAsyncHooks to the callback scope to make it not emit the event, then call the trampoline function instead?

[Qard]

@Flarna Correctness is definitely one of my priorities. The ultimate goal is to ensure that async_hooks is both fast and reliable, correctness being part of reliability. Currently promises are the main thing interfering with it being fast and thenables are one of the main things intefering with it being reliable so those are two primary areas of focus in my plans here. If you can think of anything else to investigate which falls into either of those categories, I'm happy to have a look.

@wentout I'd be interested to see any examples of your thinking there. My basic idea is to wrap callbacks assigned to the oncomplete field on request types. Just as simple as req.oncomplete = wrap(asyncId, callback) where the wrap function basically just does this:

function wrap (asyncId, callback) {
  return function (...args) {
    hooks.before(asyncId)
    callback.apply(this, args)
    hooks.after(asyncId)
  }
}

It'd step over the whole JavaScript to native jump and just go directly to the JavaScript-side event router.

@addaleax Yep, MakeCallback was my motivation for the idea. Core seems to rely more on the scope form to hold the context through a deeper stack though. If the stack was shallower it might be reasonable but it gets fairly deep with C++ streams. It's also fully blocked by native promises, so those at least would need to keep the scope-based separated before and after. I'll re-evaluate it later though. 🤔

[Qard]

I've re-arranged things a bit to focus first on not binding handlers that aren't used and on avoiding wrapping promises. I'll get back to the JavaScript-side function trampoline and wrapper ideas later. They'll probably be influenced by the changes to split up handler bindings anyway.

[Flarna]

If you can think of anything else to investigate which falls into either of those categories, I'm happy to have a look.

@Qard Unfortunatelly there is one more place that I have in mind: napi_make_callback has support for async_context but this works different for async_ids then for async resources. see also nodejs/node#32063 (comment)

Please don't take this as task given by me. And in general feel free to ping me if you need some more infos/help even I can't promise a specific amount of time to spend.

Another topic which was also controversal in the past was which execution context to link through Promises. Currently execution path is passed from Promise creation to then/catch callbacks. Another option would be to link the context where then/catch is called instead creation. For a lot usecases this is anyway the same.
But honeslty speaking I'm not sure if we should revisit this. Just want to have it here for completness.

[wentout]

@Qard

I'd be interested to see any examples of your thinking there...:

Sharing the same idea my own outdated CLS implementations covers bit more. Please look for answer variable below:

function wrap (asyncId, callback) {
  return function (...args) {
    hooks.before(asyncId);
    const answer = callback.apply(this, args);
    hooks.after(asyncId);
    if (typeof answer === 'function') {
      // might be here is necessary to wrap answer again
      // to finish the execution path... but it depends on impl
    }
    return answer;
  }
}

Keeping at lease return answer; should help to fit the gap for userland.

It'd step over the whole JavaScript to native jump and just go directly to the JavaScript-side event router.

As I understood, sorry, this would be amazing. If we had a way to see the synchronous split as a separate but sequenced part of asyn_hook IDs, this would cover everything for the User Mode Queuing problem... In an attempt to solve it for business purpose we did a patch for require module with Acorn AST walker to wrap each function with the example above. But performance was so low... so I choosed the other scenario for making code in userland. And that other scenario then followed to the async prototype chain (await new Constructor) and so on, but this still requires CLS as a part of tracing the IDs for underlying code.

And here is a proposal for Async Init of @bmeck
I think this might be connected with this initiative in future if there would be a way to properly figure out of async subclassing. But async constructors for plain function are possible right now, when we return promise from constructor, which then resolves to this: this is an async prototype chain... And this all is already covered for tracing purpose by AsyncLocalStorage from 26540.

Here are slides of Continuation idea.

Meaning, if this is possible, then of splitted Trie of the execution path made by splitting the asyncIDs of synchronous context done by native jump might solve this known traking~tracing problem, when CLS context was missing because topology sorting is not applicable (because there is a hole made by synchronous split).

[puzpuzpuz]

Avoid wrapping promises with AsyncWrap if destroy is not used

...
To deal with this issue, I'm considering a few options.
...
I'm also considering an additional option passed alongside the hooks object to indicate that destroy events are wanted for consumable resource but not for promises--possibly even a filter/subscription API of some sort to explicitly describe which things to listen to or not.

Can it be as simple as a bitset (i.e. an integer value) available on the native side where each bit means presence of any active hooks of a specific type? This bitset could be changed in lib/internal/async_hooks.js each time when hooks are enabled/disabled. Also, destroy hook presence check in PromiseHook function will be fast (a single bitwise operation) and if there are no active destroy hooks, there would be no need to wrap Promise with PromiseWrap and assigned async id can be stored as an internal field directly in the Promise. This should decrease the overhead of ALS + native promises combination, while keeping the same overall async_hooks behavior (except for resource in init hook not being a PromiseWrap, but rather Promise itself).

P.S. I'm not good at the native side of async_hooks, so take what's written above with a grain of salt.

[Qard]

Currently working on changing the PromiseHook handler to avoid needing PromiseWrap. Even if the destroy hook is not registered though, it currently still needs to track the destroy to call EmitTraceEventDestroy. There's also the isChainedPromise property on the wrapped resource to consider. Not sure what the best way to handle this would be, but my hope is that I can just move the entire PromiseHook handler into JavaScript and re-implement the wrap object on the JS side, maybe. I could just use the existing registerDestroyHook to track the destroy for the object only if there's a destroy handler listening for it. 🤔

[puzpuzpuz]

@Qard

Currently working on changing the PromiseHook handler to avoid needing PromiseWrap. Even if the destroy hook is not registered though, it currently still needs to track the destroy to call EmitTraceEventDestroy.

What is the purpose of calling EmitTraceEventDestroy when there are no destroy hooks?

There's also the isChainedPromise property on the wrapped resource to consider.

I know that it's a part of public API, but what's the purpose of having it? It's possible to track parent async resource via triggerAsyncId and check if it's a Promise (although, there may be false positives with the approach in cases when Promises are nested, not chained - but that is a rare case).

Not sure what the best way to handle this would be, but my hope is that I can just move the entire PromiseHook handler into JavaScript and re-implement the wrap object on the JS side, maybe. I could just use the existing registerDestroyHook to track the destroy for the object only if there's a destroy handler listening for it.

The idea sounds really promising IMO. This way we could use AsyncResource on JS side, which already skips destroy emit when possible (see #32429).

How about building a PoC and benchmarking it just to make sure that we get the performance improvement before going any further with a proper implementation? Let me know if there is something I can help you with here.

[Qard]

EmitTraceEventDestroy is for the trace_events API, not async_hooks. Not calling it would be a breaking change, I think.

The isChainedPromise is used by zone.js to identify if a promise has a parent promise or not. I don't know the specifics of why, but they've said before that they need it.

As for the AsyncResource comment, that's basically what I was looking at doing, though I currently have my own separate similar class specific to PromiseHook, because I don't have a callback to wrap but instead distinct before and after events. The AsyncResource class is also currently in the public file rather than the internal file, and the promise hook stuff needs to exist in the internal file, so I'd have to move a bunch of stuff around. For now, I'm just trying to get it to work as a direct port, then I'll see what I can do about switching it to AsyncResource or something like that. I currently have the JS interface exposed and mostly working already, but having some stack corruption issues I'll be investigating today.