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

incr. comp.: Use more bits for DefPath hashes #42082

Merged
merged 2 commits into from
May 19, 2017

Conversation

michaelwoerister
Copy link
Member

Use 128 instead of 64 bits for DefPath hashes, like we do for everything else. Collision probability is unnecessarily high with 64 bits.

Also change the representation of ich::Fingerprint from Fingerprint([u8; 16]) to Fingerprint(u64, u64) which is better for hashers like FxHasher.

@rust-highfive
Copy link
Collaborator

r? @eddyb

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

@eddyb
Copy link
Member

eddyb commented May 18, 2017

@bors r+

@bors
Copy link
Contributor

bors commented May 18, 2017

📌 Commit 4549423 has been approved by eddyb

@alexcrichton alexcrichton added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 18, 2017
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 18, 2017
…hes, r=eddyb

incr. comp.: Use more bits for DefPath hashes

Use 128 instead of 64 bits for DefPath hashes, like we do for everything else. Collision probability is unnecessarily high with 64 bits.

Also change the representation of `ich::Fingerprint` from `Fingerprint([u8; 16])` to `Fingerprint(u64, u64)` which is better for hashers like `FxHasher`.
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 18, 2017
…hes, r=eddyb

incr. comp.: Use more bits for DefPath hashes

Use 128 instead of 64 bits for DefPath hashes, like we do for everything else. Collision probability is unnecessarily high with 64 bits.

Also change the representation of `ich::Fingerprint` from `Fingerprint([u8; 16])` to `Fingerprint(u64, u64)` which is better for hashers like `FxHasher`.
@bors
Copy link
Contributor

bors commented May 19, 2017

⌛ Testing commit 4549423 with merge 92b0b81...

@bors
Copy link
Contributor

bors commented May 19, 2017

💔 Test failed - status-appveyor

@alexcrichton
Copy link
Member

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 19, 2017
…hes, r=eddyb

incr. comp.: Use more bits for DefPath hashes

Use 128 instead of 64 bits for DefPath hashes, like we do for everything else. Collision probability is unnecessarily high with 64 bits.

Also change the representation of `ich::Fingerprint` from `Fingerprint([u8; 16])` to `Fingerprint(u64, u64)` which is better for hashers like `FxHasher`.
bors added a commit that referenced this pull request May 19, 2017
@bors bors merged commit 4549423 into rust-lang:master May 19, 2017
@pthariensflame
Copy link
Contributor

Out of curiosity, why didn't this just use u128?

@michaelwoerister
Copy link
Member Author

@pthariensflame Mostly because our hasher works with usize, so using u128 would have brought no performance benefit while possibly introducing stricter alignment requirements.

@eddyb
Copy link
Member

eddyb commented May 29, 2017

What are the platforms where i128 has higher aligment than i64?

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jun 1, 2017
…oerister

Don't byteswap Fingerprints when encoding

Byteswapping Fingerprints when encoding is unnessesary and breaks if the Fingerprint is later decoded on a machine with different endianness to the one it was encoded on.

Fixes rust-lang#42239

This PR fixes a regression caused by rust-lang#42082. @michaelwoerister
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jun 1, 2017
…oerister

Don't byteswap Fingerprints when encoding

Byteswapping Fingerprints when encoding is unnessesary and breaks if the Fingerprint is later decoded on a machine with different endianness to the one it was encoded on.

Fixes rust-lang#42239

This PR fixes a regression caused by rust-lang#42082. @michaelwoerister
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jun 2, 2017
…oerister

Don't byteswap Fingerprints when encoding

Byteswapping Fingerprints when encoding is unnessesary and breaks if the Fingerprint is later decoded on a machine with different endianness to the one it was encoded on.

Fixes rust-lang#42239

This PR fixes a regression caused by rust-lang#42082. @michaelwoerister
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jun 2, 2017
…oerister

Don't byteswap Fingerprints when encoding

Byteswapping Fingerprints when encoding is unnessesary and breaks if the Fingerprint is later decoded on a machine with different endianness to the one it was encoded on.

Fixes rust-lang#42239

This PR fixes a regression caused by rust-lang#42082. @michaelwoerister
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants