Skip to content
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

Introduce multi hasher support #8

Merged
merged 5 commits into from
Jul 9, 2024

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Jul 3, 2024

This PR introduce multi Hasher support in our StableHasher by adding a new trait ExtendedHasher that extended the hasher trait with support for short writes and custom hash type and hook it up with StableHasher.

For brevity here is the API for ExtendedHasher:

/// Extended the [`Hasher`] trait for use with [`StableHasher`].
///
/// It permits returning an arbitrary type as the [`Self::Hash`] type
/// contrary to the [`Hasher`] trait which can only return `u64`. This
/// is useful when the hasher uses a different representation.
pub trait ExtendedHasher: Hasher {
    /// Type returned by the hasher.
    type Hash;

    /// Optimized version of [`Hasher::write`] but for small write.
    fn short_write<const LEN: usize>(&mut self, bytes: [u8; LEN]);

    /// Finalization method of the hasher to return the [`Hash`].
    fn finish(self) -> Self::Hash;
}

Unresolved questions:

  • Should we expose SipHasher128? or just exposing StableSipHasher128 is enough?
  • Should StableHasher::new exist and call Default::default? or should it take a hasher as input?
    • Added StableHasher::with_hasher to handle this.

r? @michaelwoerister
Fixes #5

src/lib.rs Outdated Show resolved Hide resolved
@michaelwoerister
Copy link
Member

Thanks, @Urgau!

Before we merge this I think we should make rustc actually use this crate instead of the one in rustc_data_structures. What do you think? It's making me a bit uneasy to keep making changes here without the feedback of them being integrated in the compiler, with respect to performance and also API design.

@michaelwoerister michaelwoerister self-assigned this Jul 8, 2024
@Urgau
Copy link
Member Author

Urgau commented Jul 8, 2024

I'm maintaining a branch on my rustc fork with the integration of this crate, Urgau:rustc-stable-hash.

I have opened rust-lang/rust#127479 so we can test the perf with those changes included, as well as seeing exactly the changes required to the compiler, which are minimal (nothing major).

I think if the perf are acceptable we should merge this PR, make a release and merge the rustc PR to make use of the crate.

@Urgau
Copy link
Member Author

Urgau commented Jul 8, 2024

@michaelwoerister perf are neutral on the rustc side. I think we can merge this PR and prepare the release.

Copy link
Member

@michaelwoerister michaelwoerister left a comment

Choose a reason for hiding this comment

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

Added some final comments. Looks great overall!

src/stable_hasher.rs Outdated Show resolved Hide resolved
src/stable_hasher.rs Outdated Show resolved Hide resolved
src/stable_hasher.rs Outdated Show resolved Hide resolved
src/stable_hasher.rs Outdated Show resolved Hide resolved
@michaelwoerister michaelwoerister merged commit 3805516 into rust-lang:main Jul 9, 2024
7 checks passed
@weihanglo
Copy link
Member

Thanks people for making this happen!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider supporting multiple hash functions
3 participants