async_hooks performance/stability improvement plans #376
Comments
|
The |
|
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. |
|
Many thanks for describing these improvements! I'm specifically interested in improving |
|
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 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 |
|
@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 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, |
|
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. |
@Qard Unfortunatelly there is one more place that I have in mind: napi_make_callback has support for 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 |
Sharing the same idea my own outdated CLS implementations covers bit more. Please look for 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
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 ( And here is a proposal for Async Init of @bmeck 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). |
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 P.S. I'm not good at the native side of |
|
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 |
What is the purpose of calling
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
The idea sounds really promising IMO. This way we could use 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. |
|
The As for the |
As far as I can see, So, it should be safe to rely on active
Hmm, I've checked current code base of zone.js and it doesn't seem to be using So, I wonder if there are any users of
That's awesome! I'll be waiting for any updates from you. |
|
That's not what I was referring to with trace_events. There's a separate Not too familiar with the current state of zone.js, so perhaps it's a non-issue. In any case, it's a very edge-casey feature within an already edge-casey feature and it's all marked as experimental so it's probably not a big deal if we just drop it. I'll just see what direction this stuff takes as I progress. 🤷 |
|
I've opened a draft PR with the current state of the code here: nodejs/node#32891 It's not working yet, but demonstrates the general idea. (Though very much not performance-tuned) |
Can't we keep that Update. Looks like that's the way how you did it in nodejs/node#32891, so ignore this question. |
|
It needs the destroy event too though. It creates a span for the callback by starting and ending at the before and after events, but it also creates a span around around the resource lifetime by starting and ending on the init and destroy events. If there is no destroy then that span will never be ended and I'm not sure what the implications of that would be for trace_events. Since it's supposed to just be an event log, it might be fine to just be missing an event. @jasnell Any insight here? |
|
yep, you're exactly right @Qard, not having visibility on the destroy event would be a disaster for tools like clinic.js that specifically analyze object lifetimes. |
But if |
|
Currently trace events will not be emitted for promises at all unless the PromiseHook is enabled. What I'm thinking is maybe only emitting them if there is a destroy hook. What we could do is have Could also just have two handlers in C++ and have a separate function like |
|
Proposal for reworking promise integration into async_hooks: Regarding the promise story, I think the current strategy is fundamentally wrong. Right now the promise lifecycle is somehow intertwined with edit: clarified that each ``[[then]]` call gets its own unique resource. I think the right approach is to instead rework promise integration into
This avoids tracking promise objects with the garbage collector, directly solving "Avoid wrapping promises with AsyncWrap if destroy is not used" and might make "Make PromiseHook support thenables" more feasible. edit: Solving "Make PromiseHook support thenables" is more feasible because we don't need to know when the object was created or destroyed. Only when it is used by native JS APIs that invokes the microtask queue. That is actually doable! Only manual calls to In terms of still providing insight into a promise lifecycle, a dedicated This does change the default async stack, as a chain of promises would become shorted to just the last promise. I think for most purposes this is actually desired, and a user can recover the full stack by integrating Finally, I want to highlight that right now, tracking the actual async boundary that PS: I know I have haven't been active in node.js for a long time. I still want to participate if you think it can be helpful on very specific issues, but the mail-storm I receive via @ |
What happens if a user-land module calls
This may be too early to emit I didn't check the above considerations in practice, so I may be wrong. |
|
The real problem, in my opinion, is that we are conflating promise gc with handle closes in the destroy hook. Those should really be two different events that can be handled separately, and that's one of the things I hope to work on. |
Resolve happens usually in a differnt context and I don't think this is the point here.
I agree with @AndreasMadsen that usually propagation from the place where But I'm not sure if this can be event implemented with current v8 APIs. |
|
I wouldn't say async function main() {
const first = asyncThing()
const second = asyncThing()
await first
await second
}The point at which the |
No, the point here is different. Notice the "some operation queuing is happening" part. There multiple popular libraries that don't play nicely with So, the question here is whether native |
|
I should point out that is exactly the timing issue that this PR is solving. In async/await, the |
They will not lose context, they will have a different context. If that context is unsatisfying, one should integrate with
The relevant conversation isn't if it is too early to emit You are right that some context is lost. I honestly don't think it matters. Let's say you have: function context() { // context: root
new Promise(fnStart)
.then(fnA) // context: root -> A
.then(fnB) // context: root -> B
.then(fnC) // context: root -> C
}Then yes you are correct you |
I see. You propose to emit I'm also aware that multiple calls to Considering this, do you propose to assign a unique async id to each invocation of
This 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 Don't get me wrong. I'm all for simplifying things, but we need to avoid breaking existing code, at least without a really strong reason. Maybe it makes sense to introduce In this case, it will be possible to introduce an analog of |
Yes. Each
I think we do need to break the existing code. As I see it there are 3 issues with promises:
I don't think any of these issues can be solved given the current
const { AsyncLocalStorage } = require('async_hooks');
const cls = new AsyncLocalStorage();
cls.run({ s: ['root'] }, function () {
Promise.resolve(1)
.then((v) => cls.run({ s: [...cls.getStore().s, 'A'] },
() => console.log(cls.getStore().s)))
.then((v) => cls.run({ s: [...cls.getStore().s, 'B'] },
() => console.log(cls.getStore().s)))
.then((v) => cls.run({ s: [...cls.getStore().s, 'C'] },
() => console.log(cls.getStore().s)))
});prints
No, that would just make a bad situation worse. |
|
I agree that the |
Makes sense. I think that would also be a good approach for hooking into the native Promise APIs to support thenables. |
Yes, you're right. Both chains can be thought as "correct". And the current behavior is
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 |
Yes, this would be a breaking change. So definitely for next major release.
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 |
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
Yes, in any case we should have a separate resource object per
|
This is a problem with In any case, the issue you describe is no different if 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
});
});
}); |
What makes you think it's a mem leak? In fact ALS is more memory safe than
I've just tried running this snippet on node 14.2.0 and got 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. |
I could be wrong. That is anyway a separate discussion.
That is great. If it works with |
Agreed.
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 |
Great. Please don't assume anymore that I intended to call the I have clarified in the initial proposal that a new resource object should be created, or alternatively the returned the Promise by |
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 |
|
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. |
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. 😅
The text was updated successfully, but these errors were encountered: