Skip to content

Make sharing map iteration independent of tree depth #4582

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Apr 29, 2019
Merged

Make sharing map iteration independent of tree depth #4582

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6162e94
Add methods to check if sharing node is non-empty and of a certain type
danpoe Apr 26, 2019
4d20bce
Make sharing map iteration independent of tree depth
danpoe Apr 26, 2019
4d3cb8c
Add unit test to test iteration over empty sharing map
danpoe Apr 25, 2019
59995bb
Make counting of unmarked nodes in sharing map independent of tree depth
danpoe Apr 28, 2019
16dae89
Make get_delta_view() of sharing map independent of tree depth
danpoe Apr 28, 2019
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
181 changes: 106 additions & 75 deletions src/util/sharing_map.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,7 +453,7 @@ class sharing_mapt
if(empty())
return;

iterate(map, 0, f);
iterate(map, f);
}

#if !defined(_MSC_VER)
Expand Down Expand Up @@ -525,12 +525,15 @@ class sharing_mapt
}

void iterate(
const baset &n,
const unsigned start_depth,
const innert &n,
std::function<void(const key_type &k, const mapped_type &m)> f) const;

void gather_all(const baset &n, const unsigned depth, delta_viewt &delta_view)
const;
void gather_all(const innert &n, delta_viewt &delta_view) const;

bool is_singular(const leaf_listt &ll) const
{
return !ll.empty() && std::next(ll.begin()) == ll.end();
}

std::size_t count_unmarked_nodes(
bool leafs_only,
Expand Down Expand Up @@ -559,42 +562,37 @@ class sharing_mapt

SHARING_MAPT(void)
::iterate(
const baset &n,
unsigned start_depth,
const innert &n,
std::function<void(const key_type &k, const mapped_type &m)> f) const
{
typedef std::pair<unsigned, const baset *> stack_itemt;
SM_ASSERT(!n.empty());

std::stack<stack_itemt> stack;
stack.push({start_depth, &n});
std::stack<const innert *> stack;
stack.push(&n);

do
{
const stack_itemt &si = stack.top();

const unsigned depth = si.first;
const baset *bp = si.second;

const innert *ip = stack.top();
stack.pop();

if(depth < steps) // internal
SM_ASSERT(!ip->empty());

if(ip->is_internal())
{
const innert *ip = static_cast<const innert *>(bp);
const to_mapt &m = ip->get_to_map();
SM_ASSERT(!m.empty());

for(const auto &item : m)
{
const innert *i = &item.second;
stack.push({depth + 1, i});
stack.push(i);
}
}
else // container
else
{
SM_ASSERT(depth == steps);
SM_ASSERT(ip->is_container());

const innert *cp = static_cast<const innert *>(bp);
const leaf_listt &ll = cp->get_container();
const leaf_listt &ll = ip->get_container();
SM_ASSERT(!ll.empty());

for(const auto &l : ll)
Expand All @@ -617,23 +615,15 @@ ::count_unmarked_nodes(

unsigned count = 0;

// depth, node pointer
typedef std::pair<unsigned, const baset *> stack_itemt;

std::stack<stack_itemt> stack;
stack.push({0, &map});
std::stack<const innert *> stack;
stack.push(&map);

do
{
const stack_itemt &si = stack.top();

const unsigned depth = si.first;
const baset *bp = si.second;

const innert *ip = stack.top();
stack.pop();

// internal node or container node
const innert *ip = static_cast<const innert *>(bp);
const unsigned use_count = ip->use_count();
const void *raw_ptr = ip->is_internal()
? (const void *)&ip->read_internal()
Expand All @@ -657,20 +647,22 @@ ::count_unmarked_nodes(
count++;
}

if(depth < steps) // internal
if(ip->is_internal())
{
SM_ASSERT(!ip->empty());

const to_mapt &m = ip->get_to_map();
SM_ASSERT(!m.empty());

for(const auto &item : m)
{
const innert *i = &item.second;
stack.push({depth + 1, i});
stack.push(i);
}
}
else // container
else
{
SM_ASSERT(depth == steps);
SM_ASSERT(ip->is_defined_container());

const leaf_listt &ll = ip->get_container();
SM_ASSERT(!ll.empty());
Expand Down Expand Up @@ -766,17 +758,17 @@ SHARING_MAPT(void)::get_view(viewt &view) const
view.push_back(view_itemt(k, m));
};

iterate(map, 0, f);
iterate(map, f);
}

SHARING_MAPT(void)
::gather_all(const baset &n, unsigned depth, delta_viewt &delta_view) const
::gather_all(const innert &n, delta_viewt &delta_view) const
{
auto f = [&delta_view](const key_type &k, const mapped_type &m) {
delta_view.push_back(delta_view_itemt(false, k, m, dummy));
};

iterate(n, depth, f);
iterate(n, f);
}

SHARING_MAPT(void)
Expand All @@ -794,13 +786,13 @@ ::get_delta_view(
{
if(!only_common)
{
gather_all(map, 0, delta_view);
gather_all(map, delta_view);
}

return;
}

typedef std::tuple<unsigned, const baset *, const baset *> stack_itemt;
typedef std::pair<const innert *, const innert *> stack_itemt;
std::stack<stack_itemt> stack;

// We do a DFS "in lockstep" simultaneously on both maps. For
Expand All @@ -810,71 +802,106 @@ ::get_delta_view(
// The stack contains the children of already visited nodes that we
// still have to visit during the traversal.

stack.push(stack_itemt(0, &map, &other.map));
if(map.shares_with(other.map))
return;

stack.push(stack_itemt(&map, &other.map));

do
{
const stack_itemt &si = stack.top();

const unsigned depth = std::get<0>(si);
const baset *bp1 = std::get<1>(si);
const baset *bp2 = std::get<2>(si);
const innert *ip1 = si.first;
const innert *ip2 = si.second;

stack.pop();

if(depth < steps) // internal
SM_ASSERT(!ip1->empty());
SM_ASSERT(!ip2->empty());

if(ip1->is_internal() && ip2->is_container())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why only this way around? Also, if a node is internal in tree 1 and a leaf in tree 2, why is it guaranteed to have a single child?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other cases are handled further down in the method.

A container node can only contain more than one element if it is at the very bottom of the tree (which happens when the hash codes of two different keys are the same). Since the corresponding node of the first map is an internal node, the node can't be at the bottom, since the bottom nodes always have to be container nodes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, got you. Suggest putting that in a comment!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, added a comment to explain this.

{
const innert *ip1 = static_cast<const innert *>(bp1);
const innert *ip2 = static_cast<const innert *>(bp2);
// The container *ip2 contains one element as only containers at the
// bottom of the tree can have more than one element. This happens when
// two different keys have the same hash code. It is known here that *ip2
// is not at the bottom of the tree, as *ip1 (the corresponding node in
// the other map) is an internal node, and internal nodes cannot be at the
// bottom of the map.
SM_ASSERT(is_singular(ip2->get_container()));

for(const auto &item : ip1->get_to_map())
{
const innert &child = item.second;
SM_ASSERT(!child.shares_with(*ip2));
stack.push(stack_itemt(&child, ip2));
}

const to_mapt &m = ip1->get_to_map();
continue;
}

for(const auto &item : m)
if(ip1->is_internal())
{
SM_ASSERT(ip2->is_internal());

for(const auto &item : ip1->get_to_map())
{
const innert *p;
const innert &child = item.second;

const innert *p;
p = ip2->find_child(item.first);
if(p==nullptr)

if(p == nullptr)
{
if(!only_common)
{
gather_all(item.second, depth + 1, delta_view);
gather_all(child, delta_view);
}
}
else if(!item.second.shares_with(*p))
else if(!child.shares_with(*p))
{
stack.push(stack_itemt(depth + 1, &item.second, p));
stack.push(stack_itemt(&child, p));
}
}

continue;
}
else // container
{
SM_ASSERT(depth == steps);

const innert *cp1 = static_cast<const innert *>(bp1);
const innert *cp2 = static_cast<const innert *>(bp2);
SM_ASSERT(ip1->is_container());

const leaf_listt &ll1 = cp1->get_container();
if(ip2->is_internal())
{
SM_ASSERT(is_singular(ip1->get_container()));

for(const auto &l1 : ll1)
for(const auto &item : ip2->get_to_map())
{
const key_type &k1=l1.get_key();
const leaft *p;
const innert &child = item.second;
SM_ASSERT(!ip1->shares_with(child));
stack.push(stack_itemt(ip1, &child));
}

p = cp2->find_leaf(k1);
continue;
}

if(p != nullptr)
{
if(!l1.shares_with(*p))
{
delta_view.push_back({true, k1, l1.get_value(), p->get_value()});
}
}
else if(!only_common)
SM_ASSERT(ip2->is_container());

for(const auto &l1 : ip1->get_container())
{
const key_type &k1 = l1.get_key();
const leaft *p;

p = ip2->find_leaf(k1);

if(p != nullptr)
{
if(!l1.shares_with(*p))
{
delta_view.push_back({false, l1.get_key(), l1.get_value(), dummy});
delta_view.push_back({true, k1, l1.get_value(), p->get_value()});
}
}
else if(!only_common)
{
delta_view.push_back({false, l1.get_key(), l1.get_value(), dummy});
}
}
}
while(!stack.empty());
Expand All @@ -894,6 +921,8 @@ SHARING_MAPT2(, innert *)::get_container_node(const key_type &k)
key >>= chunk;
}

SM_ASSERT(ip->is_container());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it not is_defined_container here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The non-const version of get_container_node() adds empty nodes if the key doesn't exist yet in the map. In this case, !empty() will not hold.


return ip;
}

Expand All @@ -916,6 +945,8 @@ SHARING_MAPT2(const, innert *)::get_container_node(const key_type &k) const
key >>= chunk;
}

SM_ASSERT(ip->is_defined_container());

return ip;
}

Expand Down
10 changes: 10 additions & 0 deletions src/util/sharing_node.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,16 @@ SN_TYPE_PAR_DEF class sharing_node_innert : public sharing_node_baset
return data.is_derived_v();
}

bool is_defined_internal() const
{
return !empty() && is_internal();
}

bool is_defined_container() const
{
return !empty() && is_container();
}

const d_it &read_internal() const
{
SN_ASSERT(!empty());
Expand Down
20 changes: 20 additions & 0 deletions unit/util/sharing_map.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,9 @@ TEST_CASE("Sharing map views and iteration", "[core][util]")
SECTION("Iterate")
{
sharing_map_standardt sm;

sm.iterate([](const irep_idt &key, const std::string &value) {});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So ... what does this test other than that it compiles?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This just checks that calling iterate() on an empty map doesn't violate an internal assertion of the map.


fill(sm);

typedef std::pair<std::string, std::string> pt;
Expand All @@ -394,6 +397,23 @@ TEST_CASE("Sharing map views and iteration", "[core][util]")
REQUIRE((pairs[2] == pt("k", "2")));
}

SECTION("Delta view (one empty)")
{
sharing_map_standardt sm1;
fill(sm1);

sharing_map_standardt sm2;

sharing_map_standardt::delta_viewt delta_view;

sm1.get_delta_view(sm2, delta_view, false);
REQUIRE(delta_view.size() == 3);

delta_view.clear();
sm2.get_delta_view(sm1, delta_view, false);
REQUIRE(delta_view.empty());
}

SECTION("Delta view (no sharing, same keys)")
{
sharing_map_standardt sm1;
Expand Down