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

src: skip costly pushing/popping if we pass zeroes as async context #41279

Closed
wants to merge 2 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Dec 22, 2021

Improve MakeCallback performance:

From uws:

This patch reliably boosts performance by 12% of req/sec on Linux:

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Dec 22, 2021
@ronag ronag added the async_hooks Issues and PRs related to the async hooks subsystem. label Dec 22, 2021
@ronag ronag requested a review from addaleax December 22, 2021 14:12
@ronag ronag added request-ci Add this label to start a Jenkins CI on a PR. and removed needs-ci PRs that need a full CI run. labels Dec 22, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 22, 2021
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

This only increases performance for deprecated legacy usage of the MakeCallback API (namely the one in which addons do not implement proper async tracking) – that seems like a bad idea, tbh.

@ronag
Copy link
Member Author

ronag commented Dec 22, 2021

What’s the harm though? I don’t have any use for the async context in my use case and the performance boost is kind of nice.

@addaleax
Copy link
Member

@ronag I mean ... it's more of a general principle that deprecated features a) should not add complexity to the code when avoidable and b) should not look like supported or encouraged use cases.

I don’t have any use for the async context in my use case

That might be true, but it seems like that's specific to your use case? Authors of addons cannot know whether their consumers use async tracking functionality or not (unless they are the same people, i.e. private addons, I guess), and Node.js has made it fairly clear that async tracking is a feature that the platform provides and that addons should support.

@uasan
Copy link

uasan commented Dec 23, 2021

@addaleax, You listed the political arguments.
That mean that everyone has to pay a price for what they don't use, this is a weird policy (

@Trott
Copy link
Member

Trott commented Dec 23, 2021

@addaleax, You listed the political arguments.

She made the argument from the point of view of maintainability.

That mean that everyone has to pay a price for what they don't use, this is a weird policy (

Maybe I'm misunderstanding something but I took her argument to be the opposite: No cost should be incurred by the project maintainers to improve performance of deprecated features. That seems like a reasonable position for someone to take.

@uasan
Copy link

uasan commented Dec 23, 2021

Maybe I'm misunderstanding something but I took her argument to be the opposite: No cost should be incurred by the project maintainers to improve performance of deprecated features. That seems like a reasonable position for someone to take.

No, here's the cost everyone pays, without this patch
img

@addaleax
Copy link
Member

No, here's the cost everyone pays, without this patch

That graph is comparing Node.js versions, and doesn't show what was actually being benchmarked. Unfortunately, that is not meaningful as data here.

And to say "everyone pays, without this patch" is misleadingly worded -- if there is a performance penalty without this patch (and I would assume there is because I assume that @ronag has benchmarked this), it is payed by people who use addons that use Node.js functionality in a deprecated way, not by everyone.

(And to be clear, this is not general opposition to performance improvements regarding MakeCallback.)

@e3dio
Copy link

e3dio commented Dec 26, 2021

Unfortunately, that is not meaningful as data

@trevnorris did detailed benchmark of MakeCallback, does this count? https://gist.github.com/trevnorris/35c8e570c3097f58739b84bfa3f0a390

Which happens to match up exactly with Addon benchmark request per second / echos per second

image

Hopefully the trend has not continued downward, I have not done recent benchmark

@addaleax
Copy link
Member

addaleax commented Dec 26, 2021

does this count

@e3dio To be clear: What’s missing isn’t valid benchmarking data, it’s relevant benchmarking data. Of course it “counts” as valid benchmarking results; but it’s only data that can be taken to say “there is a performance issue”. It’s not data that can be taken to say “this PR meaningfully addresses that issue”.

@addaleax
Copy link
Member

Opened #41331, which also removes the overhead of pushing/popping async context, and does so in a way that actually benefits all users of MakeCallback().

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. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants