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

Child spawning optimizations #17339

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

Conversation

aevyrie
Copy link
Member

@aevyrie aevyrie commented Jan 13, 2025

Objective

Solution

  • Avoid re-parenting and related checks when spawning new entities, which cannot exist in the hierarchy yet.

Testing

  • Added a spawn_children benchmark

Showcase

Fixed exponential time complexity when spawning children on an entity that already has existing children. This case can now complete in linear time.

spawn_children/10_entities
                        time:   [1.4008 µs 1.4195 µs 1.4392 µs]
                        change: [-7.2431% -5.7892% -4.2650%] (p = 0.00 < 0.05)
spawn_children/100_entities
                        time:   [10.150 µs 10.302 µs 10.461 µs]
                        change: [-22.516% -21.350% -20.218%] (p = 0.00 < 0.05)
spawn_children/1000_entities
                        time:   [88.756 µs 89.768 µs 90.867 µs]
                        change: [-75.726% -75.512% -75.309%] (p = 0.00 < 0.05)
spawn_children/10000_entities
                        time:   [912.39 µs 924.42 µs 936.40 µs]
                        change: [-96.697% -96.659% -96.618%] (p = 0.00 < 0.05)
spawn_children/100000_entities
                        time:   [9.3389 ms 9.3936 ms 9.4529 ms]
                        change: [-99.655% -99.653% -99.651%] (p = 0.00 < 0.05)
spawn_children/1000000_entities
                        time:   [111.12 ms 111.84 ms 112.58 ms]

Note the 1 million case takes 111ms on this branch. On main, I had to kill the test because it was estimated to take 8 hours.

@aevyrie aevyrie added A-ECS Entities, components, systems, and events C-Performance A change motivated by improving speed, memory usage or compile times A-Hierarchy Parent-child entity hierarchies labels Jan 13, 2025
Copy link
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

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

The rationale for your changes looks sound to me. Just a minor nit around alloc::vec::Vec.

let events = children
.iter()
.map(|&child| HierarchyEvent::ChildAdded { child, parent })
.collect::<alloc::vec::Vec<_>>();
Copy link
Contributor

Choose a reason for hiding this comment

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

Would prefer use alloc::vec::Vec; at the top of the file.

@bushrat011899 bushrat011899 added X-Uncontroversial This work is generally agreed upon D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 13, 2025
@BenjaminBrienen BenjaminBrienen added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 13, 2025
Copy link
Contributor

@Jondolf Jondolf left a comment

Choose a reason for hiding this comment

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

Changes make sense and look solid to me.

@aevyrie
Copy link
Member Author

aevyrie commented Jan 13, 2025

I have a few more changes locally I need to push. I noticed that some of the child methods send a hierarchy event, but some do not. I can't tell if this is an error or not. The docs don't really spell out any intent.

@mockersf mockersf added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Jan 14, 2025
@aevyrie
Copy link
Member Author

aevyrie commented Jan 15, 2025

Can anyone with more familiarity please comment on the HierarchyEvents? It seems like it is outright missing in a few spawn methods but I'm not sure if this is a bug or not.

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 A-Hierarchy Parent-child entity hierarchies C-Performance A change motivated by improving speed, memory usage or compile times D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants