Repository navigation
async_hooks performance/stability improvement plans #376
Description
Activity
The
InternalCallbackScopefunction 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,InternalCallbackScopeoverlays 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. 🤔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.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.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
.thenand 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 haveawait new Constuctor, though it is well tracked by promises, but there can be nested topics if this would beclassinstead ofasync 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.
@Qard As far as
InternalCallbackScopeis concerned, one possible idea might be to only optimize in the case that the(Internal)MakeCallback()utility is being used. We could passkSkipAsyncHooksto the callback scope to make it not emit the event, then call the trampoline function instead?Reacted by Viktor Vershanskiy and Stephen Belanger@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
oncompletefield on request types. Just as simple asreq.oncomplete = wrap(asyncId, callback)where thewrapfunction 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,
MakeCallbackwas 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. 🤔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.
Reacted by Andrei Pechkurov and Viktor VershanskiyIf 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_contextbut 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/catchis 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.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
answervariable 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 tothis: 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).
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.jseach time when hooks are enabled/disabled. Also,destroyhook presence check inPromiseHookfunction will be fast (a single bitwise operation) and if there are no activedestroyhooks, there would be no need to wrapPromisewithPromiseWrapand assigned async id can be stored as an internal field directly in thePromise. This should decrease the overhead of ALS + native promises combination, while keeping the same overallasync_hooksbehavior (except for resource ininithook not being aPromiseWrap, but ratherPromiseitself).P.S. I'm not good at the native side of
async_hooks, so take what's written above with a grain of salt.Reacted by Viktor VershanskiyCurrently 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 theisChainedPromiseproperty 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 existingregisterDestroyHookto track the destroy for the object only if there's a destroy handler listening for it. 🤔Reacted by Viktor VershanskiyReacted by Andrei Pechkurov and Viktor VershanskiyCurrently 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
EmitTraceEventDestroywhen there are nodestroyhooks?There's also the
isChainedPromiseproperty 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
triggerAsyncIdand 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
PromiseHookhandler into JavaScript and re-implement the wrap object on the JS side, maybe. I could just use the existingregisterDestroyHookto 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
AsyncResourceon JS side, which already skipsdestroyemit 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.
Reacted by Viktor VershanskiyEmitTraceEventDestroyis for thetrace_eventsAPI, notasync_hooks. Not calling it would be a breaking change, I think.The
isChainedPromiseis 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
AsyncResourcecomment, 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. TheAsyncResourceclass 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 toAsyncResourceor 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.Reacted by Andrei Pechkurov and Andreas Madsen18 remaining items
I agree that the
then(...)call is the most "correct" representation of the async barrier and we should therefore be creating a resource at that level. However, because await runs that in an unwrapped microtask that would break everything, the way things are currently. We would need a way to be able to capture and wrap the context for each microtask for it to run as within the queue. Perhaps we could supply our own customMicrotaskQueue?Reacted by Viktor Vershanskiy and Andreas MadsenWe would need a way to be able to capture and wrap the context for each microtask for it to run as within the queue. Perhaps we could supply our own custom MicrotaskQueue?
Makes sense. I think that would also be a good approach for hooking into the native Promise APIs to support thenables.
Reacted by Viktor VershanskiyThis will be a problem if a new context scope is started in the middle of the chain. In essence, we loose information about the async chain here, as the correct chain is root -> A -> B -> C.
- I disagree that the "correct chain" is
root -> A -> B -> C. I think there are multiple correct chains. This is also discussed in the old Microsoft paper on the subject. - I just want to point out that the current default context is not
root -> A -> B -> C:
Yes, you're right. Both chains can be thought as "correct". And the current behavior is
root -> A, root -> B, root -> Cbecause of intermediate promises which have root promise as the parent.No, that would just make a bad situation worse.
async_hooksis already mixed with promise specific semantics, such asresolve. Mixingpromise_hookswithasync_hooksjust makes things worse.Makes sense. Sounds like a change that could be shipped in the next major release.
The only potential problem that I can foresee is the fact that
AsyncLocalStoragecurrently stores the context in a symbol property of the promise wrap (or the promise itself, if we consider #32891). With the proposed change a single promise may stand for multiple async resources, yet the context is stored in a single place. Maybe we could fix that with a wrap per eachtheninvocation, but it may lead to races because of the asynchronous execution of the callback. WDYT?Reacted by Viktor Vershanskiy- I disagree that the "correct chain" is
Sounds like a change that could be shipped in the next major release.
Yes, this would be a breaking change. So definitely for next major release.
The only potential problem that I can foresee is the fact that AsyncLocalStorage currently stores the context in a symbol property of the promise wrap (or the promise itself, if we consider #32891). With the proposed change a single promise may stand for multiple async resources, yet the context is stored in a single place. Maybe we could fix that with a wrap per each then invocation, but it may lead to races because of the asynchronous execution of the callback. WDYT?
I don't really understand. There will still be an object to attach properties too, you could even make it the promise object returned by
.then(). The callbacks will be executed in the order they are queued, no difference there. It is also possible that using object properties will be less necessary since this would meditate a lot of the performance issues withdestroy.I don't really understand.
Update. The snippet below was updated, as the initial one wasn't correct.
I'm speaking of timing issues like the following one:
const als = new AsyncLocalStorage(); const aPromise = new Promise((resolve, reject) => { setTimeout(resolve, 0); }); als.run(1, () => { aPromise.then(() => { // store 1 gets propagated here console.log(als.getStore()); // 2 - Incorrect, should be 1 }); als.run(2, () => { aPromise.then(() => { // store 2 gets propagated here and overwrites store 1 console.log(als.getStore()); // 2 - Correct }); }); });
The above snippet assumes current implementation of
AsyncLocalStorageand the proposed Promise +async_hooksintegration. It's a bit synthetic and, yes, the current code base may also lead to weird behavior in edge cases (yet, they're different). But my point is that a single property in combination with the proposed behavior may lead to certain problems and it should be improved somehow during the change.There will still be an object to attach properties too, you could even make it the promise object returned by
.then().Yes, in any case we should have a separate resource object per
initemit.It is also possible that using object properties will be less necessary since this would meditate a lot of the performance issues with
destroy.AsyncResourcestill uses GC-tracking by default. Also, additional hooks have a certain overhead themselves. So,destroy-hook based CLS implementations may still be slower even after the discussed change.The above snippet assumes current implementation of AsyncLocalStorage and the proposed Promise + async_hooks integration. It's a bit synthetic and, yes, the current code base may also lead to weird behavior in edge cases (yet, they're different). But my point is that a single property in combination with the proposed behavior may lead to certain problems and it should be improved somehow during the change.
This is a problem with
AsyncLocalStorage. I was not here for when it was made, so I don't know the background for why it was designed the way it is. As far as I can see,.run()introduces a memory leak, which is also what leads to the bug you mention.In any case, the issue you describe is no different if
process.nextTickwas used. Therefore, it has nothing to do with my proposed change to Promise integration inaynnc_hooks.const als = new AsyncLocalStorage(); als.run(1, () => { process.nextTick(() => { // store 1 gets propagated here console.log(als.getStore()); // 2 - Incorrect, should be 1 }); als.run(2, () => { process.nextTick(() => { // store 2 gets propagated here and overwrites store 1 console.log(als.getStore()); // 2 - Correct }); }); });
This is a problem with
AsyncLocalStorage. I was not here for when it was made, so I don't know the background for why it was designed the way it is. As far as I can see,.run()introduces a memory leak, which is also what leads to the bug you mention.What makes you think it's a mem leak? In fact ALS is more memory safe than
destroy-based CLS implementations, as it relies on GC of async resources.In any case, the issue you describe is no different if
process.nextTickwas used. Therefore, it has nothing to do with my proposed change to Promise integration inasync_hooks.const als = new AsyncLocalStorage(); als.run(1, () => { process.nextTick(() => { // store 1 gets propagated here console.log(als.getStore()); // 2 - Incorrect, should be 1 }); als.run(2, () => { process.nextTick(() => { // store 2 gets propagated here and overwrites store 1 console.log(als.getStore()); // 2 - Correct }); }); });
I've just tried running this snippet on node 14.2.0 and got
1and2printed in console, which is the expected behavior. That happens because eachprocess.nextTickinvocation is tracked as a separate async resource, so propagation works correctly in this code.So, my original snippet, which was showing a potential problem with the proposed promise integration and current ALS implementation, still looks like a valid one.
What makes you think it's a mem leak? In fact ALS is more memory safe than destroy-based CLS implementations, as it relies on GC of async resources.
I could be wrong. That is anyway a separate discussion.
I've just tried running this snippet on node 14.2.0 and got 1 and 2 printed in console, which is the expected behavior. That happens because each process.nextTick invocation is tracked as a separate async resource, so propagation works correctly in this code.
That is great. If it works with
process.nextTickit will work with.then. My proposed change makes them behave the same. edit: well, similar toqueueMicrotask().Reacted by Matteo CollinaI could be wrong. That is anyway a separate discussion.
Agreed.
That is great. If it works with
process.nextTickit will work with.then. My proposed change makes them behave the same.With the proposed behavior in promise integration, the original snippet (the one with a promise) will produce incorrect results. Luckily, it may be fixed by having a separate resource object per
theninvocation. That's just something that should be kept in mind when implementing your proposal.With the proposed behavior in promise integration, the original snippet (the one with a promise) will produce incorrect results. Luckily, it may be fixed by having a separate resource object per then invocation. That's just something that should be kept in mind when implementing your proposal.
Great. Please don't assume anymore that I intended to call the
[[init]]multiple times on the same resource. That was never the case and it leads to a strawman discussion that is a waste of time.I have clarified in the initial proposal that a new resource object should be created, or alternatively the returned the Promise by
[[then]]is used as the resource.Reacted by Andrei PechkurovI have clarified in the initial proposal that a new resource object should be created, or alternatively the returned the Promise by
[[then]]is used as the resource.Good to know. Thanks.
Does it make sense to move your proposal to a separate GH issue?
@puzpuzpuz Alright, I wrote my proposal as its own issue. See: #389
Reacted by Andrei Pechkurov and Stephen BelangerThis 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.
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
AsyncLocalStorageclass intended for context management aims to improve performance over traditional init/destroy-based context management by using only the init hook and theexecutionAsyncResourcefunction. 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
executionAsyncResourceexisting 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 anAsyncWrapinstance 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
InternalCallbackScopeto 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
oncompletefield 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. 😅