Skip to content

Hash up to 8 bytes at once with FxHasher #51019

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 29, 2018
Merged

Hash up to 8 bytes at once with FxHasher #51019

merged 1 commit into from
May 29, 2018

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented May 24, 2018

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 24, 2018
@kennytm
Copy link
Member

kennytm commented May 24, 2018

@bors try

Let's do a perf check. cc @Mark-Simulacrum

@bors
Copy link
Collaborator

bors commented May 24, 2018

⌛ Trying commit dfb4bf2 with merge e163e6c...

bors added a commit that referenced this pull request May 24, 2018
@bors
Auto merge of #51019 - Zoxc:hash-bytes, r=<try>
e163e6c 
Hash up to 8 bytes at once with FxHasher

r? @michaelwoerister
@kennytm kennytm added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 24, 2018
michaelwoerister
michaelwoerister reviewed May 24, 2018
View reviewed changes
src/librustc_data_structures/fx.rs Outdated
unsafe {
assert!(size_of::<usize>() <= 8);
while bytes.len() >= size_of::<usize>() {
self.add_to_hash(*(bytes.as_ptr() as *const usize));
Copy link
Member

Choose a reason for hiding this comment

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

This could cause unaligned reads.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@michaelwoerister
Copy link
Member

@kennytm Can we restart that try build with the latest version of the PR?

@kennytm
Copy link
Member

kennytm commented May 24, 2018 •
edited
Loading

@bors try

@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 May 24, 2018
@bors
Copy link
Collaborator

bors commented May 24, 2018

⌛ Trying commit dc73379 with merge a0e5769...

bors added a commit that referenced this pull request May 24, 2018
@bors
Auto merge of #51019 - Zoxc:hash-bytes, r=<try>
a0e5769 
Hash up to 8 bytes at once with FxHasher

r? @michaelwoerister
@bors
Copy link
Collaborator

bors commented May 24, 2018

☀️ Test successful - status-travis
State: approved= try=True

@michaelwoerister
Copy link
Member

Thanks, @kennytm!

@kennytm kennytm 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 May 24, 2018
@leoyvens
Copy link
Contributor

The hash implementation has been moved to an external crate, could we move this PR over to it's new home?

@Zoxc
Copy link
Contributor Author

Zoxc commented May 25, 2018

http://perf.rust-lang.org/compare.html?start=7426f5ccf7b362785a5abeb365674d3da3d4df2e&end=a0e57699c9f4e5c9c794898abebf55c0840b6abe&stat=wall-time

@michaelwoerister
Copy link
Member

Looks good to me. cc @nnethercote

@kennytm kennytm removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 25, 2018
@kennytm
Copy link
Member

kennytm commented May 25, 2018

@Zoxc As mentioned in #51019 (comment), the FxHasher implementation has been moved out to rustc-hash by #50937, we can no longer merge this PR as-is.

Could you instead:

  1. File this PR to rustc-hash
  2. After a new version of rustc-hash is published to crates.io, update the src/librustc_data_structure/Cargo.toml (if the version becomes to 1.1.0 or above) and src/Cargo.lock (cargo update -p rustc-hash).

@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 25, 2018
@Zoxc Zoxc mentioned this pull request May 27, 2018
Merged
@Zoxc
Copy link
Contributor Author

Zoxc commented May 27, 2018

I've opened a PR to rustc-hash: rust-lang/rustc-hash#1

@kennytm kennytm added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 27, 2018
@michaelwoerister
Copy link
Member

Version 1.0.1 of rustc-hash containing these changes is now published on crates.io.

@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels May 28, 2018
@Zoxc
Copy link
Contributor Author

Zoxc commented May 28, 2018

This now updates the rustc-hash version.

@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 28, 2018
@michaelwoerister
Copy link
Member

Thanks, @Zoxc!

@bors r+

@bors
Copy link
Collaborator

bors commented May 29, 2018

📌 Commit 7ebd4d6 has been approved by michaelwoerister

@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 May 29, 2018
@bors
Copy link
Collaborator

bors commented May 29, 2018

⌛ Testing commit 7ebd4d6 with merge 61f35e5...

bors added a commit that referenced this pull request May 29, 2018
@bors
Auto merge of #51019 - Zoxc:hash-bytes, r=michaelwoerister
61f35e5 
Hash up to 8 bytes at once with FxHasher

r? @michaelwoerister
@bors
Copy link
Collaborator

bors commented May 29, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing 61f35e5 to master...

@bors bors merged commit 7ebd4d6 into rust-lang:master May 29, 2018
This was referenced May 29, 2018
Merged
Merged
@Zoxc Zoxc deleted the hash-bytes branch May 29, 2018 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@michaelwoerister michaelwoerister michaelwoerister left review comments

Reviewers whose approvals may not affect merge requirements
Assignees

@michaelwoerister michaelwoerister

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Zoxc @kennytm @bors @michaelwoerister @leoyvens @rust-highfive