Skip to content

Hash VecDeque in its slice parts #31224

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

Merged
merged 1 commit into from
Jan 27, 2016
Merged

Hash VecDeque in its slice parts #31224

merged 1 commit into from
Jan 27, 2016

Conversation

bluss
Copy link
Member

@bluss bluss commented Jan 26, 2016

Hash VecDeque in its slice parts

Use .as_slices() for a more efficient code path in VecDeque's Hash impl.

This still hashes the elements in the same order.

Before/after timing of VecDeque hashing 1024 elements of u8 and
u64 shows that the vecdeque now can match the Vec
(test_hashing_vec_of_u64 is the Vec run).

before

test test_hashing_u64        ... bench:  14,031 ns/iter (+/- 236) = 583 MB/s
test test_hashing_u8         ... bench:   7,887 ns/iter (+/- 65) = 129 MB/s
test test_hashing_vec_of_u64 ... bench:   6,578 ns/iter (+/- 76) = 1245 MB/s

after

test test_hashing_u64        ... bench:   6,495 ns/iter (+/- 52) = 1261 MB/s
test test_hashing_u8         ... bench:     851 ns/iter (+/- 16) = 1203 MB/s
test test_hashing_vec_of_u64 ... bench:   6,499 ns/iter (+/- 59) = 1260 MB/s

@rust-highfive
Copy link
Contributor

r? @alexcrichton

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

@bluss
Copy link
Member Author

bluss commented Jan 26, 2016

@Gankra
Copy link
Contributor

Gankra commented Jan 26, 2016

@_@

DAT PERF

💯

@Gankra
Copy link
Contributor

Gankra commented Jan 26, 2016

Can you add a test that verifies that different splits yield the same hash? Something like pushpushpush == pushpushpoppushpush

@Gankra
Copy link
Contributor

Gankra commented Jan 26, 2016

Also: dang! Should we be doing this for eq/ord, etc?

@bluss
Copy link
Member Author

bluss commented Jan 26, 2016

I'm working on this for eq. We should absolutely use slices everywhere we can (llvm likes them, see #30805).

A test to verify same hash, same eq for different splits sounds good. Will do.

@bluss bluss force-pushed the deque-hashing branch 3 times, most recently from 89a36a2 to e62ebb9 Compare January 26, 2016 22:14
@bluss
Copy link
Member Author

bluss commented Jan 26, 2016

I added a test!

for elt in &mut ring {
*elt -= 1;
}
ring.push_back(first + len - 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

first == 0, so this is just len - 1?

Copy link
Member Author

Choose a reason for hiding this comment

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

smart. yes.. why think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm fixing it.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

Use .as_slices() for a more efficient code path in VecDeque's Hash impl.

This still hashes the elements in the same order.

Before/after timing of VecDeque hashing 1024 elements of u8 and
u64 shows that the vecdeque now can match the Vec
(test_hashing_vec_of_u64 is the Vec run).

before

test test_hashing_u64        ... bench:  14,031 ns/iter (+/- 236) = 583 MB/s
test test_hashing_u8         ... bench:   7,887 ns/iter (+/- 65) = 129 MB/s
test test_hashing_vec_of_u64 ... bench:   6,578 ns/iter (+/- 76) = 1245 MB/s

after

running 5 tests
test test_hashing_u64        ... bench:   6,495 ns/iter (+/- 52) = 1261 MB/s
test test_hashing_u8         ... bench:     851 ns/iter (+/- 16) = 1203 MB/s
test test_hashing_vec_of_u64 ... bench:   6,499 ns/iter (+/- 59) = 1260 MB/s
@Gankra
Copy link
Contributor

Gankra commented Jan 26, 2016

@bors r+

@bors
Copy link
Collaborator

bors commented Jan 26, 2016

📌 Commit d3174ce has been approved by Gankro

@bluss
Copy link
Member Author

bluss commented Jan 26, 2016

Thanks for the feedback Gankro! Together we will hash these bytes, faster than ever.

@bors
Copy link
Collaborator

bors commented Jan 27, 2016

⌛ Testing commit d3174ce with merge b2f4c5c...

bors added a commit that referenced this pull request Jan 27, 2016
Hash VecDeque in its slice parts

Use .as_slices() for a more efficient code path in VecDeque's Hash impl.

This still hashes the elements in the same order.

Before/after timing of VecDeque hashing 1024 elements of u8 and
u64 shows that the vecdeque now can match the Vec
(test_hashing_vec_of_u64 is the Vec run).

```
before

test test_hashing_u64        ... bench:  14,031 ns/iter (+/- 236) = 583 MB/s
test test_hashing_u8         ... bench:   7,887 ns/iter (+/- 65) = 129 MB/s
test test_hashing_vec_of_u64 ... bench:   6,578 ns/iter (+/- 76) = 1245 MB/s

after

running 5 tests
test test_hashing_u64        ... bench:   6,495 ns/iter (+/- 52) = 1261 MB/s
test test_hashing_u8         ... bench:     851 ns/iter (+/- 16) = 1203 MB/s
test test_hashing_vec_of_u64 ... bench:   6,499 ns/iter (+/- 59) = 1260 MB/s
```
@bors bors merged commit d3174ce into rust-lang:master Jan 27, 2016
@bluss bluss deleted the deque-hashing branch January 27, 2016 21:03
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.

5 participants