Add move constructor and assignment operator to TypedValue

Summary:
`heaptrack` shows a miniscule decrease in memory usage during query
execution.

Running the below query on the TEDTalk database 100 times gives the
following results:

- number of allocations: from 642647 to 642589
- bytes allocated in total: from 48.79 MiB to 48.78 MiB

```
MATCH (t:Tag)<-[:HasTag]-(n:Talk)
RETURN t.name AS Tag, COUNT(n) AS TalksCount
ORDER BY TalksCount DESC, Tag LIMIT 20;
```

Regarding `TypedValue`'s destructor: we're allowed to manually destruct the
union memebers that we constructed using placement-new. However, it is undefined
behavior to call the destructor after an object's lifetime has ended. Calling
`TypedValue`'s own destructor within its assignment operator counts as ending
its lifetime, which means that the next call to its destructor will invoke
undefined behavior.

See the C++ Draft N4140, clauses 12.4/15 and 3.8/1.3.

Reviewers: teon.banek, mtomic

Reviewed By: teon.banek

Subscribers: pullbot

Differential Revision: https://phabricator.memgraph.io/D1630

src/distributed/produce_rpc_server.cpp

@@ -74,7 +74,7 @@ ProduceRpcServer::OngoingProduce::PullOneFromCursor() {
     if (cursor_->Pull(frame_, context_)) {
       results.reserve(pull_symbols_.size());
       for (const auto &symbol : pull_symbols_) {
-        results.emplace_back(std::move(frame_[symbol]));
+        results.push_back(frame_[symbol]);
       }
     } else {
       cursor_state_ = PullState::CURSOR_EXHAUSTED;

src/query/plan/operator.cpp

@@ -3165,18 +3165,10 @@ class CartesianCursor : public Cursor {
   }
 
   bool Pull(Frame &frame, Context &context) override {
-    auto copy_frame = [&frame]() {
-      std::vector<TypedValue> result;
-      for (auto &elem : frame.elems()) {
-        result.emplace_back(std::move(elem));
-      }
-      return result;
-    };
-
     if (!cartesian_pull_initialized_) {
       // Pull all left_op frames.
       while (left_op_cursor_->Pull(frame, context)) {
-        left_op_frames_.emplace_back(copy_frame());
+        left_op_frames_.emplace_back(frame.elems());
       }
 
       // We're setting the iterator to 'end' here so it pulls the right
@@ -3201,7 +3193,7 @@ class CartesianCursor : public Cursor {
       // Advance right_op_cursor_.
       if (!right_op_cursor_->Pull(frame, context)) return false;
 
-      right_op_frame_ = copy_frame();
+      right_op_frame_ = frame.elems();
       left_op_frames_it_ = left_op_frames_.begin();
     } else {
       // Make sure right_op_cursor last pulled results are on frame.

src/query/typed_value.cpp

@@ -4,6 +4,7 @@
 #include <cmath>
 #include <iostream>
 #include <memory>
+#include <utility>
 
 #include "glog/logging.h"
 
@@ -50,6 +51,51 @@ TypedValue::TypedValue(const PropertyValue &value) {
   LOG(FATAL) << "Unsupported type";
 }
 
+TypedValue::TypedValue(PropertyValue &&value) {
+  switch (value.type()) {
+    case PropertyValue::Type::Null:
+      type_ = Type::Null;
+      break;
+    case PropertyValue::Type::Bool:
+      type_ = Type::Bool;
+      bool_v = value.Value<bool>();
+      break;
+    case PropertyValue::Type::Int:
+      type_ = Type::Int;
+      int_v = value.Value<int64_t>();
+      break;
+    case PropertyValue::Type::Double:
+      type_ = Type::Double;
+      double_v = value.Value<double>();
+      break;
+    case PropertyValue::Type::String:
+      type_ = Type::String;
+      // TODO: std::move() when PropertyValue is fixed
+      new (&string_v) std::string(value.Value<std::string>());
+      break;
+    case PropertyValue::Type::List: {
+      // TODO: std::move() when PropertyValue is fixed
+      type_ = Type::List;
+      auto vec = value.Value<std::vector<PropertyValue>>();
+      new (&list_v)
+          std::vector<TypedValue>(std::make_move_iterator(vec.begin()),
+                                  std::make_move_iterator(vec.end()));
+      break;
+    }
+    case PropertyValue::Type::Map: {
+      // TODO: std::move() when PropertyValue is fixed
+      type_ = Type::Map;
+      auto map = value.Value<std::map<std::string, PropertyValue>>();
+      new (&map_v) std::map<std::string, TypedValue>(
+          std::make_move_iterator(map.begin()),
+          std::make_move_iterator(map.end()));
+      break;
+    }
+  }
+
+  value = PropertyValue::Null;
+}
+
 TypedValue::TypedValue(const TypedValue &other) : type_(other.type_) {
   switch (other.type_) {
     case TypedValue::Type::Null:
@@ -85,6 +131,42 @@ TypedValue::TypedValue(const TypedValue &other) : type_(other.type_) {
   LOG(FATAL) << "Unsupported TypedValue::Type";
 }
 
+TypedValue::TypedValue(TypedValue &&other) : type_(other.type_) {
+  switch (other.type_) {
+    case TypedValue::Type::Null:
+      break;
+    case TypedValue::Type::Bool:
+      this->bool_v = other.bool_v;
+      break;
+    case Type::Int:
+      this->int_v = other.int_v;
+      break;
+    case Type::Double:
+      this->double_v = other.double_v;
+      break;
+    case TypedValue::Type::String:
+      new (&string_v) std::string(std::move(other.string_v));
+      break;
+    case Type::List:
+      new (&list_v) std::vector<TypedValue>(std::move(other.list_v));
+      break;
+    case Type::Map:
+      new (&map_v) std::map<std::string, TypedValue>(std::move(other.map_v));
+      break;
+    case Type::Vertex:
+      new (&vertex_v) VertexAccessor(std::move(other.vertex_v));
+      break;
+    case Type::Edge:
+      new (&edge_v) EdgeAccessor(std::move(other.edge_v));
+      break;
+    case Type::Path:
+      new (&path_v) Path(std::move(other.path_v));
+      break;
+  }
+
+  other = TypedValue::Null;
+}
+
 TypedValue::operator PropertyValue() const {
   switch (type_) {
     case TypedValue::Type::Null:
@@ -227,35 +309,57 @@ std::ostream &operator<<(std::ostream &os, const TypedValue &value) {
   LOG(FATAL) << "Unsupported PropertyValue::Type";
 }
 
-#define DEFINE_TYPED_VALUE_ASSIGNMENT(type_param, typed_value_type, member) \
+#define DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(type_param, typed_value_type, \
-  TypedValue &TypedValue::operator=(const type_param &other) {              \
+                                           member)                       \
-    if (this->type_ == TypedValue::Type::typed_value_type) {                \
+  TypedValue &TypedValue::operator=(type_param other) {                  \
-      this->member = other;                                                 \
+    if (this->type_ == TypedValue::Type::typed_value_type) {             \
-    } else {                                                                \
+      this->member = other;                                              \
-      *this = TypedValue(other);                                            \
+    } else {                                                             \
-    }                                                                       \
+      *this = TypedValue(other);                                         \
-                                                                            \
+    }                                                                    \
-    return *this;                                                           \
+                                                                         \
+    return *this;                                                        \
   }
 
-DEFINE_TYPED_VALUE_ASSIGNMENT(char *const, String, string_v)
+DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(const char *, String, string_v)
-DEFINE_TYPED_VALUE_ASSIGNMENT(int, Int, int_v)
+DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(int, Int, int_v)
-DEFINE_TYPED_VALUE_ASSIGNMENT(bool, Bool, bool_v)
+DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(bool, Bool, bool_v)
-DEFINE_TYPED_VALUE_ASSIGNMENT(int64_t, Int, int_v)
+DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(int64_t, Int, int_v)
-DEFINE_TYPED_VALUE_ASSIGNMENT(double, Double, double_v)
+DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(double, Double, double_v)
-DEFINE_TYPED_VALUE_ASSIGNMENT(std::string, String, string_v)
+DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(const std::string &, String, string_v)
-DEFINE_TYPED_VALUE_ASSIGNMENT(std::vector<TypedValue>, List, list_v)
+DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(const std::vector<TypedValue> &, List,
-DEFINE_TYPED_VALUE_ASSIGNMENT(TypedValue::value_map_t, Map, map_v)
+                                   list_v)
-DEFINE_TYPED_VALUE_ASSIGNMENT(VertexAccessor, Vertex, vertex_v)
+DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(const TypedValue::value_map_t &, Map, map_v)
-DEFINE_TYPED_VALUE_ASSIGNMENT(EdgeAccessor, Edge, edge_v)
+DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(const VertexAccessor &, Vertex, vertex_v)
-DEFINE_TYPED_VALUE_ASSIGNMENT(Path, Path, path_v)
+DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(const EdgeAccessor &, Edge, edge_v)
+DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(const Path &, Path, path_v)
 
-#undef DEFINE_TYPED_VALUE_ASSIGNMENT
+#undef DEFINE_TYPED_VALUE_COPY_ASSIGNMENT
+
+#define DEFINE_TYPED_VALUE_MOVE_ASSIGNMENT(type_param, typed_value_type, \
+                                           member)                       \
+  TypedValue &TypedValue::operator=(type_param &&other) {                \
+    if (this->type_ == TypedValue::Type::typed_value_type) {             \
+      this->member = std::move(other);                                   \
+    } else {                                                             \
+      *this = TypedValue(std::move(other));                              \
+    }                                                                    \
+                                                                         \
+    return *this;                                                        \
+  }
+
+DEFINE_TYPED_VALUE_MOVE_ASSIGNMENT(std::string, String, string_v)
+DEFINE_TYPED_VALUE_MOVE_ASSIGNMENT(std::vector<TypedValue>, List, list_v)
+DEFINE_TYPED_VALUE_MOVE_ASSIGNMENT(TypedValue::value_map_t, Map, map_v)
+DEFINE_TYPED_VALUE_MOVE_ASSIGNMENT(VertexAccessor, Vertex, vertex_v)
+DEFINE_TYPED_VALUE_MOVE_ASSIGNMENT(EdgeAccessor, Edge, edge_v)
+DEFINE_TYPED_VALUE_MOVE_ASSIGNMENT(Path, Path, path_v)
+
+#undef DEFINE_TYPED_VALUE_MOVE_ASSIGNMENT
 
 TypedValue &TypedValue::operator=(const TypedValue &other) {
   if (this != &other) {
-    this->~TypedValue();
+    DestroyValue();
-    // set the type of this
     type_ = other.type_;
 
     switch (other.type_) {
@@ -294,44 +398,92 @@ TypedValue &TypedValue::operator=(const TypedValue &other) {
   return *this;
 }
 
+TypedValue &TypedValue::operator=(TypedValue &&other) {
+  if (this != &other) {
+    DestroyValue();
+    type_ = other.type_;
+
+    switch (other.type_) {
+      case TypedValue::Type::Null:
+        break;
+      case TypedValue::Type::Bool:
+        this->bool_v = other.bool_v;
+        break;
+      case TypedValue::Type::Int:
+        this->int_v = other.int_v;
+        break;
+      case TypedValue::Type::Double:
+        this->double_v = other.double_v;
+        break;
+      case TypedValue::Type::String:
+        new (&string_v) std::string(std::move(other.string_v));
+        break;
+      case TypedValue::Type::List:
+        new (&list_v) std::vector<TypedValue>(std::move(other.list_v));
+        break;
+      case TypedValue::Type::Map:
+        new (&map_v) std::map<std::string, TypedValue>(std::move(other.map_v));
+        break;
+      case TypedValue::Type::Vertex:
+        new (&vertex_v) VertexAccessor(std::move(other.vertex_v));
+        break;
+      case TypedValue::Type::Edge:
+        new (&edge_v) EdgeAccessor(std::move(other.edge_v));
+        break;
+      case TypedValue::Type::Path:
+        new (&path_v) Path(std::move(other.path_v));
+        break;
+    }
+
+    other = TypedValue::Null;
+  }
+
+  return *this;
+}
+
 const TypedValue TypedValue::Null = TypedValue();
 
-TypedValue::~TypedValue() {
+void TypedValue::DestroyValue() {
   switch (type_) {
     // destructor for primitive types does nothing
-    case Type::Null:
+  case Type::Null:
-    case Type::Bool:
+  case Type::Bool:
-    case Type::Int:
+  case Type::Int:
-    case Type::Double:
+  case Type::Double:
-      return;
+    break;
 
     // we need to call destructors for non primitive types since we used
     // placement new
-    case Type::String:
+  case Type::String:
-      // Clang fails to compile ~std::string. It seems it is a bug in some
+    // Clang fails to compile ~std::string. It seems it is a bug in some
-      // versions of clang. using namespace std statement solves the issue.
+    // versions of clang. using namespace std statement solves the issue.
-      using namespace std;
+    using namespace std;
-      string_v.~string();
+    string_v.~string();
-      return;
+    break;
-    case Type::List:
+  case Type::List:
-      using namespace std;
+    using namespace std;
-      list_v.~vector<TypedValue>();
+    list_v.~vector<TypedValue>();
-      return;
+    break;
-    case Type::Map:
+  case Type::Map:
-      using namespace std;
+    using namespace std;
-      map_v.~map<std::string, TypedValue>();
+    map_v.~map<std::string, TypedValue>();
-      return;
+    break;
-    case Type::Vertex:
+  case Type::Vertex:
-      vertex_v.~VertexAccessor();
+    vertex_v.~VertexAccessor();
-      return;
+    break;
-    case Type::Edge:
+  case Type::Edge:
-      edge_v.~EdgeAccessor();
+    edge_v.~EdgeAccessor();
-      return;
+    break;
-    case Type::Path:
+  case Type::Path:
-      path_v.~Path();
+    path_v.~Path();
-      return;
+    break;
   }
-  LOG(FATAL) << "Unsupported TypedValue::Type";
+
+  type_ = TypedValue::Type::Null;
+}
+
+TypedValue::~TypedValue() {
+  DestroyValue();
 }
 
 /**
@@ -651,7 +803,7 @@ bool TypedValue::BoolEqual::operator()(const TypedValue &lhs,
     default:
       LOG(FATAL)
           << "Equality between two TypedValues resulted in something other "
-             "then Null or bool";
+             "than Null or bool";
   }
 }
 

src/query/typed_value.hpp

@@ -6,6 +6,7 @@
 #include <memory>
 #include <string>
 #include <unordered_set>
+#include <utility>
 #include <vector>
 
 #include "query/path.hpp"
@@ -55,6 +56,7 @@ class TypedValue
    * in the floating point domain (TypedValue operator== behaves the same).
    * */
   using unordered_set = std::unordered_set<TypedValue, Hash, BoolEqual>;
+  using value_map_t = std::map<std::string, TypedValue>;
 
   /** Private default constructor, makes Null */
   TypedValue() : type_(Type::Null) {}
@@ -77,6 +79,9 @@ class TypedValue
   // single static reference to Null, used whenever Null should be returned
   static const TypedValue Null;
 
+  TypedValue(const TypedValue &other);
+  TypedValue(TypedValue &&other);
+
   // constructors for primitive types
   TypedValue(bool value) : type_(Type::Bool) { bool_v = value; }
   TypedValue(int value) : type_(Type::Int) { int_v = value; }
@@ -86,7 +91,7 @@ class TypedValue
   // conversion function to PropertyValue
   explicit operator PropertyValue() const;
 
-  /// constructors for non-primitive types
+  // copy constructors for non-primitive types
   TypedValue(const std::string &value) : type_(Type::String) {
     new (&string_v) std::string(value);
   }
@@ -109,33 +114,50 @@ class TypedValue
   TypedValue(const Path &path) : type_(Type::Path) { new (&path_v) Path(path); }
   TypedValue(const PropertyValue &value);
 
-/**
+  // move constructors for non-primitive types
- * There are all sorts of explicit assignments here because this way we avoid
+  TypedValue(std::string &&value) : type_(Type::String) {
- * destructor and constructor of TypedValue for creating intermediary values,
+    new (&string_v) std::string(std::move(value));
- * and can fill the typed value storage directly if it has the same underlying
+  }
- * type.
+  TypedValue(std::vector<TypedValue> &&value) : type_(Type::List) {
- */
+    new (&list_v) std::vector<TypedValue>(std::move(value));
-#define DECLARE_TYPED_VALUE_ASSIGNMENT(type_param) \
+  }
-  TypedValue &operator=(const type_param &other);
+  TypedValue(std::map<std::string, TypedValue> &&value) : type_(Type::Map) {
+    new (&map_v) std::map<std::string, TypedValue>(std::move(value));
+  }
+  TypedValue(VertexAccessor &&vertex) : type_(Type::Vertex) {
+    new (&vertex_v) VertexAccessor(std::move(vertex));
+  }
+  TypedValue(EdgeAccessor &&edge) : type_(Type::Edge) {
+    new (&edge_v) EdgeAccessor(std::move(edge));
+  }
+  TypedValue(Path &&path) : type_(Type::Path) {
+    new (&path_v) Path(std::move(path));
+  }
+  TypedValue(PropertyValue &&value);
 
-  using value_map_t = std::map<std::string, TypedValue>;
+  // copy assignment operators
-  // Don't delete char * const assignment because char* strings will be assigned
+  TypedValue &operator=(const char *);
-  // using boolean assignment (not good).
+  TypedValue &operator=(int);
-  DECLARE_TYPED_VALUE_ASSIGNMENT(char *const)
+  TypedValue &operator=(bool);
-  DECLARE_TYPED_VALUE_ASSIGNMENT(int)
+  TypedValue &operator=(int64_t);
-  DECLARE_TYPED_VALUE_ASSIGNMENT(bool)
+  TypedValue &operator=(double);
-  DECLARE_TYPED_VALUE_ASSIGNMENT(int64_t)
+  TypedValue &operator=(const std::string &);
-  DECLARE_TYPED_VALUE_ASSIGNMENT(double)
+  TypedValue &operator=(const std::vector<TypedValue> &);
-  DECLARE_TYPED_VALUE_ASSIGNMENT(std::string)
+  TypedValue &operator=(const TypedValue::value_map_t &);
-  DECLARE_TYPED_VALUE_ASSIGNMENT(std::vector<TypedValue>)
+  TypedValue &operator=(const VertexAccessor &);
-  DECLARE_TYPED_VALUE_ASSIGNMENT(TypedValue::value_map_t)
+  TypedValue &operator=(const EdgeAccessor &);
-  DECLARE_TYPED_VALUE_ASSIGNMENT(VertexAccessor)
+  TypedValue &operator=(const Path &);
-  DECLARE_TYPED_VALUE_ASSIGNMENT(EdgeAccessor)
+  TypedValue &operator=(const TypedValue &);
-  DECLARE_TYPED_VALUE_ASSIGNMENT(Path)
+
-  DECLARE_TYPED_VALUE_ASSIGNMENT(TypedValue)
+  // move assignment operators
-#undef DECLARE_TYPED_VALUE_ASSIGNMENT
+  TypedValue &operator=(std::string &&);
+  TypedValue &operator=(std::vector<TypedValue> &&);
+  TypedValue &operator=(TypedValue::value_map_t &&);
+  TypedValue &operator=(VertexAccessor &&);
+  TypedValue &operator=(EdgeAccessor &&);
+  TypedValue &operator=(Path &&);
+  TypedValue &operator=(TypedValue &&);
 
-  TypedValue(const TypedValue &other);
   ~TypedValue();
 
   Type type() const { return type_; }
@@ -189,6 +211,8 @@ class TypedValue
   friend std::ostream &operator<<(std::ostream &stream, const TypedValue &prop);
 
  private:
+  void DestroyValue();
+
   // storage for the value of the property
   union {
     bool bool_v;

src/storage/common/property_value.hpp

@@ -10,8 +10,8 @@
 #include "utils/total_ordering.hpp"
 
 /**
- * Encapsulation of a value and it's type encapsulated in a class that has no
+ * Encapsulation of a value and its type in a class that has no compile-time
- * compiled-time info about that type.
+ * info about the type.
  *
  * Values can be of a number of predefined types that are enumerated in
  * PropertyValue::Type. Each such type corresponds to exactly one C++ type.