Automatically enable portable-atomic when required#17570
Automatically enable portable-atomic when required#17570alice-i-cecile merged 3 commits intobevyengine:mainfrom
portable-atomic when required#17570Conversation
bushrat011899
left a comment
There was a problem hiding this comment.
Some notes to help reviewers.
| // 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>); | ||
|
|
There was a problem hiding this comment.
No longer required, since we can now guarantee that bevy_platform_support::sync::Arc will be alloc::sync::Arc if it exists.
| ## `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", | ||
| ] | ||
|
|
There was a problem hiding this comment.
Since bevy_platform_support can now automatically detect the need for portable-atomic, user-facing crates no longer need to provide that feature.
| [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", | ||
| ] } | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The trick could be upstreamed at some point
| [target.'cfg(not(target_has_atomic = "ptr"))'.dependencies] | ||
| portable-atomic-util = { version = "0.2.4", default-features = false } |
There was a problem hiding this comment.
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.
| [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", | ||
| ] } |
There was a problem hiding this comment.
Essentially, "If any atomic feature isn't available, pessimistically include portable-atomic"
| #[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)) | ||
| }; | ||
|
|
There was a problem hiding this comment.
Just a little tidy-up while I'm here. I'm confident the pointer-cast is valid and it avoids a second allocation.
# 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).
Objective
no_stdBevy #15460Solution
target_has_atomicconfiguration variable to automatically detect impartial atomic support and automatically switch toportable-atomicover the standard library on an as-required basis.Testing
Notes
To explain the technique employed here, consider getting
Arceither fromalloc::syncorportable-atomic-util. First, we can inspect thealloccrate to see that you only have access toArciftarget_has_atomic = "ptr". We add a target dependency for this particular configuration inverted: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 fortarget_has_atomic = "ptr":The benefits of this technique are three-fold:
--all-features(for example)portable-atomicfeature no longer needs to virally spread to all user-facing crates, it's instead something handled withinbevy_platform_support(with some extras where other dependencies also need their features enabled).