[Merged by Bors] - Freeing memory held by visible entities vector

[YohDeadfall]

• Freeing unused memory held by visible entities
• Fixed comment style

Objective

With Rust 1.56 it's possible to shrink vectors to a specified capacity. Visibility system had a comment before asking for that feature to free unused memory by a vector if its capacity is two times larger than the length.

Solution

Shrinking the vector of visible entities to the nearest power of 2 elements next to len(), if capacity exceeds it more than two times.

[mockersf]

This code seems to have been removed in the rendering rework: https://github.com/bevyengine/bevy/tree/pipelined-rendering/pipelined/bevy_render2/src/camera

[YohDeadfall]

How visibility is controlled then? Cannot find anything about in in the pipelined rendering.

[mockersf]

According to #2535, it's not!

#2861 would add it but still waiting for reviews and merge. There's still the TODO: https://github.com/bevyengine/bevy/pull/2861/files#diff-4dee68290412f102be8ef381f19aa5f435836709d962ac0207680067f5442799R181-R184

[YohDeadfall]

Should then I make another PR or change the target for this one to fix the issue in the pointed branch?

[mockersf]

You can either:

• open a PR on @superdump Bevy fork https://github.com/superdump/bevy/tree/visibility to fix it there, which will include your fix in [Merged by Bors] - Frustum culling #2861
• check with @superdump if they could include your fix directly
• wait for [Merged by Bors] - Frustum culling #2861 to be merged and then open a PR on Bevy to apply your fix

Sadly there's no obvious way forward...

[superdump]

In pipelined-rendering, we have a bunch of places where we need to handle this kind of thing with Vecs of things. It would probably be good to implement a generic utility function that does it and use that everywhere it's needed.

I was trying to figure out the behaviour because of integer divisions and so rounding. The code that does it in this PR is:

        let capacity = visible_entities.value.capacity();
        let reserved = capacity
            .checked_div(visible_entities.value.len())
            .map_or(0, |reserve| {
                if reserve > 2 {
                    capacity / (reserve / 2)
                } else {
                    capacity
                }
            });

        visible_entities.value.shrink_to(reserved);

I wrote some code to quickly test what the results would be for some small set of numbers:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=ab9002756a32df73ba9894655057a216

capacity 2, len 1: reserve = 2 / 1 = 2; reserved = 2 / (2 / 2) = 2
capacity 3, len 1: reserve = 3 / 1 = 3; reserved = 3 / (3 / 2) = 3
capacity 4, len 1: reserve = 4 / 1 = 4; reserved = 4 / (4 / 2) = 2
capacity 5, len 1: reserve = 5 / 1 = 5; reserved = 5 / (5 / 2) = 2
capacity 6, len 1: reserve = 6 / 1 = 6; reserved = 6 / (6 / 2) = 2
capacity 7, len 1: reserve = 7 / 1 = 7; reserved = 7 / (7 / 2) = 2
capacity 8, len 1: reserve = 8 / 1 = 8; reserved = 8 / (8 / 2) = 2
capacity 9, len 1: reserve = 9 / 1 = 9; reserved = 9 / (9 / 2) = 2
capacity 10, len 1: reserve = 10 / 1 = 10; reserved = 10 / (10 / 2) = 2
capacity 4, len 2: reserve = 4 / 2 = 2; reserved = 4 / (2 / 2) = 4
capacity 5, len 2: reserve = 5 / 2 = 2; reserved = 5 / (2 / 2) = 5
capacity 6, len 2: reserve = 6 / 2 = 3; reserved = 6 / (3 / 2) = 6
capacity 7, len 2: reserve = 7 / 2 = 3; reserved = 7 / (3 / 2) = 7
capacity 8, len 2: reserve = 8 / 2 = 4; reserved = 8 / (4 / 2) = 4
capacity 9, len 2: reserve = 9 / 2 = 4; reserved = 9 / (4 / 2) = 4
capacity 10, len 2: reserve = 10 / 2 = 5; reserved = 10 / (5 / 2) = 5
capacity 6, len 3: reserve = 6 / 3 = 2; reserved = 6 / (2 / 2) = 6
capacity 7, len 3: reserve = 7 / 3 = 2; reserved = 7 / (2 / 2) = 7
capacity 8, len 3: reserve = 8 / 3 = 2; reserved = 8 / (2 / 2) = 8
capacity 9, len 3: reserve = 9 / 3 = 3; reserved = 9 / (3 / 2) = 9
capacity 10, len 3: reserve = 10 / 3 = 3; reserved = 10 / (3 / 2) = 10
capacity 8, len 4: reserve = 8 / 4 = 2; reserved = 8 / (2 / 2) = 8
capacity 9, len 4: reserve = 9 / 4 = 2; reserved = 9 / (2 / 2) = 9
capacity 10, len 4: reserve = 10 / 4 = 2; reserved = 10 / (2 / 2) = 10
capacity 10, len 5: reserve = 10 / 5 = 2; reserved = 10 / (2 / 2) = 10

Note that there are a lot of divide by zero cases for the capacity / (reserve / 2) so many lines are missing.

[YohDeadfall]

There should be no division by zero since reserve is always larger than two. Therefore, reserve / 2 always gives at least 1.

By the way, are big len() changes frequent? If so the proposed solution is definitely not accurate. If it's more or less stable, shrinks should happen not so often.

Okay, I can work on a generic solution and take a look for an existing in external crates first. Perhaps, a better approach would be a chunked vector to avoid copying on grow. Maybe with an arena allocator.

Could you write me down requirements on the feature, so I can do my best from the start and not to spend our time on extra reviews?

[superdump]

Good point! I forgot to add that detail into the test code.

I'd defer to @cart for design requirements. :) Good suggestions by the way.

[alice-i-cecile]

@YohDeadfall can you rebase?

[YohDeadfall]

Sure, will do.

[YohDeadfall]

@alice-i-cecile, rebased and updated the code.

[alice-i-cecile]

@superdump @james7132 @DJMcNab, I'd like your opinions on this at some point.

[DJMcNab]

Why is this needed?

VisibleEntities is reallocated every frame at the moment, due to the new extract system.

[YohDeadfall]

Ideally, the solution shall be like in databases to avoid shrinking memory and do relocation too often. I guess, at least it should happen once per 30 seconds or in N frames. The best solution is to collect statistics on usage and if there are spikes, do nothing.

[superdump]

@DJMcNab the systems in question are in the main app schedule I think. The checking of visibility is done in the main app before extraction.

[DJMcNab]

Ah yes, of course. Sorry.

[superdump]

I think this looks ok.

[YohDeadfall]

@superdump, @cart, can it be merged?

[alice-i-cecile]

I'm not fully convinced that this is the best place to do this cleanup, but this is a nice incremental improvement that fixes the TODO.

bors r+

[bors]

Pull request successfully merged into main.

Build succeeded:

• build-and-install-on-iOS
• build-android
• build (macos-latest)
• build (ubuntu-latest)
• build-wasm
• build (windows-latest)
• build-without-default-features (bevy)
• build-without-default-features (bevy_ecs)
• build-without-default-features (bevy_reflect)
• check-compiles
• check-doc
• check-missing-examples-in-docs
• ci
• markdownlint
• run-examples
• run-examples-on-wasm
• run-examples-on-windows-dx12