From 78b5f323f87500bfd20eb5eb45a599d06324ba7b Mon Sep 17 00:00:00 2001 From: Nicola Papale Date: Sat, 13 Jan 2024 20:33:11 +0100 Subject: [PATCH] Skip alloc when updating animation path cache (#11330) Not always, but skip it if the new length is smaller. For context, `path_cache` is a `Vec>>`. # Objective Previously, when setting a new length to the `path_cache`, we would: 1. Deallocate all existing `Vec>` 2. Deallocate the `path_cache` 3. Allocate a new `Vec>>`, where each item is an empty `Vec`, and would have to be allocated when pushed to. This is a lot of allocations! ## Solution Use [`Vec::resize_with`](https://doc.rust-lang.org/stable/std/vec/struct.Vec.html#method.resize_with). With this change, what occurs is: 1. We `clear` each `Vec>`, keeping the allocation, but making the memory of each `Vec` re-usable 2. We only append new `Vec` to `path_cache` when it is too small. * Fixes #11328 ### Note on performance I didn't benchmark it, I just ran a diff on the generated assembly (ran with `--profile stress-test` and `--native`). I found this PR has 20 less instructions in `apply_animation` (out of 2504). Though on a purely abstract level, I can deduce this leads to less allocation. More information on profiling allocations in rust: https://nnethercote.github.io/perf-book/heap-allocations.html ## Future work I think a [jagged vec](https://en.wikipedia.org/wiki/Jagged_array) would be much more pertinent. Because it allocates everything in a single contiguous buffer. This would avoid dancing around allocations, and reduces the overhead of one `*mut T` and two `usize` per row, also removes indirection, improving cache efficiency. I think it would both improve code quality and performance. --- crates/bevy_animation/src/lib.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/bevy_animation/src/lib.rs b/crates/bevy_animation/src/lib.rs index c639e1efc9..02f8139a69 100644 --- a/crates/bevy_animation/src/lib.rs +++ b/crates/bevy_animation/src/lib.rs @@ -710,7 +710,9 @@ fn apply_animation( ); if animation.path_cache.len() != animation_clip.paths.len() { - animation.path_cache = vec![Vec::new(); animation_clip.paths.len()]; + let new_len = animation_clip.paths.len(); + animation.path_cache.iter_mut().for_each(|v| v.clear()); + animation.path_cache.resize_with(new_len, Vec::new); } if !verify_no_ancestor_player(maybe_parent, parents) { warn!("Animation player on {:?} has a conflicting animation player on an ancestor. Cannot safely animate.", root);