Name component simplification

[eckz]

Objective

• While checking how Name component was built, it catch my attention how needlessly complex it was.
• It's storing a pre-computed hash internally, and it's only used for PartialEq for the case where both hashes are different, as it's assumed that both are Names are different.
  • This is what was bringing most complexity to the component since it forced custom trait implementations instead of directly using #[derive]

Solution

• Removing custom Debug, Hash, PartialOrd, PartialEq, Serialize and Deserialize. Substituting them with #[derive]
• Simplify set() and mutate::<F: FnOnce(&mut String)>() with a simpler to_mut()

Testing

• Did you test these changes? If so, how?
  • cargo run -p ci Locally

Perfomance testing:

I was curious if the performance would suffer or improve compared to the previous implementation, so I've created some benchmarks to compare the PartialEq implementations (since it was the only place where the internal hash was used).
It also tests how it would behave if a lot of Name's are inserted in a HashSet.

The benchmarks are not included in this PR, since I don't see really any value in benchmarking default PartialEq in the future.

The results are the following:

Which kind of shows that previous implementation is only better if somehow you create a ton of small Name's, and compare them where internal strings are relatively small.

Just to be clear, I don't think any of this benchmarks represents any real usage of the Name component, making the previous complex implementation harder to justify.

---

Migration Guide

• Name's mutate and set are replaced with a into_mut() that returns a mutable String.

[chescock]

Since Name derives Reflect, this will change the serialized format of scenes with names. Do we need a Migration Guide to mention that?

There's a serialized Name in load_scene_example.scn.ron that might need to be updated, but I haven't checked how it's used.

bevy/assets/scenes/load_scene_example.scn.ron

Line 10 in b039bf6

"bevy_ecs::name::Name": (

[alice-i-cecile]

Since Name derives Reflect, this will change the serialized format of scenes with names. Do we need a Migration Guide to mention that?

I would like to ensure that we don't change the serialized format: this is critical enough that a manual impl is worth it. Swapping over to S-Waiting-For-Author.

[eckz]

Interestingly enough, in current main, the scene example is failing.

I was surprised because the #derive(Serialize, Deserialize) should keep the previous manual implementation by using #[serde(transparent)], but the issue is that nobody updated load_scene_example.scn.ron.

I've updated load_scene_example.scn.ron with the new format (that this PR maintains).

[chescock]

Oh, sorry for the confusion! I forgot which things use Reflect and which use Serialize, and that file threw me off.

[mockersf]

the current Name was added in #1109 and it's main goal was to use a hashed value instead of the string

and not sure your benches are entirely good, at least for the equals/different variants, as they create the hashes in the bench. I think this should be part of the setup

[eckz]

I might be doing something wrong here, but I've tried to rewrite the benchmarks to not take in account the initial Name::new, and I came with these results

For reference my machine is
SystemInfo { os: "macOS 14.7 Sonoma", kernel: "23.6.0", cpu: "Apple M1 Pro", core_count: "10", memory: "32.0 GiB" }

Benchmarks are the following:

fn name_eq(c: &mut Criterion) {
    let mut group = c.benchmark_group("name_eq");
    group.warm_up_time(core::time::Duration::from_millis(500));
    group.measurement_time(core::time::Duration::from_secs(5));

    group.bench_function("short-different", |b| {
        let names: Vec<_> = (0..65536).map(|i| Name::new(format!("Component {i}"))).collect();
        b.iter(
            move || {
                let found = black_box({
                    let search_name = Name::new(String::from("Component that does not exist"));
                    names.iter().any(|n| black_box(n == &search_name))
                });
                assert!(!found);
            },
        )
    });

    group.bench_function("short-equals", |b| {
        let name: String = rand::thread_rng()
            .sample_iter(&rand::distributions::Alphanumeric)
            .take(12)
            .map(char::from)
            .collect();

        let names: Vec<_> = (0..65536).map(|_| Name::new(name.clone())).collect();

        b.iter(
            move || {
                let all_same = black_box({
                    let search_name = Name::new(name.clone());
                    names.iter().all(|n| black_box(n == &search_name))
                });
                assert!(all_same);
            },
        )
    });

    group.bench_function("long-different", |b| {
        let suffix: String = rand::thread_rng()
            .sample_iter(&rand::distributions::Alphanumeric)
            .take(512)
            .map(char::from)
            .collect();

        let names: Vec<_> = (0..65536).map(|i| Name::new(format!("Component {i} {suffix}"))).collect();

        b.iter(
            move || {
                let found = black_box({
                    let search_name = Name::new(String::from("Component that does not exist"));
                    names.iter().any(|n| black_box(n == &search_name))
                });
                assert!(!found);
            },
        )
    });

    group.bench_function("long-equals", |b| {
        let name: String = rand::thread_rng()
            .sample_iter(&rand::distributions::Alphanumeric)
            .take(1024)
            .map(char::from)
            .collect();

        let names: Vec<_> = (0..65536).map(|_| Name::new(name.clone())).collect();

        b.iter(
            move || {
                let all_same = black_box({
                    let search_name = Name::new(name.clone());
                    names.iter().all(|n| black_box(n == &search_name))
                });
                assert!(all_same);
            },
        )
    });

    group.finish();
}