Skip to content

Commit 56e3045

Browse files
committed
FIX: shuffled: efficiency improvements
1. There was a bug. When we approximate, for example 10 with power of two - we got 16 (2^4). So register size must be 4. But instead register if size 5 was used. It's not efficient. 2. Even we have efficient std::distance and operator+(int n) functions dumb_advanced and dumb_size doesn't uses them, so they was replaced with std::advance and std::distance accordingly
1 parent 2dffbd9 commit 56e3045

File tree

2 files changed

+21
-19
lines changed

2 files changed

+21
-19
lines changed

shuffled.hpp

+8-6
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,8 @@
1010
* implemented through advance of the first (begin) iterator of the
1111
* container, so random access iterator is desirable. In the constructor
1212
* we have to calculate the total size of the container, so
13-
* dumb_distance will be used.*/
13+
* std::distance will be used. User can define more effective
14+
* std::distance for the container.*/
1415

1516
namespace iter {
1617
namespace impl {
@@ -33,7 +34,7 @@ namespace iter {
3334
impl::ShuffledView<Container, Distance> shuffled(Container&&, int seed = 1);
3435
}
3536

36-
// power of 2 approximation
37+
// power of 2 approximation (val < pow(2, get_approx(val)+1))
3738
uint16_t iter::impl::lfsr::get_approx(uint64_t val) {
3839
if (val == 0)
3940
return 0;
@@ -49,7 +50,7 @@ uint16_t iter::impl::lfsr::get_approx(uint64_t val) {
4950
break;
5051
}
5152
}
52-
return pow2_approx + 1;
53+
return pow2_approx;
5354
}
5455

5556
uint64_t iter::impl::lfsr::shift(uint64_t reg, uint8_t reg_size) {
@@ -77,12 +78,13 @@ class iter::impl::ShuffledView {
7778
using IterDeref = typename std::remove_reference<iterator_deref<Container>>;
7879
ShuffledView(ShuffledView&&) : size(0) {};
7980
ShuffledView(Container&& container, int seed)
80-
: size(dumb_size(container)), size_approx(lfsr::get_approx(size)),
81+
: size(std::distance(std::begin(container), std::end(container))),
82+
size_approx(lfsr::get_approx(size)),
8183
in_begin(std::begin(container)), seed(seed) {
8284
if (size > 0)
8385
{
8486
uint64_t mask = 0xFFFFFFFFFFFFFFFFULL;
85-
mask >> (64-size_approx);
87+
mask = (mask >> (64-size_approx));
8688
this->seed = seed & mask;
8789
this->seed = lfsr::shift(this->seed, size_approx);
8890
while(this->seed >= size)
@@ -133,7 +135,7 @@ class iter::impl::ShuffledView {
133135

134136
auto operator*() -> decltype(*copy) {
135137
copy = owner->in_begin;
136-
dumb_advance(copy, static_cast<Distance>(state-1));
138+
std::advance(copy, static_cast<Distance>(state-1));
137139
return *copy;
138140
}
139141

test/test_shuffled.cpp

+13-13
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ TEST_CASE("shuffled: iterates through a vector in shuffled order", "[shuffled]")
1919
Vec ns = {4, 0, 5, 1, 6, 7, 9, 3, 2, 8};
2020
auto s = shuffled(ns);
2121
Vec v(std::begin(s), std::end(s));
22-
Vec vc = {2, 7, 5, 9, 8, 6, 3, 1, 0, 4};
22+
Vec vc = {2, 9, 8, 6, 7, 5, 3, 1, 0, 4};
2323
REQUIRE(v == vc);
2424
}
2525

@@ -41,27 +41,27 @@ TEST_CASE("shuffled: restore iteration through a vector in shuffled order "
4141

4242
// overflow - same as default
4343
Vec v10(s.restore(10), std::end(s));
44-
Vec vc10 = {2, 7, 5, 9, 8, 6, 3, 1, 0, 4};
44+
Vec vc10 = {2, 9, 8, 6, 7, 5, 3, 1, 0, 4};
4545
REQUIRE(v10 == vc10);
4646

47-
Vec v1(s.restore(5), std::end(s));
48-
Vec vc1 = {7, 5, 9, 8, 6, 3, 1, 0, 4};
47+
Vec v1(s.restore(6), std::end(s));
48+
Vec vc1 = {9, 8, 6, 7, 5, 3, 1, 0, 4};
4949
REQUIRE(v1 == vc1);
5050

51-
Vec v2(s.restore(2), std::end(s));
52-
Vec vc2 = {5, 9, 8, 6, 3, 1, 0, 4};
51+
Vec v2(s.restore(9), std::end(s));
52+
Vec vc2 = {8, 6, 7, 5, 3, 1, 0, 4};
5353
REQUIRE(v2 == vc2);
5454

55-
Vec v3(s.restore(6), std::end(s));
56-
Vec vc3 = {9, 8, 6, 3, 1, 0, 4};
55+
Vec v3(s.restore(4), std::end(s));
56+
Vec vc3 = {6, 7, 5, 3, 1, 0, 4};
5757
REQUIRE(v3 == vc3);
5858

59-
Vec v4(s.restore(9), std::end(s));
60-
Vec vc4 = {8, 6, 3, 1, 0, 4};
59+
Vec v4(s.restore(5), std::end(s));
60+
Vec vc4 = {7, 5, 3, 1, 0, 4};
6161
REQUIRE(v4 == vc4);
6262

63-
Vec v5(s.restore(4), std::end(s));
64-
Vec vc5 = {6, 3, 1, 0, 4};
63+
Vec v5(s.restore(2), std::end(s));
64+
Vec vc5 = {5, 3, 1, 0, 4};
6565
REQUIRE(v5 == vc5);
6666

6767
Vec v6(s.restore(7), std::end(s));
@@ -96,7 +96,7 @@ TEST_CASE("shuffled: iterates through a vector in shuffled order with non"
9696
Vec ns = {4, 0, 5, 1, 6, 7, 9, 3, 2, 8};
9797
auto s = shuffled(ns, 1234);
9898
Vec v(std::begin(s), std::end(s));
99-
Vec vc = {9, 8, 6, 3, 1, 0, 4, 2, 7, 5};
99+
Vec vc = {4, 2, 9, 8, 6, 7, 5, 3, 1, 0};
100100
REQUIRE(v == vc);
101101
}
102102

0 commit comments

Comments
 (0)