Merge pull request #4665 from danpoe/refactor/sharing-map

Add sharing map documentation and move unit test sections

Author: danpoe

src/util/sharing_map.h

@@ -535,8 +535,26 @@ class sharing_mapt
     return lp;
   }
 
+  /// Move a container node (containing a single leaf) further down the tree
+  /// such as to resolve a collision with another key-value pair. This method is
+  /// called by `insert()` to resolve a collision between a key-value pair to be
+  /// newly inserted, and a key-value pair existing in the map.
+  ///
+  /// \param starting_level: the depth of the inner node pointing to the
+  ///   container node with a single leaf
+  /// \param key_suffix: hash code of the existing key in the map, shifted to
+  ///   the right by `chunk * starting_level` bits (i.e., \p key_suffix is the
+  ///   rest of the hash code used to determine the position of the key-value
+  ///   pair below level \p starting_level
+  /// \param bit_last: last portion of the hash code of the key existing in the
+  ///   map (`inner[bit_last]` points to the container node to move further down
+  ///   the tree)
+  /// \param inner: inner node of which the child `inner[bit_last]` is the
+  ///   container node to move further down the tree
+  /// \return pointer to the container to which the element to be newly inserted
+  ///   can be added
   innert *migrate(
-    const std::size_t i,
+    const std::size_t starting_level,
     const std::size_t key_suffix,
     const std::size_t bit_last,
     innert &inner);
@@ -545,6 +563,20 @@ class sharing_mapt
     const innert &n,
     std::function<void(const key_type &k, const mapped_type &m)> f) const;
 
+  /// Add a delta item to the delta view if the value in the \p container (which
+  /// must only contain a single leaf) is not shared with any of the values in
+  /// the subtree below \p inner. This method is called by `get_delta_view()`
+  /// when a container containing a single leaf is encountered in the first map,
+  /// and the corresponding node in the second map is an inner node.
+  ///
+  /// \param container: container node containing a single leaf, part of the
+  ///   first map in a call `map1.get_delta_view(map2, ...)`
+  /// \param inner: inner node which is part of the second map
+  /// \param level: depth of the nodes in the maps (both \p container and \p
+  ///   inner must be at the same depth in their respective maps)
+  /// \param delta_view: delta view to add delta items to
+  /// \param only_common: flag indicating if only items are added to the delta
+  ///   view for which the keys are in both maps
   void add_item_if_not_shared(
     const innert &container,
     const innert &inner,
@@ -572,7 +604,7 @@ class sharing_mapt
 
   // derived config
   static const std::size_t mask;
-  static const std::size_t steps;
+  static const std::size_t levels;
 
   // key-value map
   innert map;
@@ -1108,12 +1140,12 @@ SHARING_MAPT(void)::erase(const key_type &k)
 }
 
 SHARING_MAPT2(, innert *)::migrate(
-  const std::size_t step,
+  const std::size_t starting_level,
   const std::size_t key_suffix,
   const std::size_t bit_last,
   innert &inner)
 {
-  SM_ASSERT(step < steps - 1);
+  SM_ASSERT(starting_level < levels - 1);
   SM_ASSERT(inner.is_defined_internal());
 
   const innert &child = *inner.find_child(bit_last);
@@ -1127,7 +1159,7 @@ SHARING_MAPT2(, innert *)::migrate(
   const leaft &leaf = ll.front();
   std::size_t key_existing = hash()(leaf.get_key());
 
-  key_existing >>= chunk * step;
+  key_existing >>= chunk * starting_level;
 
   // Copy the container
   innert container_copy(child);
@@ -1141,13 +1173,13 @@ SHARING_MAPT2(, innert *)::migrate(
 
   // Find place for both elements
 
-  std::size_t i = step + 1;
+  std::size_t level = starting_level + 1;
   std::size_t key = key_suffix;
 
   key_existing >>= chunk;
   key >>= chunk;
 
-  SM_ASSERT(i < steps);
+  SM_ASSERT(level < levels);
 
   do
   {
@@ -1171,8 +1203,8 @@ SHARING_MAPT2(, innert *)::migrate(
     key >>= chunk;
     key_existing >>= chunk;
 
-    i++;
-  } while(i < steps);
+    level++;
+  } while(level < levels);
 
   leaft leaf_copy(as_const(&container_copy)->get_container().front());
   ip->get_container().push_front(leaf_copy);
@@ -1191,15 +1223,15 @@ ::insert(const key_type &k, valueU &&m)
   // The root cannot be a container node
   SM_ASSERT(ip->is_internal());
 
-  std::size_t i = 0;
+  std::size_t level = 0;
 
   while(true)
   {
     std::size_t bit = key & mask;
 
     SM_ASSERT(ip != nullptr);
     SM_ASSERT(ip->is_internal());
-    SM_ASSERT(i == 0 || !ip->empty());
+    SM_ASSERT(level == 0 || !ip->empty());
 
     innert *child = ip->add_child(bit);
 
@@ -1218,10 +1250,10 @@ ::insert(const key_type &k, valueU &&m)
 
     if(child->is_container())
     {
-      if(i < steps - 1)
+      if(level < levels - 1)
       {
         // Migrate the elements downwards
-        innert *cp = migrate(i, key, bit, *ip);
+        innert *cp = migrate(level, key, bit, *ip);
 
         cp->place_leaf(k, std::forward<valueU>(m));
       }
@@ -1236,11 +1268,11 @@ ::insert(const key_type &k, valueU &&m)
       return;
     }
 
-    SM_ASSERT(i == steps - 1 || child->is_defined_internal());
+    SM_ASSERT(level == levels - 1 || child->is_defined_internal());
 
     ip = child;
     key >>= chunk;
-    i++;
+    level++;
   }
 }
 
@@ -1298,6 +1330,6 @@ SHARING_MAPT(const std::size_t)::bits = 30;
 SHARING_MAPT(const std::size_t)::chunk = 3;
 
 SHARING_MAPT(const std::size_t)::mask = 0xffff >> (16 - chunk);
-SHARING_MAPT(const std::size_t)::steps = bits / chunk;
+SHARING_MAPT(const std::size_t)::levels = bits / chunk;
 
 #endif

unit/util/sharing_map.cpp

@@ -111,6 +111,10 @@ void sharing_map_internals_test()
     REQUIRE(marked.size() >= 2);
   }
 
+  // These tests check whether the internal representation of the map has the
+  // expected shape after a series of operations. This is done by checking
+  // whether the map has a number of nodes (inner nodes, container nodes, and
+  // leaf nodes) that is consistent with the expected shape.
   SECTION("single map shape")
   {
     std::set<const void *> marked;
@@ -172,94 +176,7 @@ void sharing_map_internals_test()
     }
   }
 
-  SECTION("two maps shape")
-  {
-    std::set<const void *> marked;
-    std::size_t chunk = 3;
-
-    sharing_map_unsignedt sm1;
-    sharing_map_unsignedt sm2;
-
-    SECTION("left map deeper")
-    {
-      sm1.insert(0, "a");
-      sm1.insert(1 << (2 * chunk), "b");
-
-      sm2.insert(0, "c");
-      sm2.insert(1, "d");
-
-      sharing_map_unsignedt::delta_viewt delta_view;
-      sm1.get_delta_view(sm2, delta_view, false);
-
-      REQUIRE(delta_view.size() == 2);
-
-      const bool b0 = delta_view[0].is_in_both_maps();
-      const bool b1 = delta_view[1].is_in_both_maps();
-
-      REQUIRE((b0 || b1));
-      REQUIRE(!(b0 && b1));
-
-      if(b0)
-      {
-        REQUIRE(delta_view[0].k == 0);
-        REQUIRE(delta_view[0].m == "a");
-        REQUIRE(delta_view[0].get_other_map_value() == "c");
-
-        REQUIRE(delta_view[1].k == 1 << (2 * chunk));
-        REQUIRE(delta_view[1].m == "b");
-      }
-      else
-      {
-        REQUIRE(delta_view[0].k == 1 << (2 * chunk));
-        REQUIRE(delta_view[0].m == "b");
-
-        REQUIRE(delta_view[1].k == 0);
-        REQUIRE(delta_view[1].m == "a");
-        REQUIRE(delta_view[1].get_other_map_value() == "c");
-      }
-    }
-
-    SECTION("right map deeper")
-    {
-      sm1.insert(0, "c");
-      sm1.insert(1, "d");
-
-      sm2.insert(0, "a");
-      sm2.insert(1 << (2 * chunk), "b");
-
-      sharing_map_unsignedt::delta_viewt delta_view;
-      sm1.get_delta_view(sm2, delta_view, false);
-
-      REQUIRE(delta_view.size() == 2);
-
-      const bool b0 = delta_view[0].is_in_both_maps();
-      const bool b1 = delta_view[1].is_in_both_maps();
-
-      REQUIRE((b0 || b1));
-      REQUIRE(!(b0 && b1));
-
-      if(b0)
-      {
-        REQUIRE(delta_view[0].k == 0);
-        REQUIRE(delta_view[0].m == "c");
-        REQUIRE(delta_view[0].get_other_map_value() == "a");
-
-        REQUIRE(delta_view[1].k == 1);
-        REQUIRE(delta_view[1].m == "d");
-      }
-      else
-      {
-        REQUIRE(delta_view[0].k == 1);
-        REQUIRE(delta_view[0].m == "d");
-
-        REQUIRE(delta_view[1].k == 0);
-        REQUIRE(delta_view[1].m == "c");
-        REQUIRE(delta_view[1].get_other_map_value() == "a");
-      }
-    }
-  }
-
-  SECTION("two maps shape, sharing")
+  SECTION("delta view (sharing, one of the maps is deeper)")
   {
     std::set<const void *> marked;
     std::size_t chunk = 3;
@@ -680,6 +597,93 @@ TEST_CASE("Sharing map views and iteration", "[core][util]")
     sm1.get_delta_view(sm2, delta_view, false);
     REQUIRE(delta_view.size() == 3);
   }
+
+  SECTION("delta view (no sharing, one of the maps is deeper)")
+  {
+    std::set<const void *> marked;
+    std::size_t chunk = 3;
+
+    sharing_map_unsignedt sm1;
+    sharing_map_unsignedt sm2;
+
+    SECTION("left map deeper")
+    {
+      sm1.insert(0, "a");
+      sm1.insert(1 << (2 * chunk), "b");
+
+      sm2.insert(0, "c");
+      sm2.insert(1, "d");
+
+      sharing_map_unsignedt::delta_viewt delta_view;
+      sm1.get_delta_view(sm2, delta_view, false);
+
+      REQUIRE(delta_view.size() == 2);
+
+      const bool b0 = delta_view[0].is_in_both_maps();
+      const bool b1 = delta_view[1].is_in_both_maps();
+
+      REQUIRE((b0 || b1));
+      REQUIRE(!(b0 && b1));
+
+      if(b0)
+      {
+        REQUIRE(delta_view[0].k == 0);
+        REQUIRE(delta_view[0].m == "a");
+        REQUIRE(delta_view[0].get_other_map_value() == "c");
+
+        REQUIRE(delta_view[1].k == 1 << (2 * chunk));
+        REQUIRE(delta_view[1].m == "b");
+      }
+      else
+      {
+        REQUIRE(delta_view[0].k == 1 << (2 * chunk));
+        REQUIRE(delta_view[0].m == "b");
+
+        REQUIRE(delta_view[1].k == 0);
+        REQUIRE(delta_view[1].m == "a");
+        REQUIRE(delta_view[1].get_other_map_value() == "c");
+      }
+    }
+
+    SECTION("right map deeper")
+    {
+      sm1.insert(0, "c");
+      sm1.insert(1, "d");
+
+      sm2.insert(0, "a");
+      sm2.insert(1 << (2 * chunk), "b");
+
+      sharing_map_unsignedt::delta_viewt delta_view;
+      sm1.get_delta_view(sm2, delta_view, false);
+
+      REQUIRE(delta_view.size() == 2);
+
+      const bool b0 = delta_view[0].is_in_both_maps();
+      const bool b1 = delta_view[1].is_in_both_maps();
+
+      REQUIRE((b0 || b1));
+      REQUIRE(!(b0 && b1));
+
+      if(b0)
+      {
+        REQUIRE(delta_view[0].k == 0);
+        REQUIRE(delta_view[0].m == "c");
+        REQUIRE(delta_view[0].get_other_map_value() == "a");
+
+        REQUIRE(delta_view[1].k == 1);
+        REQUIRE(delta_view[1].m == "d");
+      }
+      else
+      {
+        REQUIRE(delta_view[0].k == 1);
+        REQUIRE(delta_view[0].m == "d");
+
+        REQUIRE(delta_view[1].k == 0);
+        REQUIRE(delta_view[1].m == "c");
+        REQUIRE(delta_view[1].get_other_map_value() == "a");
+      }
+    }
+  }
 }
 
 TEST_CASE("Sharing map view validity", "[core][util]")