ECS: Use fxhash in TypeIdMap. #1119
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
benchmarks I made is here: #1097 (comment) |
DJMcNab
reviewed
Dec 22, 2020
| @@ -20,6 +20,7 @@ trace = [] | |||
| bevy_tasks = { path = "../bevy_tasks", version = "0.4.0" } | |||
| bevy_utils = { path = "../bevy_utils", version = "0.4.0" } | |||
| bevy_ecs_macros = { path = "macros", version = "0.4.0" } | |||
| fxhash = "0.2" | |||
There was a problem hiding this comment.
Do we need to use fx_hash? In bevy_util we are already set up for using ahash.
There was a problem hiding this comment.
See reasoning #1119 (comment)
Also fxhash used by other dependencies so it needs to be compiled anyway.
| /// which hashbrown needs), there is no need to hash it again. Instead, this uses the much | ||
| /// faster no-op hash. | ||
| pub(crate) type TypeIdMap<V> = HashMap<TypeId, V, BuildHasherDefault<TypeIdHasher>>; | ||
| pub(crate) type TypeIdMap<V> = HashMap<TypeId, V, fxhash::FxBuildHasher>; |
There was a problem hiding this comment.
Can we leave a comment here mentioning why we aren't just using RandomState, and perhaps mentioning TypeIdHasher.
There was a problem hiding this comment.
perhaps mentioning TypeIdHasher
I don't think that there is a reason to mention deleted code in comments. It would just distract people unneccessary.
|
@DJMcNab I run benches in I don't know is Ahash better or FxHash, it seems that both have little effect (ahash performs little better in single threaded benches while fxhash in multithreaded). I think, differences here is just noise. Comparison between old TypeIdHash and fxhash. This is comparison between FxHash and Ahash: |
|
Last time I compared fxhash to ahash in the context of Bevy ECS, aHash came out on top. We use aHash everywhere in Bevy (see bevy_utils), largely as a result of those numbers ... which were never published publicly 😄 Unless we have a very good reason, I'd prefer to stay consistent everywhere / use what already works for us. If we can prove that fxhash is a measurable / significant perf improvement, I'm down to make the switch. But otherwise id rather just stick with what currently works. When it comes to benchmarks, id prefer it if we used less synthetic benchmarks because optimizers are weird. Lets use ecs_bench_suite to check for perf changes. See this discussion on the upstream hecs repo for the rationale behind the change and some significant perf improvements we measured (both in upstream hecs and bevy ecs) |
|
@cart @DJMcNab And old hash -> ahash fxhash -> ahash I looks like that fxhash has comparable speed as noop hasher and ahash clearly slower than fxhash and noop. Probable reason of this that ahash optimized for data chunked by 128 bits (its code uses u128 heavily). fxhash, in contrast, optimized to use |
Relying on TypeId being some hash internally isn't future-proof because there is no guarantee about internal layout or structure of TypeId. I benchmarked TypeId noop hasher vs fxhash and found that there is very little difference. Also fxhash is likely to be better supported because it is widely used in rustc itself. [Benchmarks of hashers](#1097) [Engine wide benchmarks](#1119 (comment))
DJMcNab
approved these changes
Dec 26, 2020
What is not right? |
|
The exact message which I sent, because that was about the byteorder crate instead of the fxhash crate. Because I knew the byteorder crate was the only dependency of fxhash, I checked that first. |
|
Awesome. Those numbers are pretty convincing. In a few days when I'm back at the computer I've benched on before, I'll run some benches on my side just to convince myself thoroughly. But if everything lines up this looks like a clear win (small perf boosts in some cases, reduced code complexity, and maybe the potential to remove ahash from the build tree) |
|
Alrighty I finally got around to testing this. Using fxhash for this case specifically is pretty clearly a win: However switching bevy_utils to fxhash is a pretty major downgrade. I think ahash is much better at hashing very small values (ex: u32 / u64), which is way more common.
So I'm inclined to merge this, but keep AHash as the "default bevy hasher". |
rparrett
pushed a commit
to rparrett/bevy
that referenced
this pull request
Jan 27, 2021
Relying on TypeId being some hash internally isn't future-proof because there is no guarantee about internal layout or structure of TypeId. I benchmarked TypeId noop hasher vs fxhash and found that there is very little difference. Also fxhash is likely to be better supported because it is widely used in rustc itself. [Benchmarks of hashers](bevyengine#1097) [Engine wide benchmarks](bevyengine#1119 (comment))
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Relying on TypeId being some hash internally isn't future-proof because there is no guarantee about internal layout or structure of TypeId. I benchmarked TypeId noop hasher vs fxhash and found that there is very little difference.
Also fxhash is likely to be better supported because it is widely used in rustc itself.