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

Fix MeshAllocator panic #14560

Merged

Conversation

eero-lehtinen
Copy link
Contributor

@eero-lehtinen eero-lehtinen commented Jul 31, 2024

Objective

Fixes #14540

Solution

  • Clean slab layouts from stale SlabIds when freeing meshes
  • Technically performance requirements of freeing now increase based on the number of existing meshes, but maybe it doesn't matter too much in practice
  • This was the case before this PR too, but it's technically possible to free and allocate 2^32 times and overflow with SlabIds and cause incorrect behavior. It looks like new meshes would then override old ones.

Testing

  • Tested in loading_screen example and tapping keyboard 1 and 2.

@eero-lehtinen eero-lehtinen marked this pull request as draft July 31, 2024 16:55
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen P-Crash A sudden unexpected crash S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 1, 2024
@alice-i-cecile alice-i-cecile requested a review from pcwalton August 1, 2024 00:14
@Brezak
Copy link
Contributor

Brezak commented Aug 1, 2024

The crash happens when we remove empty slabs in MeshAllocator::free_meshes. This may cause the slab ids in slab_layouts to no longer point to valid slabs. Instead of ignoring invalid slab ids in candidate_slabs could we remove them outright?

@eero-lehtinen
Copy link
Contributor Author

That would probably be better.

Also does the number in SlabId matter? next_slab_id will be infinitely increasing over a long session of spawning and despawning things. If it's somehow directly mapped to memory we could run out.

@eero-lehtinen eero-lehtinen force-pushed the fix-mesh-allocator-crash branch from 464a3ad to 53d4255 Compare September 7, 2024 13:58
@eero-lehtinen
Copy link
Contributor Author

Now I made the (probably) proper fix.

@eero-lehtinen eero-lehtinen marked this pull request as ready for review September 7, 2024 14:13
Copy link
Contributor

@kristoff3r kristoff3r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this is the right fix. It's true that free is now technically linear in the number of current slabs, but I don't think that will ever be a bottleneck. And it's simpler than breaking the invariant that all the SlabIds in the HashMaps are valid entries into slabs

Copy link
Contributor

@tychedelia tychedelia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems pretty straightforwardly correct compared to the previous fix.

@janhohenheim janhohenheim 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 Sep 12, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 16, 2024
Merged via the queue into bevyengine:main with commit db525e6 Sep 16, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior P-Crash A sudden unexpected crash S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

MeshAllocator panics when spawning and despawning lots of meshes
6 participants