-
Notifications
You must be signed in to change notification settings - Fork 12.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[perf] Use Blake2 for the type-id hash #128597
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
[perf] Use Blake2 for the type-id hash and only for the `TypeId` hash, not as a general stable hasher. cc `@michaelwoerister`
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
💔 Test failed - checks-actions |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@bors try |
[perf] Use Blake2 for the type-id hash and only for the `TypeId` hash, not as a general stable hasher. cc `@michaelwoerister`
This comment has been minimized.
This comment has been minimized.
💔 Test failed - checks-actions |
|
||
// See https://stackoverflow.com/a/27952689 on why this function is | ||
// implemented this way. | ||
Fingerprint(p0.wrapping_mul(3).wrapping_add(p1), p2.wrapping_mul(3).wrapping_add(p3)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am pretty sure you can just use Fingerprint(p0, p1)
. Blake2s is proven to be indistinguishable from a random function according to https://eprint.iacr.org/2016/827. If a prefix of the hash was biased, it wouldn't be indistinguishable from a random function. Because of the lack of bias on the low bytes, there is no need to mix in the high bytes.
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
[perf] Use Blake2 for the type-id hash and only for the `TypeId` hash, not as a general stable hasher. cc `@michaelwoerister`
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (3a79c24): comparison URL. Overall result: ❌ regressions - BENCHMARK(S) FAILEDBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. @bors rollup=never ❗ ❗ ❗ ❗ ❗
❗ ❗ ❗ ❗ ❗ Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results (primary 5.3%, secondary -3.3%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 751.515s -> 753.185s (0.22%) |
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
[perf] Use Blake2 for the type-id hash and only for the `TypeId` hash, not as a general stable hasher. cc `@michaelwoerister`
@Urgau, before you invest too much time in this: I think it is preferable to go through (a possibly adapted version of) v0 mangling for type-id (and then potentially cryptographically hash that if constant space for type-ids is desirable). |
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (c2fdbf6): comparison URL. Overall result: ❌✅ regressions and improvements - ACTION NEEDEDBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)This benchmark run did not return any relevant results for this metric. CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 751.027s -> 754.153s (0.42%) |
and only for the
TypeId
hash, not as a general stable hasher.cc @michaelwoerister