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

YohDeadfall · 2021-10-22T21:36:17Z

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 · 2021-10-22T22:00:49Z

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 · 2021-10-22T22:29:06Z

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

mockersf · 2021-10-22T22:55:03Z

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 · 2021-10-22T23:01:12Z

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

mockersf · 2021-10-22T23:12:21Z

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 · 2021-10-24T17:25:25Z

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 · 2021-10-24T18:15:11Z

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 · 2021-10-25T10:44:34Z

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.

Objective During work on #3009 I've found that not all jobs use actions-rs, and therefore, an previous version of Rust is used for them. So while compilation and other stuff can pass, checking markup and Android build may fail with compilation errors. Solution This PR adds `action-rs` for any job running cargo, and updates the edition to 2021.

alice-i-cecile · 2022-07-04T13:33:04Z

@YohDeadfall can you rebase?

YohDeadfall · 2022-07-04T20:08:19Z

Sure, will do.

YohDeadfall · 2022-07-19T12:50:30Z

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

alice-i-cecile · 2022-07-19T13:28:06Z

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

DJMcNab · 2022-07-19T13:30:05Z

Why is this needed?

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

YohDeadfall · 2022-07-19T13:31:03Z

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 · 2022-07-19T13:35:57Z

@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 · 2022-07-19T13:36:50Z

Ah yes, of course. Sorry.

superdump

I think this looks ok.

YohDeadfall · 2022-09-12T08:40:50Z

@superdump, @cart, can it be merged?

alice-i-cecile · 2022-10-31T15:35:40Z

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+

- 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.

bors · 2022-10-31T15:57:42Z

Pull request successfully merged into main.

Build succeeded:

- 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.

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Oct 22, 2021

This was referenced Oct 25, 2021

Specified toolchains for all jobs #3024

Closed

[Merged by Bors] - Update to edition 2021 on master #3028

Closed

alice-i-cecile added the hacktoberfest-accepted label Nov 3, 2021

cart removed the S-Needs-Review label Dec 16, 2021

Freeing unused memory held by visible entities

a33c85b

YohDeadfall force-pushed the shrink-visible-entities branch from 1d77c95 to a33c85b Compare July 19, 2022 12:49

superdump approved these changes Jul 19, 2022

View reviewed changes

alice-i-cecile added this to the Bevy 0.9 milestone Sep 12, 2022

mockersf approved these changes Oct 31, 2022

View reviewed changes

james7132 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Oct 31, 2022

bors bot changed the title ~~Freeing memory held by visible entities vector~~ [Merged by Bors] - Freeing memory held by visible entities vector Oct 31, 2022

bors bot closed this Oct 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

YohDeadfall commented Oct 22, 2021

mockersf commented Oct 22, 2021

YohDeadfall commented Oct 22, 2021

mockersf commented Oct 22, 2021 •

edited

Loading

YohDeadfall commented Oct 22, 2021

mockersf commented Oct 22, 2021

superdump commented Oct 24, 2021

YohDeadfall commented Oct 24, 2021

superdump commented Oct 25, 2021

alice-i-cecile commented Jul 4, 2022

YohDeadfall commented Jul 4, 2022

YohDeadfall commented Jul 19, 2022

alice-i-cecile commented Jul 19, 2022

DJMcNab commented Jul 19, 2022

YohDeadfall commented Jul 19, 2022 •

edited

Loading

superdump commented Jul 19, 2022

DJMcNab commented Jul 19, 2022

superdump left a comment

YohDeadfall commented Sep 12, 2022

alice-i-cecile commented Oct 31, 2022

bors bot commented Oct 31, 2022

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

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

Conversation

YohDeadfall commented Oct 22, 2021

Objective

Solution

mockersf commented Oct 22, 2021

YohDeadfall commented Oct 22, 2021

mockersf commented Oct 22, 2021 • edited Loading

YohDeadfall commented Oct 22, 2021

mockersf commented Oct 22, 2021

superdump commented Oct 24, 2021

YohDeadfall commented Oct 24, 2021

superdump commented Oct 25, 2021

alice-i-cecile commented Jul 4, 2022

YohDeadfall commented Jul 4, 2022

YohDeadfall commented Jul 19, 2022

alice-i-cecile commented Jul 19, 2022

DJMcNab commented Jul 19, 2022

YohDeadfall commented Jul 19, 2022 • edited Loading

superdump commented Jul 19, 2022

DJMcNab commented Jul 19, 2022

superdump left a comment

Choose a reason for hiding this comment

YohDeadfall commented Sep 12, 2022

alice-i-cecile commented Oct 31, 2022

bors bot commented Oct 31, 2022

mockersf commented Oct 22, 2021 •

edited

Loading

YohDeadfall commented Jul 19, 2022 •

edited

Loading