Optimize Entity::eq (#10519)

(This is my first PR here, so I've probably missed some things. Please
let me know what else I should do to help you as a reviewer!)

# Objective

Due to rust-lang/rust#117800, the `derive`'d
`PartialEq::eq` on `Entity` isn't as good as it could be. Since that's
used in hashtable lookup, let's improve it.

## Solution

The derived `PartialEq::eq` short-circuits if the generation doesn't
match. However, having a branch there is sub-optimal, especially on
64-bit systems like x64 that could just load the whole `Entity` in one
load anyway.

Due to complications around `poison` in LLVM and the exact details of
what unsafe code is allowed to do with reference in Rust
(rust-lang/unsafe-code-guidelines#346), LLVM
isn't allowed to completely remove the short-circuiting. `&Entity` is
marked `dereferencable(8)` so LLVM knows it's allowed to *load* all 8
bytes -- and does so -- but it has to assume that the `index` might be
undef/poison if the `generation` doesn't match, and thus while it finds
a way to do it without needing a branch, it has to do something slightly
more complicated than optimal to combine the results. (LLVM is allowed
to change non-short-circuiting code to use branches, but not the other
way around.)

Here's a link showing the codegen today:
<https://rust.godbolt.org/z/9WzjxrY7c>
```rust
#[no_mangle]
pub fn demo_eq_ref(a: &Entity, b: &Entity) -> bool {
    a == b
}
```
ends up generating the following assembly:
```asm
demo_eq_ref:
        movq    xmm0, qword ptr [rdi]
        movq    xmm1, qword ptr [rsi]
        pcmpeqd xmm1, xmm0
        pshufd  xmm0, xmm1, 80
        movmskpd        eax, xmm0
        cmp     eax, 3
        sete    al
        ret
```
(It's usually not this bad in real uses after inlining and LTO, but it
makes a strong demo.)

This PR manually implements `PartialEq::eq` *without* short-circuiting,
and because that tells LLVM that neither the generations nor the index
can be poison, it doesn't need to be so careful and can generate the
"just compare the two 64-bit values" code you'd have probably already
expected:
```asm
demo_eq_ref:
        mov     rax, qword ptr [rsi]
        cmp     qword ptr [rdi], rax
        sete    al
        ret
```

Since this doesn't change the representation of `Entity`, if it's
instead passed by *value*, then each `Entity` is two `u32` registers,
and the old and the new code do exactly the same thing. (Other
approaches, like changing `Entity` to be `[u32; 2]` or `u64`, affect
this case.)

This should hopefully merge easily with changes like
#9907 that also want to change
`Entity`.

## Benchmarks

I'm not super-confident that I got my machine fully consistent for
benchmarking, but whether I run the old or the new one first I get
reasonably consistent results.

Here's a fairly typical example of the benchmarks I added in this PR:

![image](https://github.com/bevyengine/bevy/assets/18526288/24226308-4616-4082-b0ff-88fc06285ef1)

Building the sets seems to be basically the same. It's usually reported
as noise, but sometimes I see a few percent slower or faster.

But lookup hits in particular -- since a hit checks that the key is
equal -- consistently shows around 10% improvement.

`cargo run --example many_cubes --features bevy/trace_tracy --release --
--benchmark` showed as slightly faster with this change, though if I had
to bet I'd probably say it's more noise than meaningful (but at least
it's not worse either):

![image](https://github.com/bevyengine/bevy/assets/18526288/58bb8c96-9c45-487f-a5ab-544bbfe9fba0)

This is my first PR here -- and my first time running Tracy -- so please
let me know what else I should run, or run things on your own more
reliable machines to double-check.

---

## Changelog

(probably not worth including)

Changed: micro-optimized `Entity::eq` to help LLVM slightly.

## Migration Guide

(I really hope nobody was using this on uninitialized entities where
sufficiently tortured `unsafe` could could technically notice that this
has changed.)

benches/Cargo.toml

@@ -61,3 +61,8 @@ harness = false
 name = "bezier"
 path = "benches/bevy_math/bezier.rs"
 harness = false
+
+[[bench]]
+name = "utils"
+path = "benches/bevy_utils/entity_hash.rs"
+harness = false

benches/benches/bevy_utils/entity_hash.rs

@@ -0,0 +1,75 @@
+use bevy_ecs::entity::Entity;
+use bevy_utils::EntityHashSet;
+use criterion::{criterion_group, criterion_main, BenchmarkId, Criterion, Throughput};
+use rand::{Rng, SeedableRng};
+use rand_chacha::ChaCha8Rng;
+
+criterion_group!(benches, entity_set_build_and_lookup,);
+criterion_main!(benches);
+
+const SIZES: [usize; 5] = [100, 316, 1000, 3162, 10000];
+
+fn make_entity(rng: &mut impl Rng, size: usize) -> Entity {
+    // -log₂(1-x) gives an exponential distribution with median 1.0
+    // That lets us get values that are mostly small, but some are quite large
+    // * For ids, half are in [0, size), half are unboundedly larger.
+    // * For generations, half are in [0, 2), half are unboundedly larger.
+
+    let x: f64 = rng.gen();
+    let id = -(1.0 - x).log2() * (size as f64);
+    let x: f64 = rng.gen();
+    let gen = -(1.0 - x).log2() * 2.0;
+
+    // this is not reliable, but we're internal so a hack is ok
+    let bits = ((gen as u64) << 32) | (id as u64);
+    let e = Entity::from_bits(bits);
+    assert_eq!(e.index(), id as u32);
+    assert_eq!(e.generation(), gen as u32);
+    e
+}
+
+fn entity_set_build_and_lookup(c: &mut Criterion) {
+    let mut group = c.benchmark_group("entity_hash");
+    for size in SIZES {
+        // Get some random-but-consistent entities to use for all the benches below.
+        let mut rng = ChaCha8Rng::seed_from_u64(size as u64);
+        let entities = Vec::from_iter(
+            std::iter::repeat_with(|| make_entity(&mut rng, size)).take(size),
+        );
+
+        group.throughput(Throughput::Elements(size as u64));
+        group.bench_function(
+            BenchmarkId::new("entity_set_build", size),
+            |bencher| {
+                bencher.iter_with_large_drop(|| EntityHashSet::from_iter(entities.iter().copied()));
+            },
+        );
+        group.bench_function(
+            BenchmarkId::new("entity_set_lookup_hit", size),
+            |bencher| {
+                let set = EntityHashSet::from_iter(entities.iter().copied());
+                bencher.iter(|| entities.iter().copied().filter(|e| set.contains(e)).count());
+            },
+        );
+        group.bench_function(
+            BenchmarkId::new("entity_set_lookup_miss_id", size),
+            |bencher| {
+                let set = EntityHashSet::from_iter(entities.iter().copied());
+                bencher.iter(|| entities.iter()
+                    .copied()
+                    .map(|e| Entity::from_bits(e.to_bits() + 1))
+                    .filter(|e| set.contains(e)).count());
+            },
+        );
+        group.bench_function(
+            BenchmarkId::new("entity_set_lookup_miss_gen", size),
+            |bencher| {
+                let set = EntityHashSet::from_iter(entities.iter().copied());
+                bencher.iter(|| entities.iter()
+                    .copied()
+                    .map(|e| Entity::from_bits(e.to_bits() + (1 << 32)))
+                    .filter(|e| set.contains(e)).count());
+            },
+        );
+    }
+}

crates/bevy_ecs/src/entity/mod.rs

@@ -115,12 +115,21 @@ type IdCursor = isize;
 /// [`EntityCommands`]: crate::system::EntityCommands
 /// [`Query::get`]: crate::system::Query::get
 /// [`World`]: crate::world::World
-#[derive(Clone, Copy, Eq, Ord, PartialEq, PartialOrd)]
+#[derive(Clone, Copy, Eq, Ord, PartialOrd)]
 pub struct Entity {
     generation: u32,
     index: u32,
 }
 
+// By not short-circuiting in comparisons, we get better codegen.
+// See <https://github.com/rust-lang/rust/issues/117800>
+impl PartialEq for Entity {
+    #[inline]
+    fn eq(&self, other: &Entity) -> bool {
+        (self.generation == other.generation) & (self.index == other.index)
+    }
+}
+
 impl Hash for Entity {
     #[inline]
     fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
@@ -917,4 +926,30 @@ mod tests {
         assert_eq!(next_entity.index(), entity.index());
         assert!(next_entity.generation > entity.generation + GENERATIONS);
     }
+
+    #[test]
+    fn entity_comparison() {
+        // This is intentionally testing `lt` and `ge` as separate functions.
+        #![allow(clippy::nonminimal_bool)]
+
+        assert!(Entity::new(123, 456) == Entity::new(123, 456));
+        assert!(Entity::new(123, 789) != Entity::new(123, 456));
+        assert!(Entity::new(123, 456) != Entity::new(123, 789));
+        assert!(Entity::new(123, 456) != Entity::new(456, 123));
+
+        // ordering is by generation then by index
+
+        assert!(Entity::new(123, 456) >= Entity::new(123, 456));
+        assert!(Entity::new(123, 456) <= Entity::new(123, 456));
+        assert!(!(Entity::new(123, 456) < Entity::new(123, 456)));
+        assert!(!(Entity::new(123, 456) > Entity::new(123, 456)));
+
+        assert!(Entity::new(9, 1) < Entity::new(1, 9));
+        assert!(Entity::new(1, 9) > Entity::new(9, 1));
+
+        assert!(Entity::new(1, 1) < Entity::new(2, 1));
+        assert!(Entity::new(1, 1) <= Entity::new(2, 1));
+        assert!(Entity::new(2, 2) > Entity::new(1, 2));
+        assert!(Entity::new(2, 2) >= Entity::new(1, 2));
+    }
 }