Skip to content

Automatically enable portable-atomic when required#17570

Merged
alice-i-cecile merged 3 commits intobevyengine:mainfrom
bushrat011899:AutomaticPortableAtomic
Feb 24, 2025
Merged

Automatically enable portable-atomic when required#17570
alice-i-cecile merged 3 commits intobevyengine:mainfrom
bushrat011899:AutomaticPortableAtomic

Conversation

@bushrat011899
Copy link
Copy Markdown
Contributor

Objective

Solution

  • Used target_has_atomic configuration variable to automatically detect impartial atomic support and automatically switch to portable-atomic over the standard library on an as-required basis.

Testing

  • CI

Notes

To explain the technique employed here, consider getting Arc either from alloc::sync or portable-atomic-util. First, we can inspect the alloc crate to see that you only have access to Arc if target_has_atomic = "ptr". We add a target dependency for this particular configuration inverted:

[target.'cfg(not(target_has_atomic = "ptr"))'.dependencies]
portable-atomic-util = { version = "0.2.4", default-features = false }

This ensures we only have the dependency when it is needed, and it is entirely excluded from the dependency graph when it is not. Next, we adjust our configuration flags to instead of checking for feature = "portable-atomic" to instead check for target_has_atomic = "ptr":

// `alloc` feature flag hidden for brevity

#[cfg(not(target_has_atomic = "ptr"))]
use portable_atomic_util as arc;

#[cfg(target_has_atomic = "ptr")]
use alloc::sync as arc;

pub use arc::{Arc, Weak};

The benefits of this technique are three-fold:

  1. For platforms without full atomic support, the functionality is enabled automatically.
  2. For platforms with atomic support, the dependency is never included, even if a feature was enabled using --all-features (for example)
  3. The portable-atomic feature no longer needs to virally spread to all user-facing crates, it's instead something handled within bevy_platform_support (with some extras where other dependencies also need their features enabled).

@bushrat011899 bushrat011899 added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Utils Utility functions and types X-Uncontroversial This work is generally agreed upon D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward O-Embedded Weird hardware and no_std platforms labels Jan 28, 2025
Copy link
Copy Markdown
Contributor Author

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

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

Some notes to help reviewers.

Comment on lines -220 to -224
// We check despite `portable-atomic` being enabled, if the standard library `Arc` is
// also available, and implement Reflect for it.
#[cfg(all(feature = "portable-atomic", target_has_atomic = "ptr"))]
impl_reflect_opaque!(::alloc::sync::Arc<T: Send + Sync + ?Sized>);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No longer required, since we can now guarantee that bevy_platform_support::sync::Arc will be alloc::sync::Arc if it exists.

Comment on lines -53 to -61
## `portable-atomic` provides additional platform support for atomic types and
## operations, even on targets without native support.
portable-atomic = [
"bevy_app/portable-atomic",
"bevy_ecs/portable-atomic",
"bevy_reflect?/portable-atomic",
"bevy_input_focus/portable-atomic",
]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Since bevy_platform_support can now automatically detect the need for portable-atomic, user-facing crates no longer need to provide that feature.

Comment on lines +125 to +129
[target.'cfg(not(all(target_has_atomic = "8", target_has_atomic = "16", target_has_atomic = "32", target_has_atomic = "64", target_has_atomic = "ptr")))'.dependencies]
concurrent-queue = { version = "2.5.0", default-features = false, features = [
"portable-atomic",
] }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Since bevy_ecs has a dependency which itself has a portable-atomic feature, we replicate the hack here to just automatically enable its feature on required targets.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The trick could be upstreamed at some point

Comment on lines +67 to +68
[target.'cfg(not(target_has_atomic = "ptr"))'.dependencies]
portable-atomic-util = { version = "0.2.4", default-features = false }
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Since we only need portable-atomic-util for Arc, and Arc is only available when target_has_atomic = "ptr", we can simplify its target dependency a little.

Comment on lines +59 to +65
[target.'cfg(not(all(target_has_atomic = "8", target_has_atomic = "16", target_has_atomic = "32", target_has_atomic = "64", target_has_atomic = "ptr")))'.dependencies]
portable-atomic = { version = "1", default-features = false, features = [
"fallback",
] }
spin = { version = "0.9.8", default-features = false, features = [
"portable_atomic",
] }
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Essentially, "If any atomic feature isn't available, pessimistically include portable-atomic"

Comment on lines -77 to +90
#[cfg(feature = "portable-atomic")]
let arc = {
let boxed = Box::new(f);
let boxed: Box<dyn Fn() + Send + Sync + 'static> = boxed;
Arc::from(boxed)
};

#[cfg(not(feature = "portable-atomic"))]
let arc = Arc::new(f);

#[cfg(not(target_has_atomic = "ptr"))]
#[expect(
unsafe_code,
reason = "unsized coercion is an unstable feature for non-std types"
)]
// SAFETY:
// - Coercion from `impl Fn` to `dyn Fn` is valid
// - `Arc::from_raw` receives a valid pointer from a previous call to `Arc::into_raw`
let arc = unsafe {
Arc::from_raw(Arc::into_raw(arc) as *const (dyn Fn() + Send + Sync + 'static))
};

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Just a little tidy-up while I'm here. I'm confident the pointer-cast is valid and it avoids a second allocation.

@alice-i-cecile alice-i-cecile added this to the 0.16 milestone Jan 28, 2025
@bushrat011899 bushrat011899 added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Feb 22, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 24, 2025
Merged via the queue into bevyengine:main with commit 76e9bf9 Feb 24, 2025
33 checks passed
salixsalicaceae pushed a commit to fslabs/bevy that referenced this pull request Mar 26, 2026
# Objective

- Contributes to bevyengine#15460
- Reduce quantity and complexity of feature gates across Bevy

## Solution

- Used `target_has_atomic` configuration variable to automatically
detect impartial atomic support and automatically switch to
`portable-atomic` over the standard library on an as-required basis.

## Testing

- CI

## Notes

To explain the technique employed here, consider getting `Arc` either
from `alloc::sync` _or_ `portable-atomic-util`. First, we can inspect
the `alloc` crate to see that you only have access to `Arc` _if_
`target_has_atomic = "ptr"`. We add a target dependency for this
particular configuration _inverted_:

```toml
[target.'cfg(not(target_has_atomic = "ptr"))'.dependencies]
portable-atomic-util = { version = "0.2.4", default-features = false }
```

This ensures we only have the dependency when it is needed, and it is
entirely excluded from the dependency graph when it is not. Next, we
adjust our configuration flags to instead of checking for `feature =
"portable-atomic"` to instead check for `target_has_atomic = "ptr"`:

```rust
// `alloc` feature flag hidden for brevity

#[cfg(not(target_has_atomic = "ptr"))]
use portable_atomic_util as arc;

#[cfg(target_has_atomic = "ptr")]
use alloc::sync as arc;

pub use arc::{Arc, Weak};
```

The benefits of this technique are three-fold:

1. For platforms without full atomic support, the functionality is
enabled automatically.
2. For platforms with atomic support, the dependency is never included,
even if a feature was enabled using `--all-features` (for example)
3. The `portable-atomic` feature no longer needs to virally spread to
all user-facing crates, it's instead something handled within
`bevy_platform_support` (with some extras where other dependencies also
need their features enabled).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Utils Utility functions and types C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples O-Embedded Weird hardware and no_std platforms S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Uncontroversial This work is generally agreed upon

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants