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

2023 07 fix more move allocator issues #81

Merged
merged 4 commits into from
Jul 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
cmake_minimum_required(VERSION 3.12)
project("unordered_dense"
VERSION 4.0.2
VERSION 4.0.4
DESCRIPTION "A fast & densely stored hashmap and hashset based on robin-hood backward shift deletion"
HOMEPAGE_URL "https://github.com/martinus/unordered_dense")

Expand Down
68 changes: 35 additions & 33 deletions include/ankerl/unordered_dense.h
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
///////////////////////// ankerl::unordered_dense::{map, set} /////////////////////////

// A fast & densely stored hashmap and hashset based on robin-hood backward shift deletion.
// Version 4.0.2
// Version 4.0.4
// https://github.com/martinus/unordered_dense
//
// Licensed under the MIT License <http://opensource.org/licenses/MIT>.
Expand Down Expand Up @@ -32,7 +32,7 @@
// see https://semver.org/spec/v2.0.0.html
#define ANKERL_UNORDERED_DENSE_VERSION_MAJOR 4 // NOLINT(cppcoreguidelines-macro-usage) incompatible API changes
#define ANKERL_UNORDERED_DENSE_VERSION_MINOR 0 // NOLINT(cppcoreguidelines-macro-usage) backwards compatible functionality
#define ANKERL_UNORDERED_DENSE_VERSION_PATCH 2 // NOLINT(cppcoreguidelines-macro-usage) backwards compatible bug fixes
#define ANKERL_UNORDERED_DENSE_VERSION_PATCH 4 // NOLINT(cppcoreguidelines-macro-usage) backwards compatible bug fixes

// API versioning with inline namespace, see https://www.foonathan.net/2018/11/inline-namespaces/

Expand Down Expand Up @@ -598,24 +598,18 @@ class segmented_vector {
: m_blocks(vec_alloc(alloc)) {}

segmented_vector(segmented_vector&& other, Allocator alloc)
: m_blocks(vec_alloc(alloc)) {
if (other.get_allocator() == alloc) {
*this = std::move(other);
} else {
// Oh my, allocator is different so we need to copy everything.
append_everything_from(std::move(other));
}
: segmented_vector(alloc) {
*this = std::move(other);
}

segmented_vector(segmented_vector&& other) noexcept
: m_blocks(std::move(other.m_blocks))
, m_size(std::exchange(other.m_size, {})) {}

segmented_vector(segmented_vector const& other, Allocator alloc)
: m_blocks(vec_alloc(alloc)) {
append_everything_from(other);
}

segmented_vector(segmented_vector&& other) noexcept
: segmented_vector(std::move(other), get_allocator()) {}

segmented_vector(segmented_vector const& other) {
append_everything_from(other);
}
Expand All @@ -632,8 +626,14 @@ class segmented_vector {
auto operator=(segmented_vector&& other) noexcept -> segmented_vector& {
clear();
dealloc();
m_blocks = std::move(other.m_blocks);
m_size = std::exchange(other.m_size, {});
if (other.get_allocator() == get_allocator()) {
m_blocks = std::move(other.m_blocks);
m_size = std::exchange(other.m_size, {});
} else {
// make sure to construct with other's allocator!
m_blocks = std::vector<pointer, vec_alloc>(vec_alloc(other.get_allocator()));
append_everything_from(std::move(other));
}
return *this;
}

Expand Down Expand Up @@ -1169,15 +1169,8 @@ class table : public std::conditional_t<is_map_v<T>, base_table_type_map<T>, bas
: table(std::move(other), other.m_values.get_allocator()) {}

table(table&& other, allocator_type const& alloc) noexcept
: m_values(std::move(other.m_values), alloc)
, m_buckets(std::exchange(other.m_buckets, nullptr))
, m_num_buckets(std::exchange(other.m_num_buckets, 0))
, m_max_bucket_capacity(std::exchange(other.m_max_bucket_capacity, 0))
, m_max_load_factor(std::exchange(other.m_max_load_factor, default_max_load_factor))
, m_hash(std::exchange(other.m_hash, {}))
, m_equal(std::exchange(other.m_equal, {}))
, m_shifts(std::exchange(other.m_shifts, initial_shifts)) {
other.m_values.clear();
: m_values(alloc) {
*this = std::move(other);
}

table(std::initializer_list<value_type> ilist,
Expand Down Expand Up @@ -1221,21 +1214,30 @@ class table : public std::conditional_t<is_map_v<T>, base_table_type_map<T>, bas
if (&other != this) {
deallocate_buckets(); // deallocate before m_values is set (might have another allocator)
m_values = std::move(other.m_values);
other.m_values.clear();

// we can only reuse m_buckets over when both maps have the same allocator!
// we can only reuse m_buckets when both maps have the same allocator!
if (get_allocator() == other.get_allocator()) {
m_buckets = std::exchange(other.m_buckets, nullptr);
m_num_buckets = std::exchange(other.m_num_buckets, 0);
m_max_bucket_capacity = std::exchange(other.m_max_bucket_capacity, 0);
m_shifts = std::exchange(other.m_shifts, initial_shifts);
m_max_load_factor = std::exchange(other.m_max_load_factor, default_max_load_factor);
m_hash = std::exchange(other.m_hash, {});
m_equal = std::exchange(other.m_equal, {});
} else {
// set max_load_factor *before* copying the other's buckets, so we have the same
// behavior
m_max_load_factor = other.m_max_load_factor;

// copy_buckets sets m_buckets, m_num_buckets, m_max_bucket_capacity, m_shifts
copy_buckets(other);
// clear's the other's buckets so other is now already usable.
other.clear_buckets();
m_hash = other.m_hash;
m_equal = other.m_equal;
}

m_num_buckets = std::exchange(other.m_num_buckets, 0);
m_max_bucket_capacity = std::exchange(other.m_max_bucket_capacity, 0);
m_max_load_factor = std::exchange(other.m_max_load_factor, default_max_load_factor);
m_hash = std::exchange(other.m_hash, {});
m_equal = std::exchange(other.m_equal, {});
m_shifts = std::exchange(other.m_shifts, initial_shifts);
other.m_values.clear();
// map "other" is now already usable, it's empty.
}
return *this;
}
Expand Down
2 changes: 1 addition & 1 deletion meson.build
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
#

project('unordered_dense', 'cpp',
version: '4.0.2',
version: '4.0.4',
license: 'MIT',
default_options : [
'cpp_std=c++17',
Expand Down
2 changes: 1 addition & 1 deletion test/unit/namespace.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

#include <doctest.h>

namespace versioned_namespace = ankerl::unordered_dense::v4_0_2;
namespace versioned_namespace = ankerl::unordered_dense::v4_0_4;

static_assert(std::is_same_v<versioned_namespace::map<int, int>, ankerl::unordered_dense::map<int, int>>);
static_assert(std::is_same_v<versioned_namespace::hash<int>, ankerl::unordered_dense::hash<int>>);
Expand Down
92 changes: 83 additions & 9 deletions test/unit/pmr_move_with_allocators.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,27 +8,101 @@
// windows' vector has different allocation behavior, macos has linker errors
# if __linux__

using int_str_map = ankerl::unordered_dense::pmr::map<int, std::string>;
namespace {

// creates a map and moves it out
auto return_hello_world(ANKERL_UNORDERED_DENSE_PMR::memory_resource* resource) {
int_str_map map_default_resource(resource);
template <typename Map>
auto return_hello_world(ANKERL_UNORDERED_DENSE_PMR::memory_resource* resource) -> Map {
Map map_default_resource(resource);
map_default_resource[0] = "Hello";
map_default_resource[1] = "World";
return map_default_resource;
}

TEST_CASE("move_with_allocators") {
int_str_map map_default_resource(ANKERL_UNORDERED_DENSE_PMR::new_delete_resource());

template <typename Map>
auto doTest() {
ANKERL_UNORDERED_DENSE_PMR::synchronized_pool_resource pool;

// segfaults if m_buckets is reused
{
map_default_resource = return_hello_world(&pool);
REQUIRE(map_default_resource.contains(0));
Map m(ANKERL_UNORDERED_DENSE_PMR::new_delete_resource());
m = return_hello_world<Map>(&pool);
REQUIRE(m.contains(0));
}

{
Map m(ANKERL_UNORDERED_DENSE_PMR::new_delete_resource());
m[0] = "foo";
m = return_hello_world<Map>(&pool);
REQUIRE(m[0] == "Hello");
}

{
Map m(return_hello_world<Map>(&pool), ANKERL_UNORDERED_DENSE_PMR::new_delete_resource());
REQUIRE(m.contains(0));
}

{
Map a(ANKERL_UNORDERED_DENSE_PMR::new_delete_resource());
a[0] = "hello";
Map b(&pool);
b[0] = "world";

// looping here causes lots of memory held up
// in the resources
for (int i = 0; i < 100; ++i) {
std::swap(a, b);
REQUIRE(b[0] == "hello");
REQUIRE(a[0] == "world");

std::swap(a, b);
REQUIRE(a[0] == "hello");
REQUIRE(b[0] == "world");
}
}

{
Map a(ANKERL_UNORDERED_DENSE_PMR::new_delete_resource());
a[0] = "hello";
Map b(&pool);
b[0] = "world";

// looping here causes lots of memory held up
// in the resources
for (int i = 0; i < 100; ++i) {
std::swap(a, b);
REQUIRE(b[0] == "hello");
REQUIRE(a[0] == "world");

std::swap(a, b);
REQUIRE(a[0] == "hello");
REQUIRE(b[0] == "world");
}
}

{
Map a(&pool);
a[0] = "world";

Map tmp(ANKERL_UNORDERED_DENSE_PMR::new_delete_resource());
tmp[0] = "nope";
tmp = std::move(a);
REQUIRE(tmp[0] == "world");
REQUIRE(a.empty());
a[0] = "hey";
REQUIRE(a.size() == 1);
REQUIRE(a[0] == "hey");
}
}

TEST_CASE("move_with_allocators") {
doTest<ankerl::unordered_dense::pmr::map<int, std::string>>();
}

TEST_CASE("move_with_allocators_segmented") {
doTest<ankerl::unordered_dense::pmr::segmented_map<int, std::string>>();
}

} // namespace

# endif
#endif