Update entity cloning benchmarks #17084
Conversation
In doing so, I've also renamed all the benchmarks and moved them under benchmark groups.
- Add lots of comments - Move functions specific to reflection coding behind the `if` gate - Fix `black_box()` being used incorrectly
Now they're grouped based on what kind of hierarchy they are, not whether they use `Clone` or `Reflect`. I also deleted all the extra structure definitions, so now each entity has one component of 64 bytes.
BD103
added
A-ECS
|
@eugineerd if you have the time, I would appreciate your review! Since you originally created entity cloning and its benchmarks, you can catch any mistakes that I made or things that I left out. Thanks! |
There was a problem hiding this comment.
This is a nice cleanup. I created this benchmark just to test the changes in #16717 and didn't put too much effort into making it right (not to mention this is my first time using criterion in general), so this is certainly an improvement.
My only concern is that 1 component per entity is not really representative of the average workload for this command. I would expect entities to have at least a couple different components in real-world scenario. If you look at the table in #16717 , you can see that the 1 component benchmarks actually regressed a bit, even though multiple components gained a big perf boost. I'd say that just having benchmark work for multiple components should be enough as 1 component isn't as common.
- Reintroduce `ComplexBundle`, `C2`, `C3`, `C4`, etc. - Make `single()` and `hierarchy_many()` use `ComplexBundle`. - Add documentation for which benchmarks use 1 or several components. - Decrease the size of the tree in `hierarchy_many()` so it doesn't take 15 seconds. - Remove the `#[reflect(Component)]` attribute from components, since it's not necessary to clone using `Reflect` anymore.
Thank you for the review, this was exactly the kind of thing I was hoping you'd catch! It should all be fixed now in e50ab98, but let me know if anything else needs changed. :) |
BD103
added
S-Ready-For-Final-Review
# Objective - `entity_cloning` was separated from the rest of the ECS benchmarks. - There was some room for improvement in the benchmarks themselves. - Part of #16647. ## Solution - Merge `entity_cloning` into the rest of the ECS benchmarks. - Apply the `bench!` macro to all benchmark names.\ - Reorganize benchmarks and their helper functions, with more comments than before. - Remove all the extra component definitions (`C2`, `C3`, etc.), and just leave one. Now all entities have exactly one component. ## Testing ```sh # List all entity cloning benchmarks, to verify their names have updated. cargo bench -p benches --bench ecs entity_cloning -- --list # Test benchmarks by running them once. cargo test -p benches --bench ecs entity_cloning # Run all benchmarks (takes about a minute). cargo bench -p benches --bench ecs entity_cloning ``` --- ## Showcase  Interestingly, using `Clone` instead of `Reflect` appears to be 2-2.5 times faster. Furthermore, there were noticeable jumps in time when running the benchmarks:  I theorize this is because the `World` is allocating more space for all the entities, but I don't know for certain. Neat!
Objective
entity_cloningwas separated from the rest of the ECS benchmarks.Solution
entity_cloninginto the rest of the ECS benchmarks.bench!macro to all benchmark names.\C2,C3, etc.), and just leave one. Now all entities have exactly one component.Testing
Showcase
Interestingly, using
Cloneinstead ofReflectappears to be 2-2.5 times faster. Furthermore, there were noticeable jumps in time when running the benchmarks:I theorize this is because the
Worldis allocating more space for all the entities, but I don't know for certain. Neat!