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

improve performance of domains #50480

Closed
ORESoftware opened this issue Oct 30, 2023 · 15 comments
Closed

improve performance of domains #50480

ORESoftware opened this issue Oct 30, 2023 · 15 comments
Labels
domain Issues and PRs related to the domain subsystem. performance Issues and PRs related to the performance of Node.js.

Comments

@ORESoftware
Copy link
Contributor

ORESoftware commented Oct 30, 2023

What is the problem this feature will solve?

Since domains are now built on-top of async-hooks (here), and since domains serve a purpose that cannot be replaced by other functionality (that wouldn't look exactly like how domains look now) - my continued opinion is that domain should be maintained.

That being said, the implementation as it stands is a bit slow, for example:

In an express server, if we add this middleware:

const Domain = require('domain');

if(process.env.use_domains === 'yes'){

  app.use((req,res,next) => {
    const d = Domain.create();
    d.run(next);
  });

}

this one middleware handler will slow down the server about 20% - 30% when at a heavy load. Even when no further async actions are taken to process the request and respond. I can only imagine that several async calls subsequent to the domain attachment to the function call chain would be even slower.

I assume the expense comes from async-hooks, etc.
Are there any clear avenues to improve performance here?

What is the feature you are proposing to solve the problem?

Improve performance in async-hooks / domains areas

What alternatives have you considered?

No alternatives that I can think of currently

@ORESoftware ORESoftware added the feature request Issues that request new features to be added to Node.js. label Oct 30, 2023
@H4ad
Copy link
Member

H4ad commented Oct 30, 2023

Have you already tried async hooks or AsyncLocalStorage?

I don't know if we should optimize deprecated features but maybe it could be worth a look, cc @nodejs/performance,

AsyncLocalStorage will be specially faster after #48528

@H4ad H4ad added performance Issues and PRs related to the performance of Node.js. domain Issues and PRs related to the domain subsystem. and removed feature request Issues that request new features to be added to Node.js. labels Oct 30, 2023
@H4ad
Copy link
Member

H4ad commented Oct 30, 2023

Also, be aware the domains module can be removed, so I don't think optimizing this module will be viable.

@ORESoftware
Copy link
Contributor Author

@H4ad if someone could explain how AsyncLocalStorage would operate any differently than Domains I would love to hear an explanation - it will operate the same

@bnoordhuis
Copy link
Member

There are no plans to work on the performance of the domains module. Performance improvements are welcome provided they're unobtrusive and not a maintenance burden.

if someone could explain how AsyncLocalStorage would operate any differently than Domains

Domains was an experiment that never worked well for the goals it set out to accomplish; its design is fundamentally flawed. That's the reason it's been deprecated for the last 8 years.

AsyncLocalStorage cuts back on scope. It's still partially experimental but a lot more thought has been put into the design and there's a much better chance it will still be around 5 or 10 years from now.

@ORESoftware
Copy link
Contributor Author

ORESoftware commented Oct 31, 2023

@bnoordhuis I can see you put a lot of work into the PR that would remove domains, so I understand that you're vested here. I am also vested, since I believe domains serve a purpose that can't be replicated with a different implementation. I don't believe domains were implemented incorrectly (maybe only that the first implementation (so were Streams implemented improperly...) implemented improperly. Domains work now, they are just a little bit slow*. If you can assure me me that AsyncLocalStorage can accomplish what Domains can do (without the user writing 100+ lines of code), then I will be happy. But absolutely zero people have assured me of this, so far. 90% of people seem to misunderstand the importance of domains which leaves the 10% of us who use them to good effect, in production, fighting an uphill battle for years.

*promises and async/await are even slower for a node server than using domains, so it's relative. (And promises and async/await certainly use more memory than domains too).

@bnoordhuis
Copy link
Member

I can see you put a lot of work into the PR that would remove domains, so I understand that you're vested here.

Not at all. I've abandoned work after having second thoughts that was magnitudes bigger. I'm vested insofar that I believe domains are a failed experiment.

I'm in no rush to remove them but I also don't plan on keeping them around forever for the handful of direct users.

@ORESoftware
Copy link
Contributor Author

ORESoftware commented Oct 31, 2023

@bnoordhuis in which way are they failed? in which way(s) is the design flawed? I hear the words, I don't see the explanation - domains work - just a perf hit - but like I said no bigger perf hit than using promises or async/await in my estimation. Hence this issue - for us to work on performance of domains using async-hooks or what not. For all I know domains were more performant before async-hooks (circa version 8 and 10) - (I would have to test that theory out).

@bnoordhuis
Copy link
Member

You can look up the discussion from when it was deprecated. This is not the place (and I don't have the time or inclination) for re-litigating said discussion.

@Qard
Copy link
Member

Qard commented Oct 31, 2023

There are so many edge cases in domains which don't work correctly, it would take me all day to list them all. To keep it short, it failed to fully cover the use case of containing (and more importantly safely recovering from) exceptions. It's very common for it to leak open handles and eventually saturate resource availability, and there are numerous ways in which it can escape the sandbox it implies it has because the boundaries are not actually as clear as it seems.

The concept is reasonable, but it likely needs to be proposed as a web standard and implemented at the VM level for it to be able to behave correctly.

@ORESoftware
Copy link
Contributor Author

Have you already tried async hooks or AsyncLocalStorage?

I don't know if we should optimize deprecated features but maybe it could be worth a look, cc @nodejs/performance,

AsyncLocalStorage will be specially faster after #48528

Can you demo the equivalent functionality with async-hooks? Again happy to let domain go since it is currently slow...but just need the same functionality with async-hooks...!

@Qard
Copy link
Member

Qard commented Oct 31, 2023

You can do it in userland with https://github.com/tapjs/async-hook-domain.

@ORESoftware
Copy link
Contributor Author

@Qard thanks, I will test it out and see if it can do what Domains currently can do, in terms of functionality and perf

@ORESoftware
Copy link
Contributor Author

ORESoftware commented Nov 2, 2023

@mcollina
Copy link
Member

mcollina commented Nov 2, 2023

Can we close this?

@bnoordhuis
Copy link
Member

I should say so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain Issues and PRs related to the domain subsystem. performance Issues and PRs related to the performance of Node.js.
Projects
None yet
Development

No branches or pull requests

5 participants