search

nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
107.27k stars 29.44k forks source link

Deprecate destroy hook of async_hooks #32386

Closed legendecas closed 4 years ago

legendecas commented 4 years ago

With async_hooks.executionResource(https://github.com/nodejs/node/pull/30959 ) and async_hooks.AsyncLocalStorage(https://github.com/nodejs/node/pull/26540) landed on master, APM and continuation local storage users can use only the init hook and async_hooks.AsyncLocalStorage, which is also only used the init hook.

In the context of that removing destroy hook can introduce a considerable performance improvement possibilities on async_hooks enabled, is it appropriate to deprecate the destroy hook now?

legendecas commented 4 years ago

/cc @nodejs/async_hooks @nodejs/diagnostics

Flarna commented 4 years ago

Please note that there are other users then CLS/APMs for async_hooks which would like to track lifetime of resources. These user cases still need the destroy hook therefore I'm not sure if deprecation is the right approach.

It's also not yet proven that async_hooks.executionResource is applicable to all CLS usecases. For example node core is not yet using it for domains. I know that domains are deprecated but I still see them as sample for an advanced use of async hooks.

addaleax commented 4 years ago

Also, keep in mind that the destroy hook should not be adding a lot of overhead anyway when there are no destroy hooks currently enabled.

legendecas commented 4 years ago

Also, keep in mind that the destroy hook should not be adding a lot of overhead anyway when there are no destroy hooks currently enabled.

Yeah, the overhead is not introduced by destroy hook directly. But as stated in Promise Hooks Proposal, removing destroy hooks and make the lifetime tracking optional by WeakRefs can reduce the overhead of promise hooks.

Any idea?

Flarna commented 4 years ago

Maybe ask v8 team to add an option to SetPromseHook API which types it should report or add a dedicated API to register a destroy hook? As long as there is no user of async_hooks installing a destroy hook it's not installed/enabled.

legendecas commented 4 years ago

@Flarna IIRC, Promise hook doesn't provide a destroy callback. It's wrapped in node with weak global reference by PromiseWrap.

mcollina commented 4 years ago

I'm -1 in removing the destroy hook because it's highly useful for tracking actual disposal of underlining resources (sockets, files, etc..).

From my point of view we should avoid tracking the lifetime of promises if it is not needed.

Flarna commented 4 years ago

@legendecas You are right. That's all within node. As promises are resources is needed for async_hooks.executionResource to track them; at least the creation otherwise any CLS system will fails to work once await or a Promise is used.

i don't know which part of the current design is actually causing the overhead we could get rid of.

naugtur commented 4 years ago

As a user of async hooks, eg. here https://github.com/naugtur/blocked-at/ I'm interested in keeping the destroy available.

I could use async local storage, but I loose control of its size. If I want to create a limited cache where it keeps only some of the items according to custom logic, I need the destroy hook to know which are no longer needed.

I haven't implemented such a cache in the linked project yet, but I did consider it. Without destroy hook I can only save all and get them cleaned up, but can't choose to drop some earlier to save on space

That's just one example. Hope it helps.

jasnell commented 4 years ago

Please don't :-) ... the destroy hook is important in diagnostic tooling that tracks the lifetime of objects for analysis purposes. Yes, it introduces a performance hit but that's ok. What we should do instead is document that it is not needed in most cases but we still do need the hook

legendecas commented 4 years ago

Thanks all for the very detailed explanation!! Seems like this is not the best direction to address the performance issue of async hooks. Given the suggestions above, I'm going to close this and seeking a better solution.

puzpuzpuz commented 4 years ago

Does it make sense to create another issue for not emitting destroy events for async resources (including promises) when there are no active destroy hooks?

I don't know how expensive AsyncWrap::WeakCallback is (I may be wrong when pointing to this method) and whether this or something else in PromiseWrap acts as a bottleneck in AsyncLocalStorage use case, so any feedback from async_hooks maintainers is welcome.

addaleax commented 4 years ago

Does it make sense to create another issue for not emitting destroy events for async resources (including promises) when there are no active destroy hooks?

I’m not sure I understand – this is what’s already happening, for all hooks, right? They are not emitted, and there is no call from C++ to JS, when there are no listeners.

puzpuzpuz commented 4 years ago

I’m not sure I understand – this is what’s already happening, for all hooks, right? They are not emitted, and there is no call from C++ to JS, when there are no listeners.

Hmm, then my understanding seems to be wrong. Is RegisterDestroyHook and its callers the right place to look in to understand how it's implemented?

Do you also have any hints on why Promises usage increase the overhead of AsyncLocalStorage (and, thus, init async hook) when compared with plain callbacks?

addaleax commented 4 years ago

I’m not sure I understand – this is what’s already happening, for all hooks, right? They are not emitted, and there is no call from C++ to JS, when there are no listeners.

Hmm, then my understanding seems to be wrong. Is RegisterDestroyHook and its callers the right place to look in to understand how it's implemented?

Depends on what you mean by “it” – RegisterDestroyHook is what we use to track AsyncResource objects from the public async_hooks API. It’s not related to Promises in any way. AsyncWrap::EmitDestroy() is what we use for registering destroy hooks to be called from C++.

Do you also have any hints on why Promises usage increase the overhead of AsyncLocalStorage (and, thus, init async hook) when compared with plain callbacks?

It’s been a while since I looked into this, but I’m assuming that a big reason is that it comes with a lot of calls from C++ to JS for the other hooks (not destroy), which do have significant overhead.

puzpuzpuz commented 4 years ago

Thanks for the clarification, @addaleax

If the overhead is in C++ to JS calls, it's not that simple to get rid of it.

addaleax commented 4 years ago

Yeah – IIRC, one idea brought up by the V8 team a year ago at the diagnostics summit was providing PromiseHooks in a form that allowed directly registering JS functions with V8, which should reduce that overhead a lot in theory.

puzpuzpuz commented 4 years ago

Interesting. Thanks for sharing this context!

I guess that as a user-land overhead mitigation a Promise library, like bluebird, could be used instead of native promises. But most of such libraries aren't integrated with AsyncResource.

Flarna commented 4 years ago

I think the doc linked above (https://github.com/nodejs/node/issues/32386#issuecomment-601615344) describes quite well that it is not a single place where overhead is caused.

As far a I remember the main issue with destroy hook for promises is the high amount of Promises created by aplications using await. One await results in at least 3 promises. V8 team did a lot opimizations in promises. They are small and as a result and get garbage collected quite lazy. Via promise hook node attaches an object to each promise which makes them heavier and puts more work towards GC. This single hook is not that expenisve but as it happen that frequently it sums up. Async hooks trirggered by other async operations are linked to I/O operations which happen by far not that frequent as promises in an await heavy application.

@puzpuzpuz As far as I know await and util.promisify use always native v8 promises. And for most usecases they should be manwhile faster then userland promises.

mmarchini commented 4 years ago

Is it worth trying to move the proposal linked above (https://github.com/nodejs/node/issues/32386#issuecomment-601615344) forward?

legendecas commented 4 years ago

@mmarchini Is it worth trying to move the proposal linked above (#32386 (comment)) forward?

Definitely! I'm trying to investigate addressing the burden of switching contexts between JS & C++, and considering reviving zones proposal (may not fully identical to the current one, since it's quite similar to domains in some way) to TC39.

mmarchini commented 4 years ago

and considering reviving zones proposal (may not fully identical to the current one, since it's quite similar to domains in some way) to TC39

AFAIK realms and related proposals address all use cases for zones (but I might be wrong). You might want to take a look at those:

legendecas commented 4 years ago

@mmarchini Sorry, I don't get it :(. These proposals are great in their motivations and can help in sandboxing JavaScript execution environments. But seems like none of these are resolving the issue that exposing async context control/manipulation in pure JavaScript. Please correct me if I don't get it right. :)

mcollina commented 4 years ago

I would recommend to open another issue/discussion on zones.