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

Naive garbage collection (Adopted) #11582

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

tygyh
Copy link
Contributor

@tygyh tygyh commented Jan 28, 2024

Objective

Solution

  • Implement naive garbage collection

Copy link
Member

@BD103 BD103 left a comment

Choose a reason for hiding this comment

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

Awesome! Could you add doc comments to all of the new methods? The ones that were already added are a good template.

Additionally it appears that the CI is failing because the internal storage of certain structs have changed. You may need to update the respective functions to reflect these changes.

/// Shrinks the backing storage for the [`BlobVec`] such that the `len == capacity`.
///
/// This runs in `O(n)` time and may require reallocating backing buffer.
pub fn shrink_to_fit(&mut self) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a test to this method, since everything else seems to depend on this? Some good things to test:

  • Popping items from a BlobVec then shrinking
  • Shrinking a BlobVec with a capacity of 0
  • Shrinking a BlobVec with a capacity >= 1 but a length of 0

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times labels Jan 28, 2024
@james7132 james7132 self-requested a review January 29, 2024 19:25
let current_layout =
array_layout(&self.item_layout, self.capacity).expect("array layout should be valid");
self.data = if self.len == 0 {
unsafe {
Copy link
Member

Choose a reason for hiding this comment

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

This needs a safety comment, as does the realloc below. If you need a reference, the safety comments on the allocation functions in grow_exact should be pretty close to the ones we should use here.

crates/bevy_ecs/src/archetype.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/archetype.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/storage/mod.rs Outdated Show resolved Hide resolved
@@ -323,6 +323,12 @@ impl ComponentSparseSet {
}
}

pub fn shrink_to_fit(&mut self) {
Copy link
Member

Choose a reason for hiding this comment

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

This should probably have a doc comment as it's actually usable outside of bevy_ecs.

crates/bevy_ecs/src/storage/sparse_set.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/storage/table.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible C-Performance A change motivated by improving speed, memory usage or compile times
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants