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

Collect data about PromiseHooks performance #144

Closed
gsathya opened this issue Jan 31, 2018 · 11 comments
Closed

Collect data about PromiseHooks performance #144

gsathya opened this issue Jan 31, 2018 · 11 comments
Labels

Comments

@gsathya
Copy link
Member

gsathya commented Jan 31, 2018

As we discussed in the meeting today, I think it would be good to get a write up of the performance characteristics of the different solutions. More concretely, we're interested in performance of
(a) async_hooks enabled and PromiseHooks disabled
(b) monkey patching solution in JS

If (a) is vastly slower, then we have a general problem with async_hooks that need to be solved -- focusing on just promisehooks is not enough.

Alternatively, if both (a) and (b) have comparable perf (or if (a) is faster), then V8 can work on optimizing just PromiseHooks to get a net win and have acceptable performance to make async_hook production ready.

Note: solution (b) doesn't work for async/await, so this would have to be a promise based workload (or just a transpiled version of async/await workload).

cc @mcollina @ofrobots @bmeurer

@bmeurer
Copy link
Member

bmeurer commented Jan 31, 2018

I did actually measure (a), and it was mostly okayish, close to no overhead. But that's changing now that we have a guard on the V8 side that goes off when you enable the debugger or PromiseHooks for the first time.

@bmeurer
Copy link
Member

bmeurer commented Jan 31, 2018

As for data, I started to collect some data in bmeurer/async-hooks-performance-impact.

@littledan
Copy link

Throwing an unsolicited idea out: What if there were a version of the PromiseHooks API which took JS callbacks directly? This might eliminate some of the overhead.

@bmeurer
Copy link
Member

bmeurer commented Feb 2, 2018

@littledan That'd definitely be part of the solution I think. However that'll only reduce the overhead of crossing the boundary. It still leaves us with the problem of how to implement the destroy hook in an efficient way for example.

@Trott
Copy link
Member

Trott commented Feb 8, 2018

Ping @nodejs/async_hooks.

For some reason, I think @mhdawson and/or @jasnell may have some ideas as to who is using this in the real world and what the performance implications have been, although I could be wrong.

@ofrobots
Copy link
Contributor

ofrobots commented Feb 8, 2018

I actually think this issue has already been answered by the comments from @bmeurer above. @gsathya: do you agree?

@gsathya
Copy link
Member Author

gsathya commented Feb 8, 2018

Thanks @Trott!

I actually think this issue has already been answered by the comments from @bmeurer above. @gsathya: do you agree?

No. @bmeurer's benchmarks are very promise heavy and turning off PromiseHooks is almost equivalent to just turning off all async hooks. (... assuming it's the benchmarks mentioned in #144 (comment))

I'm more interested in other non promise heavy (callback based) benchmarks that use timers (and other async hooks) to see the overhead of the other async hooks.

I'm also interested in the cost of this compared to JS based monkey patching solution. If folks have previously been using JS based solutions and have now switched to async_hooks, are they seeing a performance regression?

If someone could analyze their real world code and provide data, that would be great.

@ofrobots
Copy link
Contributor

ofrobots commented Feb 8, 2018

Thanks!

I'm also interested in the cost of this compared to JS based monkey patching solution. If folks have previously been using JS based solutions and have now switched to async_hooks, are they seeing a performance regression?

That's what people have indicated during the recent diagnostics WG meeting, but I admit it is anecdotal. For Google Stackdriver trace itself, we are used to seeing ~5% overhead based on the monkey patching approaches, but that's based on our own test workloads; so not real world.

@danielkhan and @watson 'promised' to go find some real world data.

@hashseed
Copy link
Member

Considering that setting up a weak global handle is very expensive at the init hook, this proposal for weak references might be something worth considering once/if it's implemented and more mature.

@gireeshpunathil
Copy link
Member

should this remain open? [ I am trying to chase dormant issues to closure ]

@github-actions
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants