Skip to content

Hashing improvements#532

Merged
lucab merged 4 commits intotikv:masterfrom
hoxxep:hashing
Mar 21, 2025
Merged

Hashing improvements#532
lucab merged 4 commits intotikv:masterfrom
hoxxep:hashing

Conversation

@hoxxep
Copy link
Contributor

@hoxxep hoxxep commented Dec 12, 2024

As part of investigating whether rust-prometheus would benefit from rapidhash, I noticed a few improvements that could be made:

  • The MetricVecCore::children hashmap uses keys that are hash values, so introducing NoHashHasher to avoid re-hashing this hash value improves performance.
  • Allow with and get_metric_with to accept hashmaps with custom hashers.
  • Introduce criterion's black_box in some hashmaps to reduce over-optimisation in microbenchmarks. Could be applied to further benchmarks.

Some of the counter.rs benchmarks are including hashmap initialisation and destruction. They are also using fixed values and small keys/values, which means benchmarks were unclear over whether FNV or rapidhash are beneficial (hash clashes, etc). Slightly longer key/value lengths are likely to benefit from rapidhash.

@ti-chi-bot
Copy link

ti-chi-bot bot commented Dec 12, 2024

Welcome @hoxxep! It looks like this is your first PR to tikv/rust-prometheus 🎉

Signed-off-by: Liam Gray <hoxxep@gmail.com>
Copy link
Member

@lucab lucab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!
I'd be happy to land something like this but I'm slightly wary about the state of https://github.com/paritytech/nohash-hasher.
Would you be open to drop nohash-hasher and directly implement a NoHasherU64 in here?

Signed-off-by: Liam Gray <hoxxep@gmail.com>
@hoxxep
Copy link
Contributor Author

hoxxep commented Mar 20, 2025

@lucab sure! I've removed the nohash-hasher dependency and had a sparring session with the DCO signoff bot, but that should be done now 👍

Copy link
Member

@lucab lucab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This generally looks fine, I only have a minor question.
It also needs a final cargo fmt --all in order to pass CI all checks.

src/nohash.rs Outdated
Comment on lines 5 to 7
pub struct NoHashHasher(u64);

pub type BuildNoHashHasher = BuildHasherDefault<NoHashHasher>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these two can be more properly restricted to pub(crate), right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. They're technically never re-exported, but I've adjusted to pub(crate) per your request so that it's explicit to the reader.

Signed-off-by: Liam Gray <hoxxep@gmail.com>
Copy link
Member

@lucab lucab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lucab lucab merged commit 26e46ec into tikv:master Mar 21, 2025
8 checks passed
@lucab lucab mentioned this pull request Mar 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants