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

Use const generics in SipHasher128's short_write #93671

Merged
merged 1 commit into from
Feb 12, 2022

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Feb 5, 2022

This was proposed by @michaelwoerister here.
A few comments:

  1. I tried to pass &[u8; LEN] instead of [u8; LEN]. Locally, it resulted in small icount regressions (about 0.5 %). When passing by value, there were no regressions (and no improvements).
  2. I wonder if we should use to_ne_bytes() in SipHasher128 to keep it generic and only use to_le_bytes() in StableHasher. However, currently SipHasher128 is only used in StableHasher and the short_write method was private, so I couldn't use it directly from StableHasher. Using to_le() in the StableHasher was breaking this abstraction boundary before slightly.
debug_assert!(LEN <= 8);

This could be done at compile time, but actually I think that now we can remove this assert altogether.

r? @the8472

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 5, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 5, 2022
@Kobzol Kobzol force-pushed the stable-hash-const branch from 4535515 to 5fc2e56 Compare February 5, 2022 18:55
@the8472
Copy link
Member

the8472 commented Feb 5, 2022

debug_assert!(LEN <= 8);

This could be done at compile time, but actually I think that now we can remove this assert altogether.

Well, in principle this is covered by the spill capacity check but it's more explicit that way.

It's hot code and might impact compile time even though I don't really expect it to, so no rollup.
@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Feb 5, 2022

📌 Commit 5fc2e56 has been approved by the8472

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 5, 2022
@bors
Copy link
Contributor

bors commented Feb 7, 2022

⌛ Testing commit 5fc2e56 with merge 5c7c1a0f448ae46a5ef8becb90b398893372a851...

@bors
Copy link
Contributor

bors commented Feb 7, 2022

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 7, 2022
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@the8472
Copy link
Member

the8472 commented Feb 9, 2022

Looks like #93329

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 9, 2022
@bors
Copy link
Contributor

bors commented Feb 12, 2022

⌛ Testing commit 5fc2e56 with merge fc32303...

@bors
Copy link
Contributor

bors commented Feb 12, 2022

☀️ Test successful - checks-actions
Approved by: the8472
Pushing fc32303 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 12, 2022
@bors bors merged commit fc32303 into rust-lang:master Feb 12, 2022
@rustbot rustbot added this to the 1.60.0 milestone Feb 12, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (fc32303): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@Kobzol Kobzol deleted the stable-hash-const branch February 12, 2022 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants