[Merged by Bors] - Fail to compile on 16-bit platforms #4736
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
james7132
added
A-ECS
tim-blackbird
approved these changes
May 13, 2022
|
#4452 attempts something similar for 32 bit platforms. Do we need to forbid those too? |
|
Can you add a more detailed description of where this is relied upon for soundness to your PR description? I want to be able to refer back to this easily if we ever decide to support it. |
@alice-i-cecile isn’t wasm a 32-bit platform for the time being? |
No, I don't think we strictly rely on any unchecked u64 to usize conversions where unsafe code is involved, though we should definitely audit and verify this isn't the case. However, some of the core data types we use, namely Entity, assume we have access to a usize that is at least as big as u32 and the soundness of the unsafe code surrounding it relies on that assumption.
Done. |
alice-i-cecile
approved these changes
May 13, 2022
alice-i-cecile
added
the
S-Ready-For-Final-Review
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
May 13, 2022
# Objective `bevy_ecs` assumes that `u32 as usize` is a lossless operation and in a few cases relies on this for soundness and correctness. The only platforms that Rust compiles to where this invariant is broken are 16-bit systems. A very clear example of this behavior is in the SparseSetIndex impl for Entity, where it converts a u32 into a usize to act as an index. If usize is 16-bit, the conversion will overflow and provide the caller with the wrong index. This can easily result in previously unforseen aliased mutable borrows (i.e. Query::get_many_mut). ## Solution Explicitly fail compilation on 16-bit platforms instead of introducing UB. Properly supporting 16-bit systems will likely need a workable use case first. --- ## Changelog Removed: Ability to compile `bevy_ecs` on 16-bit platforms. ## Migration Guide `bevy_ecs` will now explicitly fail to compile on 16-bit platforms. If this is required, there is currently no alternative. Please file an issue (https://github.com/bevyengine/bevy/issues) to help detail your use case.
bors
bot
changed the title
robtfm
pushed a commit
to robtfm/bevy
that referenced
this pull request
May 17, 2022
# Objective `bevy_ecs` assumes that `u32 as usize` is a lossless operation and in a few cases relies on this for soundness and correctness. The only platforms that Rust compiles to where this invariant is broken are 16-bit systems. A very clear example of this behavior is in the SparseSetIndex impl for Entity, where it converts a u32 into a usize to act as an index. If usize is 16-bit, the conversion will overflow and provide the caller with the wrong index. This can easily result in previously unforseen aliased mutable borrows (i.e. Query::get_many_mut). ## Solution Explicitly fail compilation on 16-bit platforms instead of introducing UB. Properly supporting 16-bit systems will likely need a workable use case first. --- ## Changelog Removed: Ability to compile `bevy_ecs` on 16-bit platforms. ## Migration Guide `bevy_ecs` will now explicitly fail to compile on 16-bit platforms. If this is required, there is currently no alternative. Please file an issue (https://github.com/bevyengine/bevy/issues) to help detail your use case.
exjam
pushed a commit
to exjam/bevy
that referenced
this pull request
May 22, 2022
# Objective `bevy_ecs` assumes that `u32 as usize` is a lossless operation and in a few cases relies on this for soundness and correctness. The only platforms that Rust compiles to where this invariant is broken are 16-bit systems. A very clear example of this behavior is in the SparseSetIndex impl for Entity, where it converts a u32 into a usize to act as an index. If usize is 16-bit, the conversion will overflow and provide the caller with the wrong index. This can easily result in previously unforseen aliased mutable borrows (i.e. Query::get_many_mut). ## Solution Explicitly fail compilation on 16-bit platforms instead of introducing UB. Properly supporting 16-bit systems will likely need a workable use case first. --- ## Changelog Removed: Ability to compile `bevy_ecs` on 16-bit platforms. ## Migration Guide `bevy_ecs` will now explicitly fail to compile on 16-bit platforms. If this is required, there is currently no alternative. Please file an issue (https://github.com/bevyengine/bevy/issues) to help detail your use case.
bors bot
pushed a commit
that referenced
this pull request
Dec 20, 2022
# Objective This includes one part of #4899. The aim is to improve CPU-side rendering performance by reducing the memory footprint and bandwidth required. ## Solution Shrink `DrawFunctionId` to `u32`. Enforce that `u32 as usize` conversions are always safe by forbidding compilation on 16-bit platforms. This shouldn't be a breaking change since #4736 disabled compilation of `bevy_ecs` on those platforms. Shrinking `DrawFunctionId` shrinks all of the `PhaseItem` types, which is integral to sort and render phase performance. Testing against `many_cubes`, the sort phase improved by 22% (174.21us -> 141.76us per frame).  The main opaque pass also imrproved by 9% (5.49ms -> 5.03ms)  Overall frame time improved by 5% (14.85ms -> 14.09ms)  There will be a followup PR that likewise shrinks `CachedRenderPipelineId` which should yield similar results on top of these improvements.
bors bot
pushed a commit
that referenced
this pull request
Dec 20, 2022
# Objective This includes one part of #4899. The aim is to improve CPU-side rendering performance by reducing the memory footprint and bandwidth required. ## Solution Shrink `DrawFunctionId` to `u32`. Enforce that `u32 as usize` conversions are always safe by forbidding compilation on 16-bit platforms. This shouldn't be a breaking change since #4736 disabled compilation of `bevy_ecs` on those platforms. Shrinking `DrawFunctionId` shrinks all of the `PhaseItem` types, which is integral to sort and render phase performance. Testing against `many_cubes`, the sort phase improved by 22% (174.21us -> 141.76us per frame).  The main opaque pass also imrproved by 9% (5.49ms -> 5.03ms)  Overall frame time improved by 5% (14.85ms -> 14.09ms)  There will be a followup PR that likewise shrinks `CachedRenderPipelineId` which should yield similar results on top of these improvements.
bors bot
pushed a commit
that referenced
this pull request
Dec 20, 2022
# Objective This includes one part of #4899. The aim is to improve CPU-side rendering performance by reducing the memory footprint and bandwidth required. ## Solution Shrink `DrawFunctionId` to `u32`. Enforce that `u32 as usize` conversions are always safe by forbidding compilation on 16-bit platforms. This shouldn't be a breaking change since #4736 disabled compilation of `bevy_ecs` on those platforms. Shrinking `DrawFunctionId` shrinks all of the `PhaseItem` types, which is integral to sort and render phase performance. Testing against `many_cubes`, the sort phase improved by 22% (174.21us -> 141.76us per frame).  The main opaque pass also imrproved by 9% (5.49ms -> 5.03ms)  Overall frame time improved by 5% (14.85ms -> 14.09ms)  There will be a followup PR that likewise shrinks `CachedRenderPipelineId` which should yield similar results on top of these improvements.
bors bot
pushed a commit
that referenced
this pull request
Dec 20, 2022
# Objective This includes one part of #4899. The aim is to improve CPU-side rendering performance by reducing the memory footprint and bandwidth required. ## Solution Shrink `DrawFunctionId` to `u32`. Enforce that `u32 as usize` conversions are always safe by forbidding compilation on 16-bit platforms. This shouldn't be a breaking change since #4736 disabled compilation of `bevy_ecs` on those platforms. Shrinking `DrawFunctionId` shrinks all of the `PhaseItem` types, which is integral to sort and render phase performance. Testing against `many_cubes`, the sort phase improved by 22% (174.21us -> 141.76us per frame).  The main opaque pass also imrproved by 9% (5.49ms -> 5.03ms)  Overall frame time improved by 5% (14.85ms -> 14.09ms)  There will be a followup PR that likewise shrinks `CachedRenderPipelineId` which should yield similar results on top of these improvements.
bors bot
pushed a commit
that referenced
this pull request
Dec 20, 2022
# Objective This includes one part of #4899. The aim is to improve CPU-side rendering performance by reducing the memory footprint and bandwidth required. ## Solution Shrink `DrawFunctionId` to `u32`. Enforce that `u32 as usize` conversions are always safe by forbidding compilation on 16-bit platforms. This shouldn't be a breaking change since #4736 disabled compilation of `bevy_ecs` on those platforms. Shrinking `DrawFunctionId` shrinks all of the `PhaseItem` types, which is integral to sort and render phase performance. Testing against `many_cubes`, the sort phase improved by 22% (174.21us -> 141.76us per frame).  The main opaque pass also imrproved by 9% (5.49ms -> 5.03ms)  Overall frame time improved by 5% (14.85ms -> 14.09ms)  There will be a followup PR that likewise shrinks `CachedRenderPipelineId` which should yield similar results on top of these improvements.
alradish
pushed a commit
to alradish/bevy
that referenced
this pull request
Jan 22, 2023
# Objective This includes one part of bevyengine#4899. The aim is to improve CPU-side rendering performance by reducing the memory footprint and bandwidth required. ## Solution Shrink `DrawFunctionId` to `u32`. Enforce that `u32 as usize` conversions are always safe by forbidding compilation on 16-bit platforms. This shouldn't be a breaking change since bevyengine#4736 disabled compilation of `bevy_ecs` on those platforms. Shrinking `DrawFunctionId` shrinks all of the `PhaseItem` types, which is integral to sort and render phase performance. Testing against `many_cubes`, the sort phase improved by 22% (174.21us -> 141.76us per frame).  The main opaque pass also imrproved by 9% (5.49ms -> 5.03ms)  Overall frame time improved by 5% (14.85ms -> 14.09ms)  There will be a followup PR that likewise shrinks `CachedRenderPipelineId` which should yield similar results on top of these improvements.
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this pull request
Feb 1, 2023
# Objective This includes one part of bevyengine#4899. The aim is to improve CPU-side rendering performance by reducing the memory footprint and bandwidth required. ## Solution Shrink `DrawFunctionId` to `u32`. Enforce that `u32 as usize` conversions are always safe by forbidding compilation on 16-bit platforms. This shouldn't be a breaking change since bevyengine#4736 disabled compilation of `bevy_ecs` on those platforms. Shrinking `DrawFunctionId` shrinks all of the `PhaseItem` types, which is integral to sort and render phase performance. Testing against `many_cubes`, the sort phase improved by 22% (174.21us -> 141.76us per frame).  The main opaque pass also imrproved by 9% (5.49ms -> 5.03ms)  Overall frame time improved by 5% (14.85ms -> 14.09ms)  There will be a followup PR that likewise shrinks `CachedRenderPipelineId` which should yield similar results on top of these improvements.
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this pull request
Feb 1, 2023
# Objective `bevy_ecs` assumes that `u32 as usize` is a lossless operation and in a few cases relies on this for soundness and correctness. The only platforms that Rust compiles to where this invariant is broken are 16-bit systems. A very clear example of this behavior is in the SparseSetIndex impl for Entity, where it converts a u32 into a usize to act as an index. If usize is 16-bit, the conversion will overflow and provide the caller with the wrong index. This can easily result in previously unforseen aliased mutable borrows (i.e. Query::get_many_mut). ## Solution Explicitly fail compilation on 16-bit platforms instead of introducing UB. Properly supporting 16-bit systems will likely need a workable use case first. --- ## Changelog Removed: Ability to compile `bevy_ecs` on 16-bit platforms. ## Migration Guide `bevy_ecs` will now explicitly fail to compile on 16-bit platforms. If this is required, there is currently no alternative. Please file an issue (https://github.com/bevyengine/bevy/issues) to help detail your use case.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
bevy_ecsassumes thatu32 as usizeis a lossless operation and in a few cases relies on this for soundness and correctness. The only platforms that Rust compiles to where this invariant is broken are 16-bit systems.A very clear example of this behavior is in the SparseSetIndex impl for Entity, where it converts a u32 into a usize to act as an index. If usize is 16-bit, the conversion will overflow and provide the caller with the wrong index. This can easily result in previously unforseen aliased mutable borrows (i.e. Query::get_many_mut).
Solution
Explicitly fail compilation on 16-bit platforms instead of introducing UB.
Properly supporting 16-bit systems will likely need a workable use case first.
Changelog
Removed: Ability to compile
bevy_ecson 16-bit platforms.Migration Guide
bevy_ecswill now explicitly fail to compile on 16-bit platforms, due to an inability to write sound unsafe code for these platforms. If this is required, there is currently no alternative. Please file an issue (https://github.com/bevyengine/bevy/issues) to help detail your use case.