Skip to content

Commit

Permalink
Bug correction, set a moved hash map/set in a valid state so that it …
Browse files Browse the repository at this point in the history 
…can still be used even after a move.
  • Loading branch information
@Tessil
Tessil authored Jul 22, 2018
1 parent c1ab619 commit 1ed9722
Show file tree
Hide file tree
Showing 7 changed files with 284 additions and 95 deletions.
16 changes: 12 additions & 4 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,18 +65,26 @@ To implement your own policy, you have to implement the following interface.
```c++
struct custom_policy {
// Called on hash table construction, min_bucket_count_in_out is the minimum size
// that the hash table needs. The policy can change it to a higher bucket count if needed
// Called on the hash table creation and on rehash. The number of buckets for the table is passed in parameter.
// This number is a minimum, the policy may update this value with a higher value if needed (but not lower).
//
// If 0 is given, min_bucket_count_in_out must still be 0 after the policy creation and
// bucket_for_hash must always return 0 in this case.
explicit custom_policy(std::size_t& min_bucket_count_in_out);
// Return the bucket for the corresponding hash
// Return the bucket [0, bucket_count()) to which the hash belongs.
// If bucket_count() is 0, it must always return 0.
std::size_t bucket_for_hash(std::size_t hash) const noexcept;
// Return the number of buckets that should be used on next growth
std::size_t next_bucket_count() const;
// Maximum number of buckets supported by the policy
// Return the maximum number of buckets supported by the policy.
std::size_t max_bucket_count() const;
// Reset the growth policy as if it was created with a bucket count of 0.
// After a clear, the policy must always return 0 when bucket_for_hash is called.
void clear() noexcept;
}
```

Expand Down
26 changes: 13 additions & 13 deletions tests/custom_allocator_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,19 +128,19 @@ bool operator!=(const custom_allocator<T>&, const custom_allocator<U>&) {
BOOST_AUTO_TEST_SUITE(test_custom_allocator)

BOOST_AUTO_TEST_CASE(test_custom_allocator_1) {
// nb_global_new = 0;
nb_custom_allocs = 0;
tsl::sparse_map<int, int, std::hash<int>, std::equal_to<int>,
custom_allocator<std::pair<int, int>>> map;
const int nb_elements = 10000;
for(int i = 0; i < nb_elements; i++) {
map.insert({i, i*2});
}
BOOST_CHECK_NE(nb_custom_allocs, 0);
// BOOST_CHECK_EQUAL(nb_global_new, 0);
// nb_global_new = 0;
nb_custom_allocs = 0;

tsl::sparse_map<int, int, std::hash<int>, std::equal_to<int>,
custom_allocator<std::pair<int, int>>> map;

const int nb_elements = 10000;
for(int i = 0; i < nb_elements; i++) {
map.insert({i, i*2});
}

BOOST_CHECK_NE(nb_custom_allocs, 0);
// BOOST_CHECK_EQUAL(nb_global_new, 0);
}

BOOST_AUTO_TEST_SUITE_END()
9 changes: 8 additions & 1 deletion tests/policy_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,22 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(test_policy, Policy, test_types) {
std::size_t bucket_count = 0;
Policy policy(bucket_count);

BOOST_CHECK_EQUAL(policy.bucket_for_hash(0), 0);
BOOST_CHECK_EQUAL(bucket_count, 0);

try {
while(true) {
const std::size_t previous_bucket_count = bucket_count;

bucket_count = policy.next_bucket_count();
policy = Policy(bucket_count);

BOOST_CHECK_EQUAL(policy.bucket_for_hash(0), 0);
BOOST_CHECK(bucket_count > previous_bucket_count);
}
}
catch(const std::length_error& ) {
exception_thrown = true;
BOOST_CHECK_EQUAL(bucket_count, policy.max_bucket_count());
}

BOOST_CHECK(exception_thrown);
Expand Down
61 changes: 59 additions & 2 deletions tests/sparse_map_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,9 @@ BOOST_AUTO_TEST_CASE_TEMPLATE(test_insert, HMap, test_types) {
using key_t = typename HMap::key_type; using value_t = typename HMap:: mapped_type;

const std::size_t nb_values = 1000;
HMap map;
HMap map(0);
BOOST_CHECK_EQUAL(map.bucket_count(), 0);

typename HMap::iterator it;
bool inserted;

Expand Down Expand Up @@ -704,6 +706,50 @@ BOOST_AUTO_TEST_CASE(test_copy_constructor_operator) {
BOOST_CHECK(map_copy == map_copy3);
}

BOOST_AUTO_TEST_CASE(test_use_after_move_constructor) {
using HMap = tsl::sparse_map<std::string, move_only_test>;

const std::size_t nb_values = 100;
HMap map = utils::get_filled_hash_map<HMap>(nb_values);
HMap map_move(std::move(map));


BOOST_CHECK(map == (HMap()));
BOOST_CHECK_EQUAL(map.size(), 0);
BOOST_CHECK_EQUAL(map.bucket_count(), 0);
BOOST_CHECK_EQUAL(map.erase("a"), 0);
BOOST_CHECK(map.find("a") == map.end());

for(std::size_t i = 0; i < nb_values; i++) {
map.insert({utils::get_key<std::string>(i), utils::get_value<move_only_test>(i)});
}

BOOST_CHECK_EQUAL(map.size(), nb_values);
BOOST_CHECK(map == map_move);
}

BOOST_AUTO_TEST_CASE(test_use_after_move_operator) {
using HMap = tsl::sparse_map<std::string, move_only_test>;

const std::size_t nb_values = 100;
HMap map = utils::get_filled_hash_map<HMap>(nb_values);
HMap map_move(0);
map_move = std::move(map);


BOOST_CHECK(map == (HMap()));
BOOST_CHECK_EQUAL(map.size(), 0);
BOOST_CHECK_EQUAL(map.bucket_count(), 0);
BOOST_CHECK_EQUAL(map.erase("a"), 0);
BOOST_CHECK(map.find("a") == map.end());

for(std::size_t i = 0; i < nb_values; i++) {
map.insert({utils::get_key<std::string>(i), utils::get_value<move_only_test>(i)});
}

BOOST_CHECK_EQUAL(map.size(), nb_values);
BOOST_CHECK(map == map_move);
}


/**
Expand Down Expand Up @@ -762,6 +808,12 @@ BOOST_AUTO_TEST_CASE(test_swap) {

BOOST_CHECK(map == (tsl::sparse_map<std::int64_t, std::int64_t>{{4, 40}, {5, 50}}));
BOOST_CHECK(map2 == (tsl::sparse_map<std::int64_t, std::int64_t>{{1, 10}, {8, 80}, {3, 30}}));

map.insert({6, 60});
map2.insert({4, 40});

BOOST_CHECK(map == (tsl::sparse_map<std::int64_t, std::int64_t>{{4, 40}, {5, 50}, {6, 60}}));
BOOST_CHECK(map2 == (tsl::sparse_map<std::int64_t, std::int64_t>{{1, 10}, {8, 80}, {3, 30}, {4, 40}}));
}


Expand Down Expand Up @@ -924,6 +976,7 @@ BOOST_AUTO_TEST_CASE(test_heterogeneous_lookups) {
BOOST_AUTO_TEST_CASE(test_empty_map) {
tsl::sparse_map<std::string, int> map(0);

BOOST_CHECK_EQUAL(map.bucket_count(), 0);
BOOST_CHECK_EQUAL(map.size(), 0);
BOOST_CHECK(map.empty());

Expand Down Expand Up @@ -953,7 +1006,7 @@ BOOST_AUTO_TEST_CASE(test_empty_map) {
* Test precalculated hash
*/
BOOST_AUTO_TEST_CASE(test_precalculated_hash) {
tsl::sparse_map<int, int, std::hash<int>> map = {{1, -1}, {2, -2}, {3, -3}, {4, -4}, {5, -5}, {6, -6}};
tsl::sparse_map<int, int> map = {{1, -1}, {2, -2}, {3, -3}, {4, -4}, {5, -5}, {6, -6}};
const tsl::sparse_map<int, int> map_const = map;

/**
Expand Down Expand Up @@ -984,6 +1037,10 @@ BOOST_AUTO_TEST_CASE(test_precalculated_hash) {
BOOST_REQUIRE_EQUAL(std::distance(it_range.first, it_range.second), 1);
BOOST_CHECK_EQUAL(it_range.first->second, -3);

auto it_range_const = map_const.equal_range(3, map_const.hash_function()(3));
BOOST_REQUIRE_EQUAL(std::distance(it_range_const.first, it_range_const.second), 1);
BOOST_CHECK_EQUAL(it_range_const.first->second, -3);

/**
* erase
*/
Expand Down
8 changes: 8 additions & 0 deletions tests/sparse_set_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,13 +100,21 @@ BOOST_AUTO_TEST_CASE(test_compare) {
const tsl::sparse_set<std::string> set1_2 = {"e", "c", "b", "a", "d"};
const tsl::sparse_set<std::string> set2_1 = {"e", "c", "b", "a", "d", "f"};
const tsl::sparse_set<std::string> set3_1 = {"e", "c", "b", "a"};
const tsl::sparse_set<std::string> set4_1 = {};
const tsl::sparse_set<std::string> set4_2 = {};

BOOST_CHECK(set1_1 == set1_2);
BOOST_CHECK(set1_2 == set1_1);

BOOST_CHECK(set4_1 == set4_2);
BOOST_CHECK(set4_2 == set4_1);

BOOST_CHECK(set1_1 != set2_1);
BOOST_CHECK(set2_1 != set1_1);

BOOST_CHECK(set1_1 != set4_1);
BOOST_CHECK(set4_1 != set1_1);

BOOST_CHECK(set1_1 != set3_1);
BOOST_CHECK(set3_1 != set1_1);

Expand Down
79 changes: 52 additions & 27 deletions tsl/sparse_growth_policy.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,29 +51,34 @@ class power_of_two_growth_policy {
/**
* Called on the hash table creation and on rehash. The number of buckets for the table is passed in parameter.
* This number is a minimum, the policy may update this value with a higher value if needed (but not lower).
*
* If 0 is given, min_bucket_count_in_out must still be 0 after the policy creation and
* bucket_for_hash must always return 0 in this case.
*/
explicit power_of_two_growth_policy(std::size_t& min_bucket_count_in_out) {
if(min_bucket_count_in_out > max_bucket_count()) {
throw std::length_error("The hash table exceeds its maxmimum size.");
}

static_assert(MIN_BUCKETS_SIZE > 0, "MIN_BUCKETS_SIZE must be > 0.");
const std::size_t min_bucket_count = MIN_BUCKETS_SIZE;

min_bucket_count_in_out = std::max(min_bucket_count, min_bucket_count_in_out);
min_bucket_count_in_out = round_up_to_power_of_two(min_bucket_count_in_out);
m_mask = min_bucket_count_in_out - 1;
if(min_bucket_count_in_out > 0) {
min_bucket_count_in_out = round_up_to_power_of_two(min_bucket_count_in_out);
m_mask = min_bucket_count_in_out - 1;
}
else {
m_mask = 0;
}
}

/**
* Return the bucket [0, bucket_count()) to which the hash belongs.
* Return the bucket [0, bucket_count()) to which the hash belongs.
* If bucket_count() is 0, it must always return 0.
*/
std::size_t bucket_for_hash(std::size_t hash) const noexcept {
return hash & m_mask;
}

/**
* Return the bucket count to use when the bucket array grows on rehash.
* Return the number of buckets that should be used on next growth.
*/
std::size_t next_bucket_count() const {
if((m_mask + 1) > max_bucket_count() / GrowthFactor) {
Expand All @@ -91,6 +96,14 @@ class power_of_two_growth_policy {
return (std::numeric_limits<std::size_t>::max() / 2) + 1;
}

/**
* Reset the growth policy as if it was created with a bucket count of 0.
* After a clear, the policy must always return 0 when bucket_for_hash is called.
*/
void clear() noexcept {
m_mask = 0;
}

private:
static std::size_t round_up_to_power_of_two(std::size_t value) {
if(is_power_of_two(value)) {
Expand All @@ -114,7 +127,6 @@ class power_of_two_growth_policy {
}

protected:
static const std::size_t MIN_BUCKETS_SIZE = 2;
static_assert(is_power_of_two(GrowthFactor) && GrowthFactor >= 2, "GrowthFactor must be a power of two >= 2.");

std::size_t m_mask;
Expand All @@ -133,23 +145,24 @@ class mod_growth_policy {
throw std::length_error("The hash table exceeds its maxmimum size.");
}

static_assert(MIN_BUCKETS_SIZE > 0, "MIN_BUCKETS_SIZE must be > 0.");
const std::size_t min_bucket_count = MIN_BUCKETS_SIZE;

min_bucket_count_in_out = std::max(min_bucket_count, min_bucket_count_in_out);
m_bucket_count = min_bucket_count_in_out;
if(min_bucket_count_in_out > 0) {
m_mod = min_bucket_count_in_out;
}
else {
m_mod = 1;
}
}

std::size_t bucket_for_hash(std::size_t hash) const noexcept {
return hash % m_bucket_count;
return hash % m_mod;
}

std::size_t next_bucket_count() const {
if(m_bucket_count == max_bucket_count()) {
if(m_mod == max_bucket_count()) {
throw std::length_error("The hash table exceeds its maxmimum size.");
}

const double next_bucket_count = std::ceil(double(m_bucket_count) * REHASH_SIZE_MULTIPLICATION_FACTOR);
const double next_bucket_count = std::ceil(double(m_mod) * REHASH_SIZE_MULTIPLICATION_FACTOR);
if(!std::isnormal(next_bucket_count)) {
throw std::length_error("The hash table exceeds its maxmimum size.");
}
Expand All @@ -166,8 +179,11 @@ class mod_growth_policy {
return MAX_BUCKET_COUNT;
}

void clear() noexcept {
m_mod = 1;
}

private:
static const std::size_t MIN_BUCKETS_SIZE = 2;
static constexpr double REHASH_SIZE_MULTIPLICATION_FACTOR = 1.0 * GrowthFactor::num / GrowthFactor::den;
static const std::size_t MAX_BUCKET_COUNT =
std::size_t(double(
Expand All @@ -176,30 +192,30 @@ class mod_growth_policy {

static_assert(REHASH_SIZE_MULTIPLICATION_FACTOR >= 1.1, "Growth factor should be >= 1.1.");

std::size_t m_bucket_count;
std::size_t m_mod;
};



namespace detail {

static constexpr const std::array<std::size_t, 39> PRIMES = {{
5ul, 17ul, 29ul, 37ul, 53ul, 67ul, 79ul, 97ul, 131ul, 193ul, 257ul, 389ul, 521ul, 769ul, 1031ul, 1543ul, 2053ul,
3079ul, 6151ul, 12289ul, 24593ul, 49157ul, 98317ul, 196613ul, 393241ul, 786433ul, 1572869ul, 3145739ul,
6291469ul, 12582917ul, 25165843ul, 50331653ul, 100663319ul, 201326611ul, 402653189ul, 805306457ul,
1610612741ul, 3221225473ul, 4294967291ul
static constexpr const std::array<std::size_t, 40> PRIMES = {{
1ul, 5ul, 17ul, 29ul, 37ul, 53ul, 67ul, 79ul, 97ul, 131ul, 193ul, 257ul, 389ul, 521ul, 769ul, 1031ul,
1543ul, 2053ul, 3079ul, 6151ul, 12289ul, 24593ul, 49157ul, 98317ul, 196613ul, 393241ul, 786433ul,
1572869ul, 3145739ul, 6291469ul, 12582917ul, 25165843ul, 50331653ul, 100663319ul, 201326611ul,
402653189ul, 805306457ul, 1610612741ul, 3221225473ul, 4294967291ul
}};

template<unsigned int IPrime>
static constexpr std::size_t mod(std::size_t hash) { return hash % PRIMES[IPrime]; }

// MOD_PRIME[iprime](hash) returns hash % PRIMES[iprime]. This table allows for faster modulo as the
// compiler can optimize the modulo code better with a constant known at the compilation.
static constexpr const std::array<std::size_t(*)(std::size_t), 39> MOD_PRIME = {{
static constexpr const std::array<std::size_t(*)(std::size_t), 40> MOD_PRIME = {{
&mod<0>, &mod<1>, &mod<2>, &mod<3>, &mod<4>, &mod<5>, &mod<6>, &mod<7>, &mod<8>, &mod<9>, &mod<10>,
&mod<11>, &mod<12>, &mod<13>, &mod<14>, &mod<15>, &mod<16>, &mod<17>, &mod<18>, &mod<19>, &mod<20>,
&mod<21>, &mod<22>, &mod<23>, &mod<24>, &mod<25>, &mod<26>, &mod<27>, &mod<28>, &mod<29>, &mod<30>,
&mod<31>, &mod<32>, &mod<33>, &mod<34>, &mod<35>, &mod<36>, &mod<37> , &mod<38>
&mod<31>, &mod<32>, &mod<33>, &mod<34>, &mod<35>, &mod<36>, &mod<37> , &mod<38>, &mod<39>
}};

}
Expand Down Expand Up @@ -238,7 +254,12 @@ class prime_growth_policy {
}

m_iprime = static_cast<unsigned int>(std::distance(detail::PRIMES.begin(), it_prime));
min_bucket_count_in_out = *it_prime;
if(min_bucket_count_in_out > 0) {
min_bucket_count_in_out = *it_prime;
}
else {
min_bucket_count_in_out = 0;
}
}

std::size_t bucket_for_hash(std::size_t hash) const noexcept {
Expand All @@ -257,6 +278,10 @@ class prime_growth_policy {
return detail::PRIMES.back();
}

void clear() noexcept {
m_iprime = 0;
}

private:
unsigned int m_iprime;

Expand Down
Loading

0 comments on commit 1ed9722

Please sign in to comment.