Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate destroy hook of async_hooks #32386

Closed
legendecas opened this issue Mar 20, 2020 · 24 comments
Closed

Deprecate destroy hook of async_hooks #32386

legendecas opened this issue Mar 20, 2020 · 24 comments
Labels
async_hooks Issues and PRs related to the async hooks subsystem.

Comments

@legendecas
Copy link
Member

With async_hooks.executionResource(#30959 ) and async_hooks.AsyncLocalStorage(#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 legendecas added the async_hooks Issues and PRs related to the async hooks subsystem. label Mar 20, 2020
@legendecas legendecas changed the title Deprecation of destroy hook of async_hooks Deprecate destroy hook of async_hooks Mar 20, 2020
@legendecas
Copy link
Member Author

/cc @nodejs/async_hooks @nodejs/diagnostics

@Flarna
Copy link
Member

Flarna commented Mar 20, 2020

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
Copy link
Member

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
Copy link
Member Author

legendecas commented Mar 20, 2020

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
Copy link
Member

Flarna commented Mar 20, 2020

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
Copy link
Member Author

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

@mcollina
Copy link
Member

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
Copy link
Member

Flarna commented Mar 20, 2020

@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
Copy link

naugtur commented Mar 20, 2020

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
Copy link
Member

jasnell commented Mar 20, 2020

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

Flarna commented Mar 22, 2020

I think the doc linked above (#32386 (comment)) 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
Copy link
Contributor

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

@legendecas
Copy link
Member Author

@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
Copy link
Contributor

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
Copy link
Member Author

@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
Copy link
Member

mcollina commented Apr 3, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem.
Projects
None yet
Development

No branches or pull requests

8 participants