Skip rehashing TypeIds

[SpecificProtagonist]

Objective

TypeId contains a high-quality hash. Whenever a lookup based on a TypeId is performed (e.g. to insert/remove components), the hash is run through a second hash function. This is unnecessary.

Solution

Skip re-hashing TypeIds.

In my testing, this improves lookup performance consistently by 10%-15% (of course, the lookup is only a small part of e.g. a bundle insertion).

[NthTensor]

Would it be better to panic if the behavior changes, instead of silently switching to a low quality fallback?

I think there might be a few other places where this would be useful. Maybe this should go in 'bevy_util' instead?

[doonv]

Depending on how likely this is to occur it might be more beneficial to panic or to emit a warning message if this happens.

[SpecificProtagonist]

I'm pretty ambivalent about what's best here. An unimplemented!("explanation here") would probably be fine. A compile time warning would be ideal, but I don't see a reasonable way to do that.

[alice-i-cecile]

I think I'd prefer unimplemented here as well.

[SpecificProtagonist]

Changed to unimplemented.

[doonv]

I don't think unimplemented! is the right panic to use here. If this will never get called then unreachable! seems like the better choice.

[SpecificProtagonist]

I think there might be a few other places where this would be useful. Maybe this should go in 'bevy_util' instead?

Possibly, but that's orthogonal to this PR and can just be a followup PR.

[ghost]

I think there might be a few other places where this would be useful. Maybe this should go in 'bevy_util' instead?

+1

[alice-i-cecile]

Yeah I'm fine to move this later if we find downstream consumers (perhaps bevy_reflect).

[NthTensor]

... if we find upstream consumers ...

I will probably make a follow up to this if #11228 is approved. It makes heavy use of type id hashmaps.

[orzogc]

This PR relies on Rust std/core lib's internal implement. IMO we should have a last resort when TypeId's hashing behaviour changed.
Maybe we could use build.rs to check whether TypeId only hashes itself as u64 once and set cfg to switch hasher at compile-time.

[SpecificProtagonist]

This PR relies on Rust std/core lib's internal implement. IMO we should have a last resort when TypeId's hashing behaviour changed.

Note that Bevy isn't the only crate that relies on the write_u64 call. And from the discussion about the current implementation of TypeId:

The use of ::hash to extract a sufficiently hashed/mixed u64 identifier through Hasher::hash_u64 is an informally pseudo-blessed pattern for this. Merging this PR still doesn't officially endorse this, but it does add to the implicit acknowledgement that this is a legitimate pattern, since we're deliberately not breaking it.

The libs team deliberately does not endorse doing this, but as they're aware of this in the unlikely (other Hash impls were already discussed for this) case the Hash impl changes, there should be another crater run (additionally, bevy_ecs is tested under nightly miri).

Maybe we could use build.rs to check whether TypeId only hashes itself as u64 once and set cfg to switch hasher at compile-time.

I'm not sure that's worth it, build.rs is stuff can be a bit messy. An alternative I'd be happy with would be to add back in the write implementation from before I changed to a panic on request (or copy FxHasher's write) and add a test that checks that it isn't called 🙂

[alice-i-cecile]

An alternative I'd be happy with would be to add back in the write implementation from before I changed to a panic on request (or copy FxHasher's write) and add a test that checks that it isn't called 🙂

This seems like a good path forward.