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.

Sign up for GitHub

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

Jump to bottom

Stack overflow despawning entity with handle component to "large" asset #1892

Closed
TehPers opened this issue Apr 12, 2021 · 5 comments
Closed

Stack overflow despawning entity with handle component to "large" asset #1892

TehPers opened this issue Apr 12, 2021 · 5 comments
Labels
A-ECS Entities, components, systems, and events A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior

Comments

@TehPers
Copy link
Contributor

TehPers commented Apr 12, 2021 •
edited
Loading

Bevy version

0.5.0

Operating system & version

Windows 10

What you did

I've included a small project to reproduce the issue.

  1. Create a relatively large struct (in this case, 32KB) and derive TypeUuid for it. This will be your asset.
  2. For the asset type, do .add_asset::<T>() to register it as an asset.
  3. Spawn an entity, and attach a Handle<T> component to that entity. The Handle<T> should come from adding an instance of the asset to a ResMut<Assets<T>>. Check repro for example.
  4. Despawn the entity. It should stack overflow, even for one entity.

I don't know if the size needed to stack overflow is system-dependent. For my system, 32KB was more than enough (win10, stable-x86_64-pc-windows-msvc, 16GB RAM).

What you expected to happen

No stack overflow. I expected the asset to be freed once I despawned the entity because it was the only entity with a strong handle to that asset as a component.

What actually happened

It stack overflowed after the entity was despawned.

Additional information

so_repro

  • Toolchain: stable-x86_64-pc-windows-msvc
  • 16GB RAM
  • Example project: so_repro.zip
    • Press W to spawn an entity, then press S to despawn it. If you remove the guard from the match expression on line 31, you can spawn multiple entities, but it crashes even if I only spawn and despawn one.
@TehPers
Copy link
Contributor Author

TehPers commented Apr 12, 2021

It might be worth noting that replacing the array in the struct with a Vec<u8> containing 32KB of data causes it to no longer stack overflow.

This was referenced Apr 12, 2021
Closed
Closed
@rparrett
Copy link
Contributor

Can repro with MacOS 11.2.3 (intel), rustc 1.51.0

Magic number was 58144.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen A-ECS Entities, components, systems, and events labels Apr 12, 2021
@cart
Copy link
Member

cart commented Apr 14, 2021

Hmm I'm guessing that the issue is that we're moving something allocated on the heap (the Assets<T> collection) onto the stack when we free it, but its too big to fit. I'm not sure what the best course of action is here, as we're calling HashMap::remove, which moves the item on to the stack.

@cart
Copy link
Member

cart commented Apr 14, 2021

This feels more like a "programming / language / OS constraint" problem than a Bevy problem.

bors bot pushed a commit that referenced this issue Apr 14, 2021
@TehPers
Implement RenderResource for Box<T> (#1893)
e0b5207 
Allows render resources to move data to the heap by boxing them. I did this as a workaround to #1892, but it seems like it'd be useful regardless. If not, feel free to close this PR.
@TehPers
Copy link
Contributor Author

TehPers commented Apr 15, 2021

Maybe it would be best just to document it as a limitation then? This issue can be avoided by just boxing large fields or using heap-allocated types like Vec<T>.

@mockersf mockersf mentioned this issue Apr 24, 2021
Closed
@bors bors bot closed this as completed in 0a8576b Apr 24, 2021
ostwilkens pushed a commit to ostwilkens/bevy that referenced this issue Jul 27, 2021
@TehPers @ostwilkens
Implement RenderResource for Box<T> (bevyengine#1893)
c1e4910 
Allows render resources to move data to the heap by boxing them. I did this as a workaround to bevyengine#1892, but it seems like it'd be useful regardless. If not, feel free to close this PR.
ostwilkens pushed a commit to ostwilkens/bevy that referenced this issue Jul 27, 2021
@mockersf @ostwilkens
support assets of any size (bevyengine#1997)
d4efe2f 
Fixes bevyengine#1892 

The following code is a cut down version of the issue, and crashes the same way:
```rust
enum AssetLifecycleEvent <T> {
    Create(T),
    Free
}

fn main() {
    let (sender, _receiver) = crossbeam_channel::unbounded();
    sender.send(AssetLifecycleEvent::<[u32; 32000]>::Free).unwrap();
}
```

- We're creating a channel that need to be able to hold `AssetLifecycleEvent::Create(T)` which has the size of our type `T`
- The two variants of the enums have a very different size

By keeping `T` boxed while sending through the channel, it doesn't crash
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Merged by Bors] - support assets of any size mockersf/bevy
4 participants
@rparrett @TehPers @cart @alice-i-cecile