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

Automatically enable portable-atomic when required #17570

Merged
merged 3 commits into from
Feb 24, 2025

Conversation

bushrat011899
Copy link
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
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
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
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
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
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
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
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
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
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