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

Experimental refactor in legion_experiment crate #115

Merged
merged 48 commits into from
Jul 27, 2020
Merged

Conversation

TomGillen
Copy link
Collaborator

@TomGillen TomGillen commented Mar 8, 2020

This started as a simple refactor of legion into smaller sub-modules, as some of the module files were well over 1k LoC and were getting unmaintainable. However, while moving things around, I found some soundness issues and saw some opportunity to simplify things. Those in turn caused more changes downstream, and the simplifications made other improvements that I had been planning fairly trivial to implement. This ended up being a much larger and more impactful refactor of the entire legion_core crate than I had originally planned. This work is not yet complete, but it is wide reaching enough that I want to give everyone the opportunity to see what I've been doing and collect some feedback.

The general changes are:

  • Much shorter module files, each with a more clearly defined scope.
  • Simpler code in general. I don't have a line count yet, but I expect this to have shaved a few hundred lines off the library overall.
  • Removed many uses of unsafe.

Changes to World (which now more closely resembles a std collection):

  • Added len to retrieve entity count.
  • Renamed insert to extend.
  • Added push as sugar for inserting a single entity.
  • Renamed delete to remove.
  • Renamed is_alive to contains.
  • Added entity(Entity) -> Entry and entity_mut(Entity) -> EntryMut functions. Entries contain the APIs for interacting with a single entity. e.g. get_component, add_component and layout (which in turn offers has_component etc).
  • iter and iter_mut for iterating through all entity entries.
  • You can now insert with a tuple of component vecs, in addition to an iterator of component tuples. This is about 50% faster.

Changes to storage:

  • Much clearer distinction between structural changes to storage (e.g. adding entities) and the interior mutability involved in accessing components.
  • Chunk metadata has been pulled out into a LayoutIndex.
  • The layout index is optimised for search and provides a .search(EntityFilter) -> impl Iterator<Item = ArchetypeIndex> function.
  • Renamed "archetype" to "layout".
  • Renamed "chunk set" to "archetype".
  • Storage now derefs into [Archetype] and can be indexed by ArchetypeIndex.
  • Chunks no longer free their internal memory when empty. Defrag now deletes empty chunks.
  • Event subscribers can ask to recieve initial creation/insertion events for pre-existing archetypes and entities when they subscribe.

Changes to filters:

  • No longer implemented as iterators. This was a pain without GATs (it is the canonical streaming iterator problem).
  • Filters now only provide their matching logic.
  • Distinction made between stateless layout/archetype filters and stateful chunk filters.
  • Filters can now cache their matches and incrementally update queries. System queries should do this by default in the future.

It is likely that SystemQuery won't be needed anymore, too.

I don't forsee any more major API changes beyond completing this rework, and legion v1.0.0 will likely closely resemble this, after which things should stabilise and breaking changes will hopefully be rare.

Copy link
Contributor

@Guvante Guvante left a comment

Choose a reason for hiding this comment

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

I sat down and read some of the changes at a high level and made minor comments about a couple of parts that tripped me up.

All the changes look good so far by the way.

legion_experiment/src/borrow.rs Outdated Show resolved Hide resolved
legion_experiment/src/query/view.rs Outdated Show resolved Hide resolved
legion_experiment/src/query/view.rs Outdated Show resolved Hide resolved
legion_experiment/src/query/view.rs Outdated Show resolved Hide resolved
legion_experiment/src/world.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@Guvante Guvante left a comment

Choose a reason for hiding this comment

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

I noticed that the Or macro matches the And macro for how it coalesces while seeing if the new version made a fix for weird behavior when different levels of queries interact better.

legion_experiment/src/storage/filter.rs Outdated Show resolved Hide resolved
legion_experiment/src/storage/filter.rs Outdated Show resolved Hide resolved
@caelunshun caelunshun mentioned this pull request Mar 15, 2020
2 tasks
@ooflorent
Copy link

