Skip to content

/rust

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

incr.comp.: Use 128bit SipHash for fingerprinting #45319

Merged
merged 4 commits into from Oct 20, 2017

Conversation

Reviewers

@nikomatsakis nikomatsakis

Assignees

@nikomatsakis nikomatsakis

Labels
S-waiting-on-bors
Projects
None yet
Milestone
No milestone
7 participants
@michaelwoerister @rust-highfive @bors @Mark-Simulacrum @nikomatsakis @kennytm @pnkfelix
@michaelwoerister
Copy link
Contributor

michaelwoerister commented Oct 16, 2017 •
edited

This PR switches incr. comp. result fingerprinting from 128 bit BLAKE2 to 128 bit SipHash. When we started using BLAKE2 for fingerprinting, the 128 bit version of SipHash was still experimental. Now that it isn't anymore we should be able to get a nice performance boost without significantly increasing collision probability.

I'm going to start a try-build for this, so we can gauge the performance impact before merging (hence the WIP in the title).

EDIT: Performance improvements look as expected. Tests seem to be passing.

Fixes #41215.

@rust-highfive rust-highfive assigned pnkfelix Oct 16, 2017

@rust-highfive

This comment has been minimized.

Sign in to view
Copy link
Collaborator

rust-highfive commented Oct 16, 2017

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)
@michaelwoerister

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

michaelwoerister commented Oct 16, 2017

@bors try
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Oct 16, 2017

⌛️ Trying commit 10e289e with merge 232a35e...

bors added a commit that referenced this pull request Oct 16, 2017

@bors
Auto merge of #45319 - michaelwoerister:use-128bit-siphash, r=<try> 
[WIP] incr.comp.: Use 128bit SipHash for fingerprinting

This PR switches incr. comp. result fingerprinting from 128 bit BLAKE2 to 128 bit SipHash. When we started using BLAKE2 for fingerprinting, the 128 bit version of SipHash was still experimental. Now that it isn't anymore we should be able to get a nice performance boost without significantly increasing collision probability.

I'm going to start a try-build for this, so we can gauge the performance impact before merging (hence the `WIP` in the title).
Loading status checks…
232a35e
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Oct 16, 2017

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

@kennytm kennytm added the S-waiting-on-author label Oct 16, 2017

@michaelwoerister

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

michaelwoerister commented Oct 16, 2017

Thanks @Mark-Simulacrum!

Yes this looks pretty much like what I expected after the previous benchmarks. I'll go fix those mir-opt tests.

@michaelwoerister michaelwoerister changed the title [WIP] incr.comp.: Use 128bit SipHash for fingerprinting incr.comp.: Use 128bit SipHash for fingerprinting Oct 16, 2017

@michaelwoerister

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

michaelwoerister commented Oct 16, 2017

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned pnkfelix Oct 16, 2017

@nikomatsakis

nikomatsakis approved these changes Oct 16, 2017
View changes

@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Oct 16, 2017

@bors r+
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Oct 16, 2017

📌 Commit 27b6c91 has been approved by nikomatsakis

@michaelwoerister michaelwoerister added S-waiting-on-bors and removed S-waiting-on-author labels Oct 17, 2017

@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Oct 19, 2017

⌛️ Testing commit 27b6c91 with merge 33ac6b8...

bors added a commit that referenced this pull request Oct 19, 2017

@bors
Auto merge of #45319 - michaelwoerister:use-128bit-siphash, r=nikomat… 
…sakis

incr.comp.: Use 128bit SipHash for fingerprinting

This PR switches incr. comp. result fingerprinting from 128 bit BLAKE2 to 128 bit SipHash. When we started using BLAKE2 for fingerprinting, the 128 bit version of SipHash was still experimental. Now that it isn't anymore we should be able to get a nice performance boost without significantly increasing collision probability.

~~I'm going to start a try-build for this, so we can gauge the performance impact before merging (hence the `WIP` in the title).~~

EDIT: Performance improvements look as expected. Tests seem to be passing.

Fixes #41215.
Loading status checks…
33ac6b8
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Oct 19, 2017

💔 Test failed - status-travis
@kennytm

This comment has been minimized.

Sign in to view
Copy link
Member

kennytm commented Oct 19, 2017

@bors retry Travis macOS 6.5-hour timeout.
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Oct 20, 2017

⌛️ Testing commit 27b6c91 with merge c0956ff...

bors added a commit that referenced this pull request Oct 20, 2017

@bors
Auto merge of #45319 - michaelwoerister:use-128bit-siphash, r=nikomat… 
…sakis

incr.comp.: Use 128bit SipHash for fingerprinting

This PR switches incr. comp. result fingerprinting from 128 bit BLAKE2 to 128 bit SipHash. When we started using BLAKE2 for fingerprinting, the 128 bit version of SipHash was still experimental. Now that it isn't anymore we should be able to get a nice performance boost without significantly increasing collision probability.

~~I'm going to start a try-build for this, so we can gauge the performance impact before merging (hence the `WIP` in the title).~~

EDIT: Performance improvements look as expected. Tests seem to be passing.

Fixes #41215.
Loading status checks…
c0956ff
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Oct 20, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing c0956ff to master...

@bors bors merged commit 27b6c91 into rust-lang:master Oct 20, 2017
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@Xanewok Xanewok referenced this pull request Oct 23, 2017

Merged

Use 128 bit instead of Symbol for crate disambiguator #45476

bors added a commit that referenced this pull request Oct 25, 2017

@bors
Auto merge of #45476 - Xanewok:fingerprint-disambiguator, r=michaelwo… 
…erister

Use 128 bit instead of Symbol for crate disambiguator

As discussed on gitter, this changes `crate_disambiguator` from Strings to what they are represented as, a 128 bit number.

There's also one bit I think also needs to change, but wasn't 100% sure how: [create_root_def](https://github.com/rust-lang/rust/blob/f338dba29705e144bad8b2a675284538dd514896/src/librustc/hir/map/definitions.rs#L468-L482). Should I change `DefKey::root_parent_stable_hash` to accept `Fingerprint` as crate_disambiguator to quickly combine the hash of `crate_name` with the new 128 bit hash instead of a string for a disambiguator?

r? @michaelwoerister

EDIT: Are those 3 tests `mir-opt` failing, because the hash is different, because we calculate it a little bit differently (storing directly instead of hashing the hex-string representation)? Should it be updated like in #45319?
Loading status checks…
f764eaf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.