-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Automatically enable portable-atomic
when required
#17570
Conversation
There was a problem hiding this 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.
// 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.
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.
## `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.
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.
[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.
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.
There was a problem hiding this comment.
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
[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.
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.
[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.
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
"
#[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.
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.
Objective
no_std
Bevy #15460Solution
target_has_atomic
configuration variable to automatically detect impartial atomic support and automatically switch toportable-atomic
over the standard library on an as-required basis.Testing
Notes
To explain the technique employed here, consider getting
Arc
either fromalloc::sync
orportable-atomic-util
. First, we can inspect thealloc
crate to see that you only have access toArc
iftarget_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-atomic
feature 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).