It looks like there are some confusions around matches_layout: sometimes it is called with components first, sometimes with tags.

@TomGillen
Copy link
Collaborator Author

TomGillen commented Apr 28, 2020

I have just pushed something I have been messing about with for some time.

Packed Archetypes

The largest change is the introduction of what I am calling "packed archetypes". Legion can now optimise the layout of its internal storage for specific access orders as defined by pre-defined component groupings. Component slices for each archetype may be packed in-order into a single memory allocation if the world determines it to be worthwhile. Packed archetype slices can shrink and grow in-place but, if they run into the next slice, they will be "unpacked" into their own vec. This is very similar to having to resize a vec as items are added. An archetype slice will only be considered a candidate for packing if the slice has remained stable (i.e. no entities added or removed) for a certain amount of time. I would expect this to include the vast majority of the entities in most typical game scenes. This decision is made per component type, not per archetype. Each component storage maintains a "fragmentation" heuristic, which is an estimate of the number of cache misses which would be avoided if the storage were re-packed, and this is used to determine if a component should be packed.

Component groups are specified via a tupl;e of component types (e.g. <(A, B, C)>::into_group(), which will accelerate queries which exactly ask for (A), (A, B), or (A, B, C). Queries which do not match a group are not negatively impacted relative to how legion has always performed without this feature.

The largest code change as a result of this feature is that all components of a specific type are now owned by a single component storage per world, rather than each archetype holding onto its own component storage. Archetypes are represented as slices of components whereby each index across all slices refer to the same entity, as they do now, but these slices are no longer divided into fixed size chunks.

Tags

I have not ported tags over to this architecture. It could be done, but I am not yet decided whether or not to drop tags as a feature. Legion#s internal design is very heavily inspired by Unity's ECS, and I initially found the example use cases Unity gave for their "shared components" to be quite compelling (and, indeed, those examples are still compelling). However, in using both legion and the Unity ECS I have found myself and my colleages very rarely using the feature. More importantly, one of the most commonly asked questions I get from users of legion is "should I use a component or a tag?", and my answer so have has, every time, been "use a component". I would like to hear more feedback on people's experience with legions tags before deciding on how to proceed here.

Performance

I have not done a lot of benchmarking of this branch so far, but here are the preliminary results:

legion-master           time:   [6.0800 us 6.0970 us 6.1150 us]
Found 2 outliers among 100 measurements (2.00%)
  1 (1.00%) high mild
  1 (1.00%) high severe

legion-packed-archetypes ordered
                        time:   [2.1003 us 2.1337 us 2.1897 us]
Found 6 outliers among 100 measurements (6.00%)
  2 (2.00%) high mild
  4 (4.00%) high severe

legion-packed-archetypes unordered
                        time:   [2.0995 us 2.1083 us 2.1168 us]
Found 6 outliers among 100 measurements (6.00%)
  1 (1.00%) low mild
  5 (5.00%) high mild

This benchmark creates entities with 4 different component layouts, (A,), (A, B), (A, B, C) and (A, B, C, D), with 1000 entities for each layout. It measures the iteration times for a query over (&A, &mut B).

The experimental branch is 3 times as fast.

However, there are a few things to note. The first is that the "ordered" benchmark (which defines an (A, B, C, D) group) is slightly slower than the "unordered" benchmark. The difference is small, but it is consistent. This makes no sense. I would not expect to see much gain from archetype packing when there are only three archetypes, but even so the "ordered" query path is identical except for performing one fewer indirection into a lookup table and its accesses are more linear and ordered.

The second issues is that, upon further investigation, I do not believe that the performance difference between the two branches is actually caused by any of the major changes in this branch....

At its core, a legion query produces an iterator of chunks. Those chunks can then be iterated through to pull out each entity's components. The most "manual" way of doing this is via a nested loop:

for chunk in query.iter_chunks(&world) {
   for components in chunk {
      ....
   }
}

On the experimental branch, this takes approximately 150ns in one of my benchmarks.

However, users usually don't want to interact with chunks directly, and so we provide wrapper functions which will flatten this nested loop. On the experimental branch, this is simply implemented by wrapping iter_chunks inside a std::iter::Flatten. Master uses a custom iterator, but it functions in much the same way. Flatten will iterate though the outer iterator, call IntoIterator on each value, and then iterate through the inner iterator. It does just what our manual nested loop does.

query.iter(&world) takes 800ns.

It would appear that std::iter::Flatten is really slow. The experimental branch's Query::for_each is implemented internally via a nested loop, and I believe this accounts for most of the performance improvement.

Misc Changes

Universe Sharding

Universes can be sharded via Universe::sharded(n: u64, of: u64) -> Universe. Sharding a universe will divide the Entity ID space between all shards, ensuring that each universe will allocate unique IDs. This can be used to, for example, ensure that entity IDs for serialised content and runtime created entities will never conflict, or to ensure that simulations running in different processes (or different machines) will have unique IDs.

Entity IDs are no longer generational

We no longer ever use an entity ID as an index, which means we don't need to try and limit the size of the address space via generations. Entity IDs are now defined as Entity(u64) and are allocated by incrementing an atomic counter. This greatly simplifies entity ID management, which was a large portion of the entity insertion time on the master branch (about 50% of entity insertion cost).

Query Caching

Caching of query archetype layout matches is now mandatory and always happens.

Chunk API

The chunks returned from iter_chunk (of which there is now a 1:1 match with archetypes) have a few new APIs:

  • into_iter(). Chunks now implement IntoIterator which produces an iterator over the view components.
  • into_components(). This returns a friendly raw representation of the chunk's internal data. The exact type is determined by the view, but for example a (Read<A>, Write<B>, TryWrite<C>) view would return (&[A], &mut [B], Option<&mut [C]>).
  • archetype(&self) -> &Archetype. Allows you to retrieve the archetype associated with the chunk. This archetype allows the user to ask about the layout of the entity such as what component types are attached to the entities in the chunk.

Entity is now a View.

The Query::iter_entities, Query::for_each_entities and related methods are gone. Instead, users can include Entity in their view. e.g. <(Entity, Read<A>, Write<B>)>::into_query().

@Nehliin
Copy link

Nehliin commented May 4, 2020

@TomGillen I have been using "model handle" which refers to a unique asset as a tag in my code. The handle itself contains an id which is used to fetch the actual asset. This means I can do chunk iteration of transform data tagged by a model handle and do instance drawing per chunk which is really nice. Here is a short snippet showing it off:

let mut offset_map = HashMap::new()
let query =
            <(Read<Transform>, Tagged<ModelHandle>)>::query().filter();
for chunk in query.par_iter_chunks(world) {
     // This is guaranteed to be the same for each chunk
     let model = chunk.tag::<ModelHandle>().unwrap();
     let offset = *offset_map.get(model).unwrap_or(&0);
     let transforms = chunk.components::<Transform>().unwrap();
     offset_map.insert(model.clone(), offset + transforms.len());
     let model = asset_manager.get_model(model).unwrap();
     render_pass
          .draw_model_instanced(model, offset as u32..(offset + transforms.len()) as u32);
}

How could I achieve something similar using components? If I would use a unique model handle type per asset I would have to have one query per unique model which is certainly not maintainable in the long run.

@kabergstrom
Copy link
Contributor

kabergstrom commented May 4, 2020

Tags are like custom primary keys, which is very useful for modelling data layout. Recommended usage is probably approximately similar to primary keys in RDMSs, except point lookups are obviously not fast.

I don't really like the idea of the storage system doing things like repacking automatically based on heuristics. But my understanding is that the grouping feature is opt-in?

@cart
Copy link

cart commented May 5, 2020

I'm building a general purpose game engine with legion and I do not use tags anywhere. I agree that being able to iterate over "chunks" having the same asset id tags seems powerful on paper, but in practice i don't just care about iterating over sets of entities that use the same model/texture pair. I also care about entity position in the world so I can draw back-to-front or front-to-back. This means that I'm accessing entities randomly anyway.

I also consider tags to be confusing for users. I abstract tags out entirely in my "WorldBuilder" api because i believe they negatively affect legibility and introduce cognitive load . I'm sure theres a use case for tags, I just haven't personally encountered it. Ideally tags would be optional / only a part of the api when you need them.

@TomGillen
Copy link
Collaborator Author

I don't really like the idea of the storage system doing things like repacking automatically based on heuristics. But my understanding is that the grouping feature is opt-in?

Yes. Repacking only happens if you call World::pack, and you can configure how it decides when a component storage is sufficiently fragmented to be repacked, and how each archetype within is considered eligable for packing.

@cart
Copy link

cart commented May 19, 2020

It would be great to be able to use Entity(u64) as a stable id for serialization / networking. Currently that isn't possible because entity id allocation is fully abstracted out from the user. It seems like this would be possible with the following changes:

  • EntityAllocator becomes a trait. By default it could still be an ascending atomic counter, but there could be an alternative impl that avoids collisions.
  • Expose an api that lets users instantiate entities with explicit Entity values. Maybe something like world.extend_with_ids(vec![(A,B), (A,B)], vec![Entity(0), Entity(1)])
    • EDIT: this part might not even be necessary. the EntityAllocator could be manually configured to return a list of ids for the next X requests. serializers could then be careful about configuring EntityAllocator with the correct ids for the next X insertions.

Of course users could always define their own EntityId component, but that seems a bit redundant given that the definition of Entity is "unique identifier of entity in world" and the definition of EntityId would also be "unique identifier of entity in world". It would also mean that I would need to build a higher level wrapper over entity insertion to ensure that all entities have the EntityId component.

@cart
Copy link

cart commented May 20, 2020

Last message on the subject because I dont want to spam you all with potentially off topic information. I just created a "GuidEntityAllocator" in my legion fork that generates a new random id for each entity and allows "queuing up" new Entity ids. Nothing appears to be broken in my relatively complex project and I was able to write a serde implementation that preserves entity ids.

It works well enough for my use cases that I will probably roll with it regardless of the choices made here, but it would be nice to support this in upstream legion.

@TomGillen
Copy link
Collaborator Author

And does the Duplicate merger not allow for this?

@aclysma
Copy link
Contributor

aclysma commented Jul 11, 2020

Functionally it might, but clone_from and merge_from both take the source world by mutable reference. Probably because it's sharing the codepath with Move. Merger::merge_archetype passes the source world along to those impls via mutable reference.

@TomGillen
Copy link
Collaborator Author

Yea, it might make more sense to separate the two further to allow duplicate to only require &World. Does the functionality do what you need it to though? That type was designed to allow what I thought you were doing with the prefab stuff, but without requiring the user to implement so much code themselves before they could use it, but I might have overlooked something.

@aclysma
Copy link
Contributor

aclysma commented Jul 11, 2020

I think it will but it's hard to say without trying it. It did feel like there was a case for both a Merger and MergerMut but it might increase the code required by quite a bit. For what it's worth, I'm not planning to use either the Move or Duplicate impls of Merger. Similar to serialization we already have a registry for other reasons (like hooks for serde_diff and atelier assets). The prefab crate provides its own interfaces for downstream code to specify how to clone/transform data. The Merger trait looks like it could do the job but I'd like to implement it to make certain. :)

