Fix/issue 9 entry union

phtree/benchmark/insert_d_benchmark.cc

@@ -31,6 +31,7 @@ const double GLOBAL_MAX = 10000;
  */
 template <dimension_t DIM>
 class IndexBenchmark {
+    using Index = PhTreeD<DIM, std::int32_t>;
   public:
     IndexBenchmark(benchmark::State& state, TestGenerator data_type, int num_entities);
 
@@ -39,7 +40,7 @@ class IndexBenchmark {
   private:
     void SetupWorld(benchmark::State& state);
 
-    void Insert(benchmark::State& state, PhTreeD<DIM, int>& tree);
+    void Insert(benchmark::State& state, Index& tree);
 
     const TestGenerator data_type_;
     const int num_entities_;
@@ -58,7 +59,7 @@ template <dimension_t DIM>
 void IndexBenchmark<DIM>::Benchmark(benchmark::State& state) {
     for (auto _ : state) {
         state.PauseTiming();
-        auto* tree = new PhTreeD<DIM, int>();
+        auto* tree = new Index();
         state.ResumeTiming();
 
         Insert(state, *tree);
@@ -82,7 +83,7 @@ void IndexBenchmark<DIM>::SetupWorld(benchmark::State& state) {
 }
 
 template <dimension_t DIM>
-void IndexBenchmark<DIM>::Insert(benchmark::State& state, PhTreeD<DIM, int>& tree) {
+void IndexBenchmark<DIM>::Insert(benchmark::State& state, Index& tree) {
     for (int i = 0; i < num_entities_; ++i) {
         PhPointD<DIM>& p = points_[i];
         tree.emplace(p, i);

phtree/phtree_d_test.cc

@@ -48,7 +48,10 @@ struct Id {
         return _i == rhs._i;
     }
 
+    Id(Id const& rhs) = default;
+    Id(Id && rhs) = default;
     Id& operator=(Id const& rhs) = default;
+    Id& operator=(Id && rhs) = default;
 
     int _i;
 };

phtree/phtree_test.cc

@@ -76,6 +76,20 @@ struct Id {
         _i = other._i;
     }
 
+//    Id& operator=(const Id& other) = default;
+//    Id& operator=(Id&& other) = default;
+
+    Id& operator=(const Id& other) noexcept {
+        ++copy_assign_count_;
+        _i = other._i;
+        return *this;
+    }
+    Id& operator=(Id&& other)  noexcept {
+        ++move_assign_count_;
+        _i = other._i;
+        return *this;
+    }
+
     bool operator==(const Id& rhs) const {
         ++copy_assign_count_;
         return _i == rhs._i;
@@ -90,8 +104,6 @@ struct Id {
         ++destruct_count_;
     }
 
-    Id& operator=(Id const& rhs) = default;
-
     int _i;
 };
 
@@ -221,7 +233,9 @@ void SmokeTestBasicOps(size_t N) {
     ASSERT_TRUE(tree.empty());
     PhTreeDebugHelper::CheckConsistency(tree);
 
-    ASSERT_EQ(construct_count_ + copy_construct_count_ + move_construct_count_, destruct_count_);
+    // Normal construction and destruction should be symmetric. Move-construction is ignored.
+    ASSERT_GE(construct_count_ + copy_construct_count_ + move_construct_count_, destruct_count_);
+    ASSERT_LE(construct_count_ + copy_construct_count_, destruct_count_);
     // The following assertions exist only as sanity checks and may need adjusting.
     // There is nothing fundamentally wrong if a change in the implementation violates
     // any of the following assertions, as long as performance/memory impact is observed.

phtree/v16/entry.h

@@ -23,14 +23,19 @@
 #include <memory>
 #include <optional>
 
+//#define PH_TREE_ENTRY_POSTLEN 1
+
 namespace improbable::phtree::v16 {
 
 template <dimension_t DIM, typename T, typename SCALAR>
 class Node;
 
+template <dimension_t DIM, typename T, typename SCALAR>
+struct EntryVariant;
+
 /*
- * Nodes in the PH-Tree contain up to 2^DIM PhEntries, one in each geometric quadrant.
- * PhEntries can contain two types of data:
+ * Nodes in the PH-Tree contain up to 2^DIM Entries, one in each geometric quadrant.
+ * Entries can contain two types of data:
  * - A key/value pair (value of type T)
  * - A prefix/child-node pair, where prefix is the prefix of the child node and the
  *   child node is contained in a unique_ptr.
@@ -41,87 +46,200 @@ class Entry {
     using ValueT = std::remove_const_t<T>;
     using NodeT = Node<DIM, T, SCALAR>;
 
+    enum {
+        VALUE = 0,
+        NODE = 1,
+        EMPTY = 2,
+    };
+
   public:
     /*
      * Construct entry with existing node.
      */
-    Entry(const KeyT& k, std::unique_ptr<NodeT>&& node_ptr)
-    : kd_key_{k}, node_{std::move(node_ptr)}, value_{std::nullopt} {}
+    Entry(const KeyT& k, std::unique_ptr<NodeT>&& node_ptr) noexcept
+    : kd_key_{k}
+    , node_{std::move(node_ptr)}
+    , type{NODE}
+#ifdef PH_TREE_ENTRY_POSTLEN
+    , postfix_len_{node_->GetPostfixLen()}
+#endif
+    {
+    }
 
     /*
      * Construct entry with a new node.
      */
-    Entry(bit_width_t infix_len, bit_width_t postfix_len)
-    : kd_key_(), node_{std::make_unique<NodeT>(infix_len, postfix_len)}, value_{std::nullopt} {}
+    Entry(bit_width_t infix_len, bit_width_t postfix_len) noexcept
+    : kd_key_()
+    , node_{std::make_unique<NodeT>(infix_len, postfix_len)}
+    , type{NODE}
+#ifdef PH_TREE_ENTRY_POSTLEN
+    , postfix_len_{postfix_len}
+#endif
+    {
+    }
 
     /*
      * Construct entry with existing T.
      */
-    Entry(const KeyT& k, std::optional<ValueT>&& value)
-    : kd_key_{k}, node_{nullptr}, value_{std::move(value)} {}
+    Entry(const KeyT& k, std::optional<ValueT>&& value) noexcept
+    : kd_key_{k}
+    , value_{std::move(value)}
+    , type{VALUE}
+#ifdef PH_TREE_ENTRY_POSTLEN
+    , postfix_len_{0}
+#endif
+    {
+        //        value.reset();  // std::optional's move constructor does not destruct the previous
+    }
 
     /*
      * Construct entry with new T or moved T.
      */
     template <typename... Args>
-    explicit Entry(const KeyT& k, Args&&... args)
-    : kd_key_{k}, node_{nullptr}, value_{std::in_place, std::forward<Args>(args)...} {}
+    explicit Entry(const KeyT& k, Args&&... args) noexcept
+    : kd_key_{k}
+    , value_{std::in_place, std::forward<Args>(args)...}
+    , type{VALUE}
+#ifdef PH_TREE_ENTRY_POSTLEN
+    , postfix_len_{0}
+#endif
+    {
+    }
+
+    Entry(const Entry& other) = delete;
+    Entry& operator=(const Entry& other) = delete;
+
+    Entry(Entry&& other) noexcept : kd_key_{std::move(other.kd_key_)}, type{std::move(other.type)} {
+#ifdef PH_TREE_ENTRY_POSTLEN
+        postfix_len_ = std::move(other.postfix_len_);
+#endif
+        AssignUnion(std::move(other));
+    }
+
+    Entry& operator=(Entry&& other) noexcept {
+        kd_key_ = std::move(other.kd_key_);
+#ifdef PH_TREE_ENTRY_POSTLEN
+        postfix_len_ = std::move(other.postfix_len_);
+#endif
+        DestroyUnion();
+        AssignUnion(std::move(other));
+        return *this;
+    }
+
+    ~Entry() noexcept {
+        DestroyUnion();
+    }
 
     [[nodiscard]] const KeyT& GetKey() const {
         return kd_key_;
     }
 
     [[nodiscard]] bool IsValue() const {
-        return value_.has_value();
+        return type == VALUE;
     }
 
     [[nodiscard]] bool IsNode() const {
-        return node_.get() != nullptr;
+        return type == NODE;
     }
 
     [[nodiscard]] T& GetValue() const {
-        assert(IsValue());
+        assert(type == VALUE);
         return const_cast<T&>(*value_);
     }
 
     [[nodiscard]] NodeT& GetNode() const {
-        assert(IsNode());
+        assert(type == NODE);
         return *node_;
     }
 
-    void SetNode(std::unique_ptr<NodeT>&& node) {
-        assert(!IsNode());
-        node_ = std::move(node);
-        value_.reset();
+    void SetNode(std::unique_ptr<NodeT>&& node) noexcept {
+#ifdef PH_TREE_ENTRY_POSTLEN
+        postfix_len_ = node->GetPostfixLen();
+#endif
+        //        std::cout << "size EV : " << sizeof(kd_key_) << " +  " << sizeof(node_) << " +  "
+        //                  << sizeof(value_) << "+" << sizeof(type) << " = " << sizeof(*this) <<
+        //                  std::endl;
+        DestroyUnion();
+        type = NODE;
+        new (&node_) std::unique_ptr<NodeT>{std::move(node)};
+        assert(!node);
+    }
+
+    [[nodiscard]] bit_width_t GetNodePostfixLen() const {
+        assert(IsNode());
+#ifdef PH_TREE_ENTRY_POSTLEN
+        return postfix_len_;
+#else
+        return GetNode().GetPostfixLen();
+#endif
     }
 
     [[nodiscard]] std::optional<ValueT>&& ExtractValue() {
         assert(IsValue());
+        type = EMPTY;
         return std::move(value_);
     }
 
     [[nodiscard]] std::unique_ptr<NodeT>&& ExtractNode() {
         assert(IsNode());
+        type = EMPTY;
         return std::move(node_);
     }
 
     void ReplaceNodeWithDataFromEntry(Entry&& other) {
         assert(IsNode());
-        kd_key_ = other.GetKey();
+        // 'other' may be referenced from the local node, so we need to do move(other)
+        // before destructing the local node.
+        auto node = std::move(node_);
+        type = EMPTY;
+        *this = std::move(other);
+        node.~unique_ptr();
+#ifdef PH_TREE_ENTRY_POSTLEN
+        postfix_len_ = std::move(other.postfix_len_);
+#endif
+    }
 
-        if (other.IsNode()) {
-            node_ = std::move(other.node_);
+  private:
+    void AssignUnion(Entry&& other) noexcept {
+        type = std::move(other.type);
+        if (type == NODE) {
+            new (&node_) std::unique_ptr<NodeT>{std::move(other.node_)};
+        } else if (type == VALUE) {
+            new (&value_) std::optional<ValueT>{std::move(other.value_)};
         } else {
-            value_ = std::move(other.value_);
-            node_.reset();
+            assert(false && "Assigning from an EMPTY variant is a waste of time.");
         }
     }
 
-  private:
+    void DestroyUnion() noexcept {
+        if (type == VALUE) {
+            value_.~optional();
+        } else if (type == NODE) {
+            node_.~unique_ptr();
+        } else {
+            assert(EMPTY);
+        }
+        type = EMPTY;
+    }
+
     KeyT kd_key_;
-    std::unique_ptr<NodeT> node_;
-    std::optional<ValueT> value_;
+    union {
+        std::unique_ptr<NodeT> node_;
+        std::optional<ValueT> value_;
+    };
+    alignas(2) std::uint16_t type;
+    // The length (number of bits) of post fixes (the part of the coordinate that is 'below' the
+    // current node). If a variable prefix_len would refer to the number of bits in this node's
+    // prefix, and if we assume 64 bit values, the following would always hold:
+    // prefix_len + 1 + postfix_len = 64.
+    // The '+1' accounts for the 1 bit that is represented by the local node's hypercube,
+    // i.e. the same bit that is used to create the lookup keys in entries_.
+#ifdef PH_TREE_ENTRY_POSTLEN
+    alignas(2) bit_width_t postfix_len_;
+#endif
 };
+
 }  // namespace improbable::phtree::v16
 
 #endif  // PHTREE_V16_ENTRY_H