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

feat(rt): read default worker thread from env #4250

Merged
merged 17 commits into from
Dec 21, 2022

Conversation

PureWhiteWu
Copy link
Contributor

Implements #4249

@PureWhiteWu
Copy link
Contributor Author

Kindly ping if you missed this~ @Darksonn @Noah-Kennedy

@Darksonn
Copy link
Contributor

Darksonn commented Dec 7, 2021

I'm in the middle of exam week, so I'm busy with other stuff this week.

@carllerche
Copy link
Member

Thanks for submitting the PR. The code seems fine. IIRC, when this was discussed, there was a question about whether or not an env variable should change the default globally. I.e. should an env variable impact a runtime created deep within a library (for example).

I think what we could do instead is to limit the env variable to #[tokio::main]. So, it would only change the number of threads created for the "main" runtime in a bin.

This change should be discussed by @tokio-rs/maintainers though before we decide to move forward.

@jonhoo
Copy link
Contributor

jonhoo commented Dec 10, 2021

My instinct here is the same as @Darksonn's initial take in #4249 (comment), which is that this should be up to each application to do by using the builder construction. I don't know that the addition of a magic env var actually buys you that much compared to the slight increase in complexity for callers when they do need this. If anything, the requirement to be explicit about the size of the thread pool being determined by an environment variable seems valuable. It also makes it easier to do things like pick up variables by other names that may already be set in the environment for this purpose.

If we do decide to adopt this, I'm torn as to whether it should affect all executors or just #[tokio::main]. On the one hand, this is clearly intended for when you don't want to use num_cpus, and why wouldn't that also apply to executors you did not construct directly yourself? But on the other hand, it changes the behavior of other executors in a way that's hard to opt out of if you do want them to continue to use num_cpus. I think I lean towards "have it apply globally if set", but this ambiguity is to me another sign that we shouldn't adopt this kind of switch.

@davidpdrsn
Copy link
Member

I agree with @jonhoo and @Darksonn. Not sure this needs to be built in 🤔 I don't feel it gives you much outside of using the builder directly.

@PureWhiteWu
Copy link
Contributor Author

Firstly, I'd propose this is because in some cases, people who write code isn't people who really run and operate the application, and the author also doesn't know which environment the application will run in. And the environment sometimes have custom limitations that the num_cpus can't figure out.
For example, in our production environment, the application runs in a 192-core physical machine, but we only want it to use 4 cores, and we apply this limitation in a custom way instead of cgroup so num_cpus can't figure out(there are some complex reasons that I just skip here). In this situation, tokio creates 192 threads and we have no way to fix this in runtime(which caused great performance decrease).

Second, I think this change should be "global", all the libraries which create tokio multi-thread runtime should apply this change. Because if a user only creates a multi-thread runtime without specify the worker threads using the builder, then I think we can assume this should be self-adaptive in runtime, and the user gives us(tokio) the right to do this.
Now we only get the values from num_cpus which auto-detects the value by parsing cgroup configs. But this is limited, and sometimes may not work. I think we can also give the rights to the operator and the environment to specify this value manually. This is much more accurate and should be trusted.

Finally, sorry for my poor English😭.

@Ralith
Copy link
Contributor

Ralith commented Dec 11, 2021

we have no way to fix this

Why can't you ask the people responsible for developing the software to read the desired number of cores from an environment variable?

@PureWhiteWu
Copy link
Contributor Author

we have no way to fix this

Why can't you ask the people responsible for developing the software to read the desired number of cores from an environment variable?

First, in our situation, there's thousands of developers in the company, and there's thousands of server-side applications, we are not able to ask everyone to write such logic in their code. It's also impossible to ask them to use a internal-version of tokio(we will never do things like fork an internal version of tokio). And this also can't solve the problem that some dependency may use the "official" tokio.

Second, we think this is quite a common logic, because the run-time environment (and the operator) should have ability to specify the thread count, that's what called "cloud native": you write the code, and you don't care (and won't know) what environment it runs in. To achieve this, we need to expose this ability. And I think that's why Go supports using "GOMAXPROCS" to specify the P nums.

@tobz
Copy link
Member

tobz commented Dec 11, 2021

As is, I don't think this PR should be merged.

It controls a single variable of the runtime, and adds extra code that may or may not end up surprising users when the runtime doesn't behave as they expect simply because an environment variable existed when an application was started.

However, I do think there could be a better pattern for allowing runtime configuration to be overridden via environment variables.

While this is a personal feeling, I'd be interested in seeing a helper type -- perhaps in tokio-util -- that allowed pulling in various runtime configurations via environment variable, perhaps with a configurable prefix. Practically speaking, this type would have to be constructed directly but would read the most common runtime configuration flags/values as environment variables, and could be passed a runtime::Builder, where it would then apply the configuration it loaded and provide a method to actually build the Runtime.

While you raise a fair point that it might be difficult to get hundreds or thousands of developers to use a common pattern, such a helper type as I describe could build a runtime from environment variables in only 2-3 lines of code. That's not many more lines than the actual Cargo.toml imports of Tokio itself.

Benefits of this approach:

  • tokio needs no changes (better for long-term support, avoids code bloat)
  • tokio-util is still an official library, so the helper type will be held up to the standards of the Tokio project overall
  • able to configure more than just one aspect of the runtime
  • has to be called directly, so there's no surprise behavior for users

@PureWhiteWu
Copy link
Contributor Author

Firstly, I think we need to be clear about what the user expects when he specifies to use multi-thread runtime. Has he given tokio the right to decide on the number of threads? I think the answer is "yes".
So the next question is: "what's the source of truth?" Maybe "num_cpus" can provide a default value, but it also has limitations, it can only detect the settings of cgroup, it doesn't know anything else. So I think we can also expose this ability to ask the runtime to specify this value, this is reasonable and useful.

Secondly, I have also considered your approach, but this still does not solve the problem. Almost all users use #[tokio::main] directly on their main func to use tokio, If we only provide something in tokio-util, then we also can't ask everyone to change their code to manually build the runtime by using something from tokio-util.

@Darksonn
Copy link
Contributor

Eh, I agree that putting the feature to do this in tokio-util isn't really a solution. People aren't going to be using that.

@PureWhiteWu
Copy link
Contributor Author

Hello, do you have any more concerns about this PR?
Thanks.

@Darksonn
Copy link
Contributor

Well, the current status is that there is no consensus about the shape we want this feature in. There isn't anything wrong with the implementation per se.

@github-actions github-actions bot added the R-loom Run loom tests on this PR label Oct 24, 2022
@Noah-Kennedy
Copy link
Contributor

I think that a better way to do this would be to only use the variable with #[tokio::main], not within the builder.

@PureWhiteWu
Copy link
Contributor Author

I think that a better way to do this would be to only use the variable with #[tokio::main], not within the builder.

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.

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Sorry about taking so long — I forgot about this. The implementation looks more or less fine. I have a few comments.

tokio/src/runtime/builder.rs Outdated Show resolved Hide resolved
tokio/src/runtime/builder.rs Outdated Show resolved Hide resolved
tokio/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor Author

@PureWhiteWu PureWhiteWu left a comment

Choose a reason for hiding this comment

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

Thanks for your suggestions!
Please take a look again!

tokio/src/runtime/builder.rs Outdated Show resolved Hide resolved
@PureWhiteWu
Copy link
Contributor Author

The ci fail seems has nothing to do with this pr, do you know why it fails?

@Darksonn
Copy link
Contributor

It's an existing test that is flaky. It should be fixed, but in this PR I will simply restart the CI for you if it fails.

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@Darksonn Darksonn merged commit 682e93d into tokio-rs:master Dec 21, 2022
@PureWhiteWu PureWhiteWu deleted the feat/env_worker_thread branch December 21, 2022 14:44
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Jan 8, 2023
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dependencies | minor | `1.23.0` -> `1.24.1` |
| [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dev-dependencies | minor | `1.23.0` -> `1.24.1` |

---

### Release Notes

<details>
<summary>tokio-rs/tokio</summary>

### [`v1.24.1`](https://github.com/tokio-rs/tokio/releases/tag/tokio-1.24.1): Tokio v1.24.1

[Compare Source](tokio-rs/tokio@tokio-1.24.0...tokio-1.24.1)

This release fixes a compilation failure on targets without `AtomicU64` when using rustc older than 1.63. ([#&#8203;5356])

[#&#8203;5356]: tokio-rs/tokio#5356

### [`v1.24.0`](https://github.com/tokio-rs/tokio/releases/tag/tokio-1.24.0): Tokio v1.24.0

[Compare Source](tokio-rs/tokio@tokio-1.23.1...tokio-1.24.0)

The highlight of this release is the reduction of lock contention for all I/O operations ([#&#8203;5300](tokio-rs/tokio#5300)). We have received reports of up to a 20% improvement in CPU utilization and increased throughput for real-world I/O heavy applications.

##### Fixed

-   rt: improve native `AtomicU64` support detection ([#&#8203;5284])

##### Added

-   rt: add configuration option for max number of I/O events polled from the OS
    per tick ([#&#8203;5186])
-   rt: add an environment variable for configuring the default number of worker
    threads per runtime instance ([#&#8203;4250])

##### Changed

-   sync: reduce MPSC channel stack usage ([#&#8203;5294])
-   io: reduce lock contention in I/O operations  ([#&#8203;5300])
-   fs: speed up `read_dir()` by chunking operations ([#&#8203;5309])
-   rt: use internal `ThreadId` implementation ([#&#8203;5329])
-   test: don't auto-advance time when a `spawn_blocking` task is running ([#&#8203;5115])

[#&#8203;5186]: tokio-rs/tokio#5186

[#&#8203;5294]: tokio-rs/tokio#5294

[#&#8203;5284]: tokio-rs/tokio#5284

[#&#8203;4250]: tokio-rs/tokio#4250

[#&#8203;5300]: tokio-rs/tokio#5300

[#&#8203;5329]: tokio-rs/tokio#5329

[#&#8203;5115]: tokio-rs/tokio#5115

[#&#8203;5309]: tokio-rs/tokio#5309

### [`v1.23.1`](https://github.com/tokio-rs/tokio/releases/tag/tokio-1.23.1): Tokio v1.23.1

[Compare Source](tokio-rs/tokio@tokio-1.23.0...tokio-1.23.1)

This release forward ports changes from 1.18.4.

##### Fixed

-   net: fix Windows named pipe server builder to maintain option when toggling
    pipe mode ([#&#8203;5336]).

[#&#8203;5336]: tokio-rs/tokio#5336

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about these updates again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC44NC4xIiwidXBkYXRlZEluVmVyIjoiMzQuODQuMSJ9-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1703
Reviewed-by: crapStone <crapstone@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
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 M-runtime Module: tokio/runtime R-loom Run loom tests on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proposal: use env to specify the default worker thread count for multi-thread runtime
8 participants