@TomGillen
Copy link
Collaborator Author

TomGillen commented Jul 13, 2020

@aclysma and @kabergstrom

I have reworked how entity IDs, serialization and merging work in response to your feedback. It all makes more sense when you see how it all fits together, but I'll try and explain:

Entitites:

  • Entity IDs are opaque structures. They identify a location in a world at which an entity may exist. Users cannot create their own Entity structs.
  • Any given world may or may not contain a specific Entity, and that may change as the world is mutated.
  • Entity IDs are meaningless outside of the process.

Canon Names:

  • Entities can be assigned a canonical name (a 16 byte UUID).
  • This is the only form of "strong identity" that an entity can have.
  • A given name will always be associated with the same given Entity ID within the same universe.
  • An entity pushed into two worlds with the same name will be assigned the same Entity in both worlds.
  • "Canonized" entities are the only way that two entities in different worlds can be assigned the same identity.
  • Any attempt to push an entity with a name into a world which already has that name (via push_named, universe.canon_mut().canonize, merging, or deserialization) will cause the entity in the destination to be replaced with that defined in the source. As names are the only way two entities can have the same identity, this is the only way two entities can "conflict".

Serialization:

  • As Entity is meaningless outside of the process, serialized entities are always identified via their canon name.
  • Entities without a name will be assigned one during serialization.
  • These names round trip correctly. i.e. If you create a world, serialize it, deserialize it in another process, modify the world, and serialize it again, the entity names will be stable.

Entity Serialization:

  • The Entity struct, wherever it appears inside a component as a reference to another entity, will serialize itself as the target entity's name.
  • The target entity will be canonized if it does not yet have a name.
  • Upon deserialization, the name will be resolved to an Entity ID.
  • The above works, even if the target entity is not defined in the file, or does not yet exist in any runtime world.
  • When an entity with the same name as that referenced is at any point created, it will be created with the same Entity ID as was deserialized, and no other entity will ever be created with that ID.

I have also made a few miscellaneous changes: fixed a few types not being exported, separated clone_from and copy_from code paths, clone_from no longer requires &mut World, etc. Universe sharding is gone, as it does not make any sense any more.

@TomGillen
Copy link
Collaborator Author

JSON (human readable) serialization format now looks like:

{
  "entities": {
    "14ab8211-4a7c-4817-813b-8acf282c27de": {
      "bool": false,
      "isize": 4,
      "usize": 4
    },
    "614a1c19-3718-4d30-b249-04d5c45df547": {
      "entity_ref": "d6be1905-3714-40be-8226-60f3177ca395",
      "isize": 6,
      "usize": 6
    },
    "9d51f554-92c5-446b-b764-5892be5dd14a": {
      "bool": false,
      "isize": 3,
      "usize": 3
    },
    "c8ca7a5c-834b-4f7e-a359-68a3a3779ed7": {
      "entity_ref": "d6be1905-3714-40be-8226-60f3177ca395",
      "isize": 7,
      "usize": 7
    },
    "d3ef5175-d430-4f15-ab16-9b6082bfeefd": {
      "bool": false,
      "isize": 2,
      "usize": 2
    },
    "d6be1905-3714-40be-8226-60f3177ca395": {
      "bool": false,
      "isize": 1,
      "usize": 1
    },
    "ea27971a-9c71-41a6-babb-4c1a47418c77": {
      "entity_ref": "d6be1905-3714-40be-8226-60f3177ca395",
      "isize": 8,
      "usize": 8
    },
    "fa5f9c08-a308-4555-a7e5-11121d2e499f": {
      "entity_ref": "d6be1905-3714-40be-8226-60f3177ca395",
      "isize": 5,
      "usize": 5
    }
  }
}

Where "isize", "usize" "bool" and "entity_ref" are the external names registered with the Registry for the component type IDs (these would probably be UUIDs in a proper application).

@TimonPost
Copy link
Member

TimonPost commented Jul 15, 2020

Json serializer/deserializer seems to work good, however, bincode deserializer doesn't seem to deserialize very well.

    // JSON
    let serialized = serde_json::to_value(&world.as_serializable(any(), &registry)).unwrap();
    println!("{:#}", serialized.to_string());

    let universe = Universe::new();
    let world: World = registry
        .as_deserialize(&universe)
        .deserialize( &mut serde_json::Deserializer::from_str(&serialized.to_string())).unwrap();

    println!("Number of entities in deserialized world {}\n\n", world.len());

    // BINCODE
    let serialized = bincode::serialize(&world.as_serializable(any(), &registry)).unwrap();
    println!("{:?}", serialized);

    let universe = Universe::new();
    let world: World = registry
        .as_deserialize(&universe)
        .deserialize( &mut bincode::Deserializer::from_slice(&serialized, bincode::DefaultOptions::default())).unwrap();

    println!("Number of entities in deserialized world {}", world.len())

Any ideas why this is?

@TomGillen
Copy link
Collaborator Author

@TimonPost Your bincode example won't deserialize correctly because you serialized with bincode::serialize and deserialized using deserialize with bincode::DefaultOptions::default(). Believe it or not, but bincode's default serialization options are not DefaultOptions::default(). You need to also used fixed int length encoding and allow trailing bytes. Not setting those configuration options causes bincode to just read nonsense and often without erroring (in this case, some of the sequences are being read as empty).

You need to use:

bincode::DefaultOptions::new()
    .with_fixint_encoding()
    .allow_trailing_bytes()

@TimonPost
Copy link
Member

TimonPost commented Jul 25, 2020

I again came across weird behavior in my bincode serializing code. When a type id is not known it will use IgnoreAny here. IgnoreAny is not supported by bincode and will throw an error (Bincode rust does not support Serializer::deserialize ignored any).

This is done in the following two places:

https://github.com/TomGillen/legion/pull/115/files#diff-fb7fb3d74cc05a5c59858d2d6b136bd8R249
https://github.com/TomGillen/legion/pull/115/files#diff-613f8cc135f1762b6483319d71983843R406

While the legion logic is correct logic there is some underlying issue considering the nature of TypeId and its unique hash when using legion across multiple binaries.

I use two separate binaries who use the same registered types and same logic for constructing the registry with the types but somehow the SerializedTypeIds are different for each binary. Those two binaries are two endpoints communication over the interwebs. Upon receipt of the world state, it should deserialize it and apply it to the local game world. Because of the deferring, SerializeTypeId's this code will error out because the type id coming from the other binary is different then from the one that is registerd locally. This is not only a problem with bincode but with any other serializer, it will ignore the deserializing and thus I end up with an empty entity in my world.

I am not sure if this is something to care about. But if legion is used across multiple binaries/network using TypeId can be misleading because it defers on each platform. If this is known I would suggest placing a comment it can save a lot of confusion for users. It is possible to specify an own type id right?

Related issues to the Type id problem

rust-lang/rust#61553

@kabergstrom
Copy link
Contributor

kabergstrom commented Jul 25, 2020

@TimonPost SerializableTypeId has this comment:

/// A default serializable type ID. This ID is not stable between compiles,
/// so serialized words serialized by a different binary may not deserialize
/// correctly.

I think it's a bad default to include in the project though, since it's more or less useless for what you'd expect it to be.

As for IgnoreAny, bincode is not self-delimiting so it's impossible to deserialize something without knowing the exact type layout used when serializing. Unfortunately this is a limitation of the format, if you want something more robust, I'd suggest messagepack.

@kabergstrom
Copy link
Contributor

I recommend using https://docs.rs/type-uuid/0.1.2/type_uuid/ for your serialized type ID

@TomGillen
Copy link
Collaborator Author

I am going to merge this PR. Further feedback should be better served with more focused and specific issues and PRs. I am going to give it a bit more time for us to discover and work through issues with this rewrite before publishing 0.3 to crates.io.

@TomGillen TomGillen merged commit 91266c8 into master Jul 27, 2020
@TomGillen TomGillen deleted the experimental branch July 27, 2020 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants