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.

Sign up for GitHub

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

Jump to bottom

benchmark: benchmarking impacts of async hooks on promises #31188

Closed
wants to merge 3 commits into from
Closed

benchmark: benchmarking impacts of async hooks on promises #31188

wants to merge 3 commits into from

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Jan 5, 2020 •
edited
Loading

Refs: nodejs/diagnostics#124

FWIW, following is the result of the benchmark:

async_hooks/promises.js asyncHooks="enabled" n=1000000: 143,333.79312043046
async_hooks/promises.js asyncHooks="disabled" n=1000000: 394,449.2637313192
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@legendecas legendecas added the async_hooks Issues and PRs related to the async hooks subsystem. label Jan 5, 2020
@nodejs-github-bot nodejs-github-bot added the benchmark Issues and PRs related to the benchmark subsystem. label Jan 5, 2020
addaleax
addaleax approved these changes Jan 5, 2020
View reviewed changes
benchmark/async_hooks/promises.js Outdated
run(n).then(() => {
bench.end(n);
});
break;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could fall through here after the hook.enable() call, if you like.

lundibundi
lundibundi approved these changes Jan 5, 2020
View reviewed changes
benchmark/async_hooks/promises.js Outdated Show resolved Hide resolved
benchmark/async_hooks/promises.js Outdated
Comment on lines 14 to 16
await new Promise((resolve) => resolve())
.then(() => { throw new Error('foobar'); })
.catch((e) => e);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

    await Promise.resolve()
      .then(() => { throw new Error('foobar'); })
      .catch((e) => e);

Trott
Trott reviewed Jan 6, 2020
View reviewed changes
benchmark/async_hooks/promises.js Outdated

const bench = common.createBenchmark(main, {
n: [1e6],
method: [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: These aren't methods. They're descriptive strings, right? I don't have a better idea than method though, so maybe it's fine and if someone comes up with something better in the future, they can PR that in.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about:

tracking: [
  'enabled',
  'disabled'
]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(If we do make this change, which I like, we'll need to add tracking to test-benchmark-async-hooks.js.)

@Trott
Copy link
Member

Trott commented Jan 6, 2020

Confirmed on the command line that test/benchmark/test-benchmark-async-hooks.js still works.

Trott
Trott reviewed Jan 6, 2020
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional nit or something for someone to do in a subsequent PR: It might be a good idea to have a default value for method (like we do for cipher in benchmark/crypto/aes-gcm-throughput.js) and then specify an empty string for method in test/benchmark/test-benchmark-async-hooks.js. Then subsequent benchmarks that want to use method don't have to use trackingDisabled or whatever if that's not appropriate for that benchmark.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/28234/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/28241/

@Trott
Copy link
Member

Trott commented Jan 7, 2020

Benchmark tests CI: https://ci.nodejs.org/job/node-test-commit-custom-suites-freestyle/11413/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/28270/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/28291/

@Trott
Copy link
Member

Trott commented Jan 8, 2020

Landed in b3b0ae5

@Trott Trott closed this Jan 8, 2020
Trott pushed a commit that referenced this pull request Jan 8, 2020
@legendecas @Trott
benchmark: benchmarking impacts of async hooks on promises
b3b0ae5 
PR-URL: #31188
Refs: nodejs/diagnostics#124
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@legendecas legendecas deleted the benchmark-async-hooks-promises branch January 8, 2020 23:30
MylesBorins pushed a commit that referenced this pull request Jan 16, 2020
@legendecas @MylesBorins
benchmark: benchmarking impacts of async hooks on promises
317d2db 
PR-URL: #31188
Refs: nodejs/diagnostics#124
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@codebytere codebytere mentioned this pull request Jan 16, 2020
Merged
codebytere pushed a commit that referenced this pull request Mar 14, 2020
@legendecas @codebytere
benchmark: benchmarking impacts of async hooks on promises
4f9d57d 
PR-URL: #31188
Refs: nodejs/diagnostics#124
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@codebytere codebytere mentioned this pull request Mar 17, 2020
Merged
@puzpuzpuz
Copy link
Member

Is there an issue for continuation of this work (I mean further optimization)?

I did a simple experiment recently. I have removed emit resolve call from PromiseWrap, so that C++ to JS calls (at least, for resolve hook) don't happen in this benchmark. It did almost no impact on the benchmark result: increase for the async_hooks enabled scenario was around 10-15%. This makes me thinking that C++ to JS calls are less expensive than PromiseWrap itself, at least in this benchmark.

@legendecas
Copy link
Member Author

@puzpuzpuz Yeah, your result is quite similar in my local reproduction. I'm trying to find out improvements. But, alas, if you have any idea you can go ahead 😄.

@puzpuzpuz
Copy link
Member

@puzpuzpuz Yeah, your result is quite similar in my local reproduction. I'm trying to find out improvements. But, alas, if you have any idea you can go ahead .

I was thinking of optimizations for AsyncLocalStorage scenario, i.e. when only init hook is present. Maybe, some corners can be cut in this case, but I don't have anything specific on my mind yet. Going to do some more experiments to gather ideas.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lundibundi lundibundi lundibundi approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott approved these changes

Assignees
No one assigned
Labels
async_hooks Issues and PRs related to the async hooks subsystem. benchmark Issues and PRs related to the benchmark subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@legendecas @Trott @nodejs-github-bot @puzpuzpuz @addaleax @BridgeAR @lundibundi