[ecs] Rework Entity to use u64

[NathanSWard]

Objective

• Currently Entity is defined as struct Entity { id: u32, gen: u32 } however, this produces sub-optimal assembly for various operations (including: Eq, Ord, Copy, and new)
• Fixes Entity cmp::Eq improvement #2346

Solution

• Have entity be defined as a single Entity(u64) and use some bit manipulation to extract/store the id/generation numbers.
• This ends up producing better assembly in almost all cases.

[NathanSWard]

In case anyone is curious, here is a rust playground link to see the different assembly outputs.
https://play.rust-lang.org/?version=stable&mode=release&edition=2018&gist=e936d6c69c52df4bc559f47fc25650ff

Most notably the new (or as I call v2) implementation has nicer assembly for:

• new(id: u32)
• std::cmp::Eq
• std::cmp::Ord
• Copy

Old has better asm output for (by a single instruction):

• from_id_and_gen(id: u32, gen: u32)
• generation()
  (as both require a shift)

the old and new have almost identical assembly for retrieving the id

[mockersf]

could you run the benches from https://github.com/cart/ecs_bench_suite/tree/bevy-benches before/after (hopefully they don't need updating...)?

[TheRawMeatball]

Hmm, has anyone tried out unsafe optimizations yet? I tried adding #[repr(align(8))] to the old impl, and while it didn't improve all of them, it did have some positive effects. Also, I don't think testing with constants is very fair for the assembly output, as when they're changed to variables the new with id and gen function actually has one more instruction than the old impl.

[NathanSWard]

I don't think testing with constants is very fair for the assembly output, as when they're changed to variables the new with id and gen function actually has one more instruction than the old impl.

Yep, that's fair!
I updated the link/description to better reflect this.

[NathanSWard]

Hmm, has anyone tried out unsafe optimizations yet?
I'm not quite sure about which optimization you're talking about, however in the original linked issues, there's the recommendation to define Eq as:

impl Eq for Entity {
    fn eq(&self, other: &Self) -> bool {
        *(self as *const Entity as *const u64) == *(other as *const Entity as *const u64)
    }
}

which outputs the better assembly.

If we went this path, we (should) do it for Eq and Ord.

(The one that still really confuses me is the Copy assembly....

[TheRawMeatball]

Hmm, it looks like using arrays gets us almost all the way there, but is arguably easier to understand than bit manipulation: https://play.rust-lang.org/?version=stable&mode=release&edition=2018&gist=7c664f861a7d533cbe457cf37c7cb9a0

The only function that's different is the cmp impl, but this impl is never used in actual code, and more importantly could still be improved using unsafe: https://play.rust-lang.org/?version=stable&mode=release&edition=2018&gist=2fbce7c58019f4a98bb3d81f7e638515

[bjorn3]

(The one that still really confuses me is the Copy assembly....

The ABI is different. For EntityOld the abi is ScalarPair meaning that it is passed in two registers. For EntityNew the abi is Scalar meaning that it is passed in a single register. (You can annotate a type definition or alias with #[rustc_layout(abi)] on nightly to see the abi or with #[rustc_layout(debug)] to see the full layout.)

Edit: the argument pass mode is identical (as the type matches &<sized type>, in both cases), but the return pass mode is not. For EntityOld it is returned by return pointer. For EntityNew it is returned by register.

[NathanSWard]

Hmm, it looks like using arrays gets us almost all the way there, but is arguably easier to understand than bit manipulation:

I agree, I think the array is a little easier to understand, however, if we can avoid unsafe that's ideal (imo).
So I would have a slight preference for Entity(u64) however, I'm also more than happy to use Entity([u32;2])

[DJMcNab]

impl looks good, I have no thoughts on the performance.

[DJMcNab]

We should have a non-doc comment here saying what scheme we use and why

[DJMcNab]

Do we actually want this function?

[alice-i-cecile]

I've found this useful for writing tests before.

[NathanSWard]

Closing for now as we don't seem to have a strong argument for adding this extra complexity.