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

Change flags for having an atomic u64 #5284

Merged
merged 3 commits into from
Dec 16, 2022
Merged

Change flags for having an atomic u64 #5284

merged 3 commits into from
Dec 16, 2022

Conversation

notgull
Copy link
Contributor

@notgull notgull commented Dec 11, 2022

Motivation

See this comment.

Solution

The aim of this PR is to use the target_has_atomic feature on Rust 1.60, in order to avoid using a less efficient strategy (e.g. mutexes) on platforms that do actually have atomic U64. It changes the cfg_(not_)has_atomic_u64 to incorporate target_has_atomic = "64" based on a build flag.

Copy link
Member

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

LGTM

@taiki-e taiki-e added the A-tokio Area: The main tokio crate label Dec 11, 2022
@Darksonn
Copy link
Contributor

If we're going to have a probe, we might as well replace the entire 64-bit atomic detection logic with a build-time probe that just checks whether the AtomicU64 type exists.

@taiki-e
Copy link
Member

taiki-e commented Dec 11, 2022

If we're going to have a probe, we might as well replace the entire 64-bit atomic detection logic with a build-time probe that just checks whether the AtomicU64 type exists.

Unfortunately, probe is not very robust, and detecting everything that way does not work well in some environments or targets. See also rust-lang/futures-rs#2294.

@Darksonn
Copy link
Contributor

We could have it use target_has_atomic when available, and use an autocfg probe for old compilers.

@Darksonn
Copy link
Contributor

As an additional precaution against autocfg being brittle, we can have the build script emit a warning and enable everything if autocfg fails. Users can explicitly disable things themselves by passing --cfg flags if they're on a weird platform where autocfg doesn't work.

@notgull
Copy link
Contributor Author

notgull commented Dec 11, 2022

I'm fine with that approach, since that particular autocfg probe won't run on newer versions of Rust, so the performance/weirdness penalty won't come up for versions we care about.

@taiki-e
Copy link
Member

taiki-e commented Dec 12, 2022

As an additional precaution against autocfg being brittle, we can have the build script emit a warning and enable everything if autocfg fails. Users can explicitly disable things themselves by passing --cfg flags if they're on a weird platform where autocfg doesn't work.

Well, the warnings from the build script are usually not shown to users, but I'm fine with this approach since only the old rustc users are affected.

tokio/build.rs Outdated Show resolved Hide resolved
tokio/build.rs Outdated
Comment on lines 164 to 170
if !target_has_atomic_u64 {
// To disable this feature on compilers that support it, you can
// explicitly pass this flag with the following environment variable:
//
// RUSTFLAGS="--cfg tokio_no_atomic_u64"
autocfg::emit("tokio_no_atomic_u64")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This variable naming is a bit confusing currently, because target_has_atomic_u64 is always true on the latest compiler even if the target doesn't have atomic u64.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would something like target_needs_atomic_u64_fallback be better?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's certainly an option.

tokio/build.rs Outdated
Comment on lines 164 to 170
if !target_needs_atomic_u64_fallback {
// To disable this feature on compilers that support it, you can
// explicitly pass this flag with the following environment variable:
//
// RUSTFLAGS="--cfg tokio_no_atomic_u64"
autocfg::emit("tokio_no_atomic_u64")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like the boolean is inverted here. Surely we would only disable atomic u64 if we need the fallback?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason, when I do this, it looks like the FreeBSD checks fail?

Copy link
Member

Choose a reason for hiding this comment

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

tokio/build.rs Outdated Show resolved Hide resolved
tokio/build.rs Outdated Show resolved Hide resolved
@Darksonn
Copy link
Contributor

You can push an empty commit to trigger CI again when the incident is resolved.

@Darksonn Darksonn merged commit 42db755 into tokio-rs:master Dec 16, 2022
@notgull notgull deleted the better-atomic-detection branch December 16, 2022 19:06
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants