Skip to content

Commit

Permalink
Merge pull request #226 from arximboldi/fix-dec-unsafe
Browse files Browse the repository at this point in the history 
Remove all usage of `dec_unsafe` from the code-base
  • Loading branch information
@arximboldi
arximboldi authored Aug 1, 2022
2 parents e02cbd7 + ad759c4 commit d98e82d
Show file tree
Hide file tree
Showing 8 changed files with 84 additions and 54 deletions.
40 changes: 20 additions & 20 deletions immer/detail/hamts/champ.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -604,8 +604,8 @@ struct champ
auto result =
do_add_mut(e, child, std::move(v), hash, shift + B);
node->children()[offset] = result.node;
if (!result.mutated)
child->dec_unsafe();
if (!result.mutated && child->dec())
node_t::delete_deep_shift(child, shift + B);
return {node, result.added, true};
} else {
assert(node->children()[offset]);
Expand Down Expand Up @@ -672,8 +672,8 @@ struct champ
{
auto hash = Hash{}(v);
auto res = do_add_mut(e, root, std::move(v), hash, 0);
if (!res.mutated)
root->dec_unsafe();
if (!res.mutated && root->dec())
node_t::delete_deep(root, 0);
root = res.node;
size += res.added ? 1 : 0;
}
Expand Down Expand Up @@ -897,8 +897,8 @@ struct champ
auto result = do_update_mut<Project, Default, Combine>(
e, child, k, std::forward<Fn>(fn), hash, shift + B);
node->children()[offset] = result.node;
if (!result.mutated)
child->dec_unsafe();
if (!result.mutated && child->dec())
node_t::delete_deep_shift(child, shift + B);
return {node, result.added, true};
} else {
auto result = do_update<Project, Default, Combine>(
Expand Down Expand Up @@ -980,8 +980,8 @@ struct champ
auto hash = Hash{}(k);
auto res = do_update_mut<Project, Default, Combine>(
e, root, k, std::forward<Fn>(fn), hash, 0);
if (!res.mutated)
root->dec_unsafe();
if (!res.mutated && root->dec())
node_t::delete_deep(root, 0);
root = res.node;
size += res.added ? 1 : 0;
}
Expand Down Expand Up @@ -1032,8 +1032,8 @@ struct champ
e, child, k, std::forward<Fn>(fn), hash, shift + B);
if (result.node) {
node->children()[offset] = result.node;
if (!result.mutated)
child->dec_unsafe();
if (!result.mutated && child->dec())
node_t::delete_deep_shift(child, shift + B);
return {node, true};
} else {
return {nullptr, false};
Expand Down Expand Up @@ -1091,8 +1091,8 @@ struct champ
auto res = do_update_if_exists_mut<Project, Combine>(
e, root, k, std::forward<Fn>(fn), hash, 0);
if (res.node) {
if (!res.mutated)
root->dec_unsafe();
if (!res.mutated && root->dec())
node_t::delete_deep(root, 0);
root = res.node;
}
}
Expand Down Expand Up @@ -1305,8 +1305,8 @@ struct champ
shift > 0) {
if (mutate) {
node_t::delete_inner(node);
if (!result.mutated)
child->dec_unsafe();
if (!result.mutated && child->dec())
node_t::delete_deep_shift(child, shift + B);
}
return {result.data.singleton, mutate};
} else {
Expand All @@ -1326,15 +1326,15 @@ struct champ
*result.data.singleton);
if (result.mutated)
detail::destroy_at(result.data.singleton);
else if (mutate)
child->dec_unsafe();
else if (mutate && child->dec())
node_t::delete_deep_shift(child, shift + B);
return {node_t::owned_values(r, e), mutate};
}
case sub_result::tree:
if (mutate) {
children[offset] = result.data.tree;
if (!result.mutated)
child->dec_unsafe();
if (!result.mutated && child->dec())
node_t::delete_deep_shift(child, shift + B);
return {node, true};
} else {
IMMER_TRY {
Expand Down Expand Up @@ -1403,8 +1403,8 @@ struct champ
case sub_result::nothing:
break;
case sub_result::tree:
if (!res.mutated)
root->dec_unsafe();
if (!res.mutated && root->dec())
node_t::delete_deep(root, 0);
root = res.data.tree;
--size;
break;
Expand Down
12 changes: 6 additions & 6 deletions immer/detail/hamts/node.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -369,8 +369,9 @@ struct node
deallocate_values(nxt, nv);
IMMER_RETHROW;
}
refs(old).dec_unsafe();
impl.d.data.inner.values = nxt;
if (refs(old).dec())
delete_values(old, nv);
return dst;
}
}
Expand Down Expand Up @@ -518,8 +519,8 @@ struct node
dst->impl.d.data.inner.datamap = src->datamap();
dst->impl.d.data.inner.nodemap = src->nodemap();
std::copy(srcp, srcp + n, dstp);
inc_nodes(srcp, n);
srcp[offset]->dec_unsafe();
inc_nodes(srcp, offset);
inc_nodes(srcp + offset + 1, n - offset - 1);
dstp[offset] = child;
return dst;
}
Expand Down Expand Up @@ -710,8 +711,8 @@ struct node
deallocate_inner(dst, n - 1, nv + 1);
IMMER_RETHROW;
}
inc_nodes(src->children(), n);
src->children()[noffset]->dec_unsafe();
inc_nodes(src->children(), noffset);
inc_nodes(src->children() + noffset + 1, n - noffset - 1);
std::copy(src->children(), src->children() + noffset, dst->children());
std::copy(src->children() + noffset + 1,
src->children() + n,
Expand Down Expand Up @@ -1046,7 +1047,6 @@ struct node
}

bool dec() const { return refs(this).dec(); }
void dec_unsafe() const { refs(this).dec_unsafe(); }

static void inc_nodes(node_t** p, count_t n)
{
Expand Down
58 changes: 54 additions & 4 deletions immer/detail/rbts/node.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,19 @@ struct node
return dst;
}

static node_t* do_copy_inner_replace(
node_t* dst, node_t* src, count_t n, count_t offset, node_t* child)
{
IMMER_ASSERT_TAGGED(dst->kind() == kind_t::inner);
IMMER_ASSERT_TAGGED(src->kind() == kind_t::inner);
auto p = src->inner();
inc_nodes(p, offset);
inc_nodes(p + offset + 1, n - offset - 1);
std::copy(p, p + n, dst->inner());
dst->inner()[offset] = child;
return dst;
}

static node_t* copy_inner_r(node_t* src, count_t n)
{
IMMER_ASSERT_TAGGED(src->kind() == kind_t::inner);
Expand Down Expand Up @@ -582,6 +595,23 @@ struct node
return dst;
}

static node_t* do_copy_inner_replace_r(
node_t* dst, node_t* src, count_t n, count_t offset, node_t* child)
{
IMMER_ASSERT_TAGGED(dst->kind() == kind_t::inner);
IMMER_ASSERT_TAGGED(src->kind() == kind_t::inner);
auto src_r = src->relaxed();
auto dst_r = dst->relaxed();
auto p = src->inner();
inc_nodes(p, offset);
inc_nodes(p + offset + 1, n - offset - 1);
std::copy(src->inner(), src->inner() + n, dst->inner());
std::copy(src_r->d.sizes, src_r->d.sizes + n, dst_r->d.sizes);
dst_r->d.count = n;
dst->inner()[offset] = child;
return dst;
}

static node_t* do_copy_inner_sr(node_t* dst, node_t* src, count_t n)
{
if (embed_relaxed)
Expand All @@ -593,6 +623,21 @@ struct node
}
}

static node_t* do_copy_inner_replace_sr(
node_t* dst, node_t* src, count_t n, count_t offset, node_t* child)
{
if (embed_relaxed)
return do_copy_inner_replace_r(dst, src, n, offset, child);
else {
auto p = src->inner();
inc_nodes(p, offset);
inc_nodes(p + offset + 1, n - offset - 1);
std::copy(p, p + n, dst->inner());
dst->inner()[offset] = child;
return dst;
}
}

static node_t* copy_leaf(node_t* src, count_t n)
{
IMMER_ASSERT_TAGGED(src->kind() == kind_t::leaf);
Expand Down Expand Up @@ -817,10 +862,12 @@ struct node
auto dst_r = impl.d.data.inner.relaxed =
new (heap::allocate(max_sizeof_relaxed)) relaxed_t;
if (src_r) {
node_t::refs(src_r).dec_unsafe();
auto n = dst_r->d.count = src_r->d.count;
std::copy(
src_r->d.sizes, src_r->d.sizes + n, dst_r->d.sizes);
if (node_t::refs(src_r).dec())
heap::deallocate(node_t::sizeof_inner_r_n(n),
src_r);
}
node_t::ownee(dst_r) = e;
return dst_r;
Expand All @@ -842,10 +889,12 @@ struct node
auto dst_r = impl.d.data.inner.relaxed =
new (heap::allocate(max_sizeof_relaxed)) relaxed_t;
if (src_r) {
node_t::refs(src_r).dec_unsafe();
auto n = dst_r->d.count = src_r->d.count;
std::copy(
src_r->d.sizes, src_r->d.sizes + n, dst_r->d.sizes);
if (node_t::refs(src_r).dec())
heap::deallocate(node_t::sizeof_inner_r_n(n),
src_r);
}
node_t::ownee(dst_r) = ec;
return dst_r;
Expand All @@ -866,9 +915,11 @@ struct node
auto dst_r =
new (heap::allocate(max_sizeof_relaxed)) relaxed_t;
if (src_r) {
node_t::refs(src_r).dec_unsafe();
std::copy(
src_r->d.sizes, src_r->d.sizes + n, dst_r->d.sizes);
if (node_t::refs(src_r).dec())
heap::deallocate(node_t::sizeof_inner_r_n(n),
src_r);
}
dst_r->d.count = n;
node_t::ownee(dst_r) = e;
Expand All @@ -890,7 +941,6 @@ struct node
}

bool dec() const { return refs(this).dec(); }
void dec_unsafe() const { refs(this).dec_unsafe(); }

static void inc_nodes(node_t** p, count_t n)
{
Expand Down
10 changes: 4 additions & 6 deletions immer/detail/rbts/operations.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -468,9 +468,8 @@ struct update_visitor : visitor_base<update_visitor<NodeT>>
auto node = node_t::make_inner_sr_n(count, pos.relaxed());
IMMER_TRY {
auto child = pos.towards_oh(this_t{}, idx, offset, fn);
node_t::do_copy_inner_sr(node, pos.node(), count);
node->inner()[offset]->dec_unsafe();
node->inner()[offset] = child;
node_t::do_copy_inner_replace_sr(
node, pos.node(), count, offset, child);
return node;
}
IMMER_CATCH (...) {
Expand All @@ -487,9 +486,8 @@ struct update_visitor : visitor_base<update_visitor<NodeT>>
auto node = node_t::make_inner_n(count);
IMMER_TRY {
auto child = pos.towards_oh_ch(this_t{}, idx, offset, count, fn);
node_t::do_copy_inner(node, pos.node(), count);
node->inner()[offset]->dec_unsafe();
node->inner()[offset] = child;
node_t::do_copy_inner_replace(
node, pos.node(), count, offset, child);
return node;
}
IMMER_CATCH (...) {
Expand Down
1 change: 0 additions & 1 deletion immer/refcount/no_refcount_policy.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ struct no_refcount_policy

void inc() {}
bool dec() { return false; }
void dec_unsafe() {}
bool unique() { return false; }
};

Expand Down
6 changes: 0 additions & 6 deletions immer/refcount/refcount_policy.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,6 @@ struct refcount_policy

bool dec() { return 1 == refcount.fetch_sub(1, std::memory_order_acq_rel); }

void dec_unsafe()
{
assert(refcount.load() > 1);
refcount.fetch_sub(1, std::memory_order_relaxed);
}

bool unique() { return refcount == 1; }
};

Expand Down
1 change: 0 additions & 1 deletion immer/refcount/unsafe_refcount_policy.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ struct unsafe_refcount_policy

void inc() { ++refcount; }
bool dec() { return --refcount == 0; }
void dec_unsafe() { --refcount; }
bool unique() { return refcount == 1; }
};

Expand Down
10 changes: 0 additions & 10 deletions test/memory/refcounts.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,16 +42,6 @@ void test_refcount()
CHECK(!elem.dec());
CHECK(elem.dec());
}

SECTION("inc dec unsafe")
{
refcount elem{};
elem.inc();
CHECK(!elem.dec());
elem.inc();
elem.dec_unsafe();
CHECK(elem.dec());
}
}

TEST_CASE("basic refcount") { test_refcount<immer::refcount_policy>(); }
Expand Down

0 comments on commit d98e82d

Please sign in to comment.