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

Proposal: use env to specify the default worker thread count for multi-thread runtime #4249

Closed
PureWhiteWu opened this issue Nov 21, 2021 · 26 comments · Fixed by #4250
Closed
Labels
A-tokio Area: The main tokio crate C-feature-request Category: A feature request. M-runtime Module: tokio/runtime

Comments

@PureWhiteWu
Copy link
Sponsor Contributor

PureWhiteWu commented Nov 21, 2021

Is your feature request related to a problem? Please describe.
Sometimes our server binary can run in any environment and may have different custom resource limitations(e.g., CPU).
So it's impossible to specify the worker thread statically in our code, and the default worker thread read by num_cpus doesn't meet our custom resource limitations.
This problem is particularly outstanding in a hybrid deployment environment.

Describe the solution you'd like
I'd propose to add an environment variable TOKIO_WORKER_THREADS to specify the default values for the multi-thread runtime builder. Just like the GOMAXPROCS in Go.
If the environment variable exists, then the value will be set when calling Builder::new so that it can be overridden by code, which doesn't affect the behavior now.

Describe alternatives you've considered
None yet.

Additional context
We are encountering problems in our production environment because we have custom resource limitation approaches.
This feature really helps in our scenery.

I've also implemented this in #4250.

@PureWhiteWu PureWhiteWu added A-tokio Area: The main tokio crate C-feature-request Category: A feature request. labels Nov 21, 2021
@Darksonn Darksonn added the M-runtime Module: tokio/runtime label Nov 22, 2021
@Darksonn
Copy link
Contributor

You can already to this yourself with the Builder::worker_threads method. Why isn't that enough?

@PureWhiteWu
Copy link
Sponsor Contributor Author

You can already to this yourself with the Builder::worker_threads method. Why isn't that enough?

Firstly, to specify worker_threads to a const value in the code is a compile-time behaviour.
Secondly, if you mean to read the environment in our code at runtime and set it manually, then there's no way to use #[tokio::main] directly, and our business side will all need to create the tokio runtime and write this logic by themselves, which we are not able to control.
We are the infra team of ByteDance, and I think provide a environment variable to specify the worker threads is a common use case not only needed by us. For example, Go also provides GOMAXPROCS to do the same thing.

@Darksonn
Copy link
Contributor

Yes, it's true that you can't use #[tokio::main] when doing this.

@PureWhiteWu
Copy link
Sponsor Contributor Author

Yes, it's true that you can't use #[tokio::main] when doing this.

So I think it's better to provide a mechanism to specify the worker threads at runtime, without the need to ask everyone to change their code.
What do you think?

@Noah-Kennedy
Copy link
Contributor

I think that this is a good feature to have, especially since configuring things like this is easily done with environment variables when under systems like K8s.

@Darksonn
Copy link
Contributor

Alright, let's go for it. Let's bikeshed the name of the env variable. The current suggested name is TOKIO_WORKER_THREAD, but why not add an S at the end?

@PureWhiteWu
Copy link
Sponsor Contributor Author

Alright, let's go for it. Let's bikeshed the name of the env variable. The current suggested name is TOKIO_WORKER_THREAD, but why not add an S at the end?

Great, It's my fault. Let's change it to TOKIO_WORKER_THREADS.

@PureWhiteWu
Copy link
Sponsor Contributor Author

Alright, let's go for it. Let's bikeshed the name of the env variable. The current suggested name is TOKIO_WORKER_THREAD, but why not add an S at the end?

I've changed all the names to TOKIO_WORKER_THREADS.
Shall we move forward to the PR?

@Noah-Kennedy Noah-Kennedy linked a pull request Nov 25, 2021 that will close this issue
@SeanEClarke
Copy link

Is there a reason why this has been progressed or merged?

@Noah-Kennedy
Copy link
Contributor

It wasn't really clear what the path forward was for this, and discussion kinda died out.

@SeanEClarke
Copy link

That's a shame, as the situation won't resolve itself and with the growing core count it is getting worse, we have some new servers that pack 256 cores/HTs - using the standard runtime/worker thread model we spin up 256 threads in the pool, run 4 services and you are up to 1K threads.

The discussed point is correct, you can not use the default and create your own, however you potentially then have to pass your custom runtime through your app. I can't see the issue of taking the default config from an ENV variable - if its not present you get the default, if its there you use it.

@PureWhiteWu
Copy link
Sponsor Contributor Author

We've encountered similar problems, the only possible way for us is to specify the thread count by using env variable.
We are not able to change the code since the code might be developed by other teams, and we even don't have the permission to read their code.
This is really a waste of resource and can cause great performance regression.

Shall we take another look at this proposal/pr to determine if we can move forward? cc @Noah-Kennedy @Darksonn

@Noah-Kennedy
Copy link
Contributor

Personally, I have zero issues with this so long as it only impacts #[tokio::main].

@Darksonn
Copy link
Contributor

Only #[tokio::main]?

@Noah-Kennedy
Copy link
Contributor

My thoughts here is that #[tokio::main] is already kinda "magic", and that users of the builder are trying to directly control every aspect of the runtime construction anyways.

@notgull
Copy link
Contributor

notgull commented Oct 30, 2022

Bikeshed: Should there also be a TOKIO_THREADS variable that constrains both worker threads and blocking threads?

@SeanEClarke
Copy link

I still think lacking the ability to change behaviour in an environment buy using an environmental input is short sighted/limiting - however I can see you can at least alleviate some of my initial concerns (i.e. spinning up a process with a thread er core on servers with large core counts) bu using parameters passed to the tokio::main macro:

#[tokio::main(flavor = "multi_thread", worker_threads = 6)]

@SeanEClarke
Copy link

Only #[tokio::main]?

I think so, as has already been stated those using the Builder are likely wanting a finer level of control, using the magic #[tokio::main] together with some form of restriction seems like a halfway house / compromise.

@PureWhiteWu
Copy link
Sponsor Contributor Author

Just paste my comment in the pr here:

Only #[tokio::main]?

I have thought about this before, but I don't think that make sense, because users may set some other builder configs or create the runtime manually and use it (we have a lot of real production use cases for that). For example, when writing dylib in async rust and call it from c++/go using ffi, we need to create the runtime manually using something like lazy_static.

And if this env is set, the operator's intention must be that all runtimes need to use this, unless the developer has explicitly set the worker threads.

So I think set it in builder is better than in the #[tokio::main] macro.

@Noah-Kennedy
Copy link
Contributor

Think about this more, I do take back what I said earlier about the macro. It would make more sense to have this be the behavior for all runtimes.

@PureWhiteWu
Copy link
Sponsor Contributor Author

Hi, can we move forward on this?

@PureWhiteWu
Copy link
Sponsor Contributor Author

Kindly ping if you missed this~

@Darksonn
Copy link
Contributor

Darksonn commented Dec 1, 2022

I asked about this on discord again after your previous ping, and nobody objected, so I think its fine to move forward.

@carllerche
Copy link
Member

I think it is OK to have an env var set runtime configuration settings if nothing else sets these values explicitly. We can move forward w/ this.

@Noah-Kennedy
Copy link
Contributor

I'm in agreement.

@PureWhiteWu
Copy link
Sponsor Contributor Author

Great, thank you very much!
Let's move to the code part, do you have any more comments about the implementation? #4250

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-feature-request Category: A feature request. M-runtime Module: tokio/runtime
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants