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

libcollection : Switches from uint to u32 in BitV and BitVSet #18018

Merged
merged 1 commit into from
Oct 15, 2014

Conversation

gamazeps
Copy link
Contributor

Closes #16736
linked to #18009

@Gankra
Copy link
Contributor

Gankra commented Oct 13, 2014

Did you run the benches (PLEASE_BENCH=1) on this? I'd be interested to see if there were perf consequences on 64-bit platforms.

CC @apoelstra

@gamazeps
Copy link
Contributor Author

Yup, here are the result with u32

test bitv::tests::bench_bitv_big_iter ... bench: 17185 ns/iter (+/- 634)
test bitv::tests::bench_bitv_big_union ... bench: 1017 ns/iter (+/- 22)
test bitv::tests::bench_bitv_set_big_fixed ... bench: 681 ns/iter (+/- 5)
test bitv::tests::bench_bitv_set_big_variable ... bench: 1694 ns/iter (+/- 25)
test bitv::tests::bench_bitv_set_small ... bench: 643 ns/iter (+/- 11)
test bitv::tests::bench_bitv_small_iter ... bench: 474 ns/iter (+/- 23)
test bitv::tests::bench_bitvset_big ... bench: 993 ns/iter (+/- 5)
test bitv::tests::bench_bitvset_iter ... bench: 25473 ns/iter (+/- 726)
test bitv::tests::bench_bitvset_small ... bench: 989 ns/iter (+/- 14)
test bitv::tests::bench_uint_small ... bench: 572 ns/iter (+/- 7)

and the results with uint
test bitv::tests::bench_bitv_big_iter ... bench: 17347 ns/iter (+/- 650)
test bitv::tests::bench_bitv_big_union ... bench: 519 ns/iter (+/- 8)
test bitv::tests::bench_bitv_set_big_fixed ... bench: 685 ns/iter (+/- 10)
test bitv::tests::bench_bitv_set_big_variable ... bench: 1713 ns/iter (+/- 26)
test bitv::tests::bench_bitv_set_small ... bench: 674 ns/iter (+/- 11)
test bitv::tests::bench_bitv_small_iter ... bench: 782 ns/iter (+/- 22)
test bitv::tests::bench_bitvset_big ... bench: 1069 ns/iter (+/- 10)
test bitv::tests::bench_bitvset_iter ... bench: 40832 ns/iter (+/- 7077)
test bitv::tests::bench_bitvset_small ... bench: 1065 ns/iter (+/- 30)
test bitv::tests::bench_uint_small ... bench: 602 ns/iter (+/- 5)

u32 are faster for everything but unions, for which they are much slower.
I think it's a good tradeoff, what's your opinion on it ?

cc @thestinger @aturon

@Gankra
Copy link
Contributor

Gankra commented Oct 14, 2014

Looks good to me! Thanks a bunch!

@gamazeps
Copy link
Contributor Author

r? @aturon

bors added a commit that referenced this pull request Oct 14, 2014
@bors bors closed this Oct 15, 2014
@bors bors merged commit 1ef5e38 into rust-lang:master Oct 15, 2014
bors added a commit that referenced this pull request Oct 17, 2014
I was going to write some doc in order to remove the #[allow(missing_doc)] but there was actually none missing.
I also removed a warning i didn't see in my last commit  #18018
Linked to #18009
@gamazeps gamazeps deleted the isuue16736 branch October 17, 2014 11:43
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.

Bitv uses architecture-sized uints for backing storage
4 participants