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

ecs: use generational entity ids and other optimizations #504

Merged
merged 3 commits into from
Sep 18, 2020

Conversation

cart
Copy link
Member

@cart cart commented Sep 17, 2020

Move back to generational entity ids

edit: I just realized I didn't call out that this is moving us back to the hecs Entities implementation. I didn't write most of the Entities code in this pr and I certainly don't want anyone to think I did.

This gives a pretty big speed boost when constructing the world and when we directly access components. When combined with the new apis below, this gives us lightning-fast direct entity component access from the world (about 2x faster than legion master, 3x faster than the current bevy_ecs implementation, and >2x faster than normal hecs).

The biggest tradeoffs of this approach:

  • Entity ids are no longer globally unique. They cannot be saved to a file and then used directly later. This means I had to remove SceneSpawner::load(). Now there is only SceneSpawner::spawn() (which creates new ids).
  • Entity ids can no longer be created in a decentralized manner (ex: Entity::new()). Instead, they must be created in the centralized Entities storage. This is multi-threadable because of atomics and the new EntityReserver type. Each Commands instance has its own copy of EntityReserver.
  • The implementation is much more complicated
  • We had to remove Commands::spawn_as_entity() and World::spawn_as_entity()

ReadOnlyFetch / read only queries

These traits are automatically implemented for any read only query. This allows us to guarantee that a query's world access will be read only.

Remove locking from World apis

This gives us a really nice speed boost. We can do this safely because of the addition of ReadOnlyFetch and making any mutable queries/accesses a mutable World borrow. This is not enabled for Queries in systems because a system could have multiple Queries, which could be iterated/accessed in a way that doesn't make mutable access unique. I think theres a way we can remove unnecessary locking here, but thats a difficult problem that should be handled separately. Fortunately "for-each" systems don't have any collision risk, so we now use lock-less queries there.

There are now the following QueryBorrow types:

  • QueryBorrow (no locks. used by World and foreach systems)
  • QueryBorrowChecked (locks. used by system Queries)

I did a quick investigation to see if perf didn't tank by removing QueryBorrow entirely. Sadly, it still tanks, so we still need to use &mut query.iter() and query.iter().iter().

Simplify Archetype TypeState access

The various archetype.get_x_with_y variants felt pretty gross. Now there is a single get_with_type_state() api, and TypeState exposes relevant collections directly.

Use usize directly for archetype indices / lengths

This cuts down on a lot of casting from u32 to usize

Add get_at_location apis to remove double entity lookups

The Query type used in systems first does an entity location lookup to validate archetype access. Currently, it does a world.get::<T>(entity) after that, which looks up the entity again. I added unsafe world.get_at_location<T: Component>(entity) apis, which allows us to cut out the redundant lookup in Query::get::<T>(entity).

Remove system Arc + RwLock from ParallelExecutor

Turns out we don't need them anymore! Not a major performance win, but every bit counts.

@cart
Copy link
Member Author

cart commented Sep 17, 2020

Ooh turns out bevy performs much better on the frag_iter test in ecs_bench_suite after this pr

@Moxinilian Moxinilian added A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible labels Sep 17, 2020
@karroffel karroffel added the C-Performance A change motivated by improving speed, memory usage or compile times label Sep 17, 2020
@memoryruins
Copy link
Contributor

It looks like this makes a lot of improvements in many places!

Entity ids are no longer globally unique. They cannot be saved to a file and then used directly later. This means I had to remove SceneSpawner::load(). Now there is only SceneSpawner::spawn() (which creates new ids).

While a tradeoff, this might ultimately simplify the API for users.

I saw that it brings entity IDs back to u32. Testing this PR with the minimized example in #369 (comment) quickly led to a collision

...
number of items : 49000
number of items : 50000
number of items : 51000
number of items : 51000
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: NoSuchEntity', bevy\crates\bevy_ecs\hecs\src\world.rs:572:46

@cart
Copy link
Member Author

cart commented Sep 17, 2020

Wow I'm glad you tested that. Collisions shouldn't be a problem unless we hit u32::MAX concurrently spawned entities, so this is a bit of a mystery. I also get failures consistently in the 48000-70000 "number of items" range.

Bumping to a u64 doesn't resolve this, or change the 48000-70000 failure range.

Interestingly, if we track the "max generation", its always zero. I would expect entity ids to get reused (and therefore have a generation bump). So something is amiss here.

@cart
Copy link
Member Author

cart commented Sep 17, 2020

Turns out this fails immediately after the first set of remove commands are applied (and 1000 items are removed). Now we just need to figure out why :)

@cart
Copy link
Member Author

cart commented Sep 17, 2020

lol its almost certainly this:
image

@cart
Copy link
Member Author

cart commented Sep 17, 2020

fixed!

crates/bevy_ecs/hecs/src/archetype.rs Outdated Show resolved Hide resolved
@cart cart merged commit 70ad667 into bevyengine:master Sep 18, 2020
mrk-its pushed a commit to mrk-its/bevy that referenced this pull request Oct 6, 2020
)

ecs: use generational entity ids and other optimizations
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 C-Feature A new feature, making something new possible C-Performance A change motivated by improving speed, memory usage or compile times
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants