Reduce the size of TypedValue (#560)

- Reduce the size of TypedValue
- Fix double allocation
- Add `Graph` to `TypedValue` unit tests
- Fix allocator usage in `TypedValue`
- Add graph projection to `long_running.cpp` stress test

src/query/typed_value.cpp

@@ -22,6 +22,7 @@
 #include "storage/v2/temporal.hpp"
 #include "utils/exceptions.hpp"
 #include "utils/fnv.hpp"
+#include "utils/memory.hpp"
 
 namespace memgraph::query {
 
@@ -215,7 +216,8 @@ TypedValue::TypedValue(const TypedValue &other, utils::MemoryResource *memory) :
       new (&duration_v) utils::Duration(other.duration_v);
       return;
     case Type::Graph:
-      new (&graph_v) Graph(other.graph_v, memory_);
+      auto *graph_ptr = utils::Allocator<Graph>(memory_).new_object<Graph>(*other.graph_v);
+      new (&graph_v) std::unique_ptr<Graph>(graph_ptr);
       return;
   }
   LOG_FATAL("Unsupported TypedValue::Type");
@@ -267,7 +269,12 @@ TypedValue::TypedValue(TypedValue &&other, utils::MemoryResource *memory) : memo
       new (&duration_v) utils::Duration(other.duration_v);
       break;
     case Type::Graph:
-      new (&graph_v) Graph(std::move(other.graph_v), memory_);
+      if (other.GetMemoryResource() == memory_) {
+        new (&graph_v) std::unique_ptr<Graph>(std::move(other.graph_v));
+      } else {
+        auto *graph_ptr = utils::Allocator<Graph>(memory_).new_object<Graph>(std::move(*other.graph_v));
+        new (&graph_v) std::unique_ptr<Graph>(graph_ptr);
+      }
   }
   other.DestroyValue();
 }
@@ -336,7 +343,22 @@ DEFINE_VALUE_AND_TYPE_GETTERS(utils::Date, Date, date_v)
 DEFINE_VALUE_AND_TYPE_GETTERS(utils::LocalTime, LocalTime, local_time_v)
 DEFINE_VALUE_AND_TYPE_GETTERS(utils::LocalDateTime, LocalDateTime, local_date_time_v)
 DEFINE_VALUE_AND_TYPE_GETTERS(utils::Duration, Duration, duration_v)
-DEFINE_VALUE_AND_TYPE_GETTERS(Graph, Graph, graph_v)
+
+Graph &TypedValue::ValueGraph() {
+  if (type_ != Type::Graph) {
+    throw TypedValueException("TypedValue is of type '{}', not '{}'", type_, Type::Graph);
+  }
+  return *graph_v;
+}
+
+const Graph &TypedValue::ValueGraph() const {
+  if (type_ != Type::Graph) {
+    throw TypedValueException("TypedValue is of type '{}', not '{}'", type_, Type::Graph);
+  }
+  return *graph_v;
+}
+
+bool TypedValue::IsGraph() const { return type_ == Type::Graph; }
 
 #undef DEFINE_VALUE_AND_TYPE_GETTERS
 
@@ -530,9 +552,11 @@ TypedValue &TypedValue::operator=(const TypedValue &other) {
       case TypedValue::Type::Path:
         new (&path_v) Path(other.path_v, memory_);
         return *this;
-      case TypedValue::Type::Graph:
+      case TypedValue::Type::Graph: {
-        new (&graph_v) Graph(other.graph_v, memory_);
+        auto *graph_ptr = utils::Allocator<Graph>(memory_).new_object<Graph>(*other.graph_v);
+        new (&graph_v) std::unique_ptr<Graph>(graph_ptr);
         return *this;
+      }
       case Type::Date:
         new (&date_v) utils::Date(other.date_v);
         return *this;
@@ -605,7 +629,12 @@ TypedValue &TypedValue::operator=(TypedValue &&other) noexcept(false) {
         new (&duration_v) utils::Duration(other.duration_v);
         break;
       case Type::Graph:
-        new (&graph_v) Graph(std::move(other.graph_v), memory_);
+        if (other.GetMemoryResource() == memory_) {
+          new (&graph_v) std::unique_ptr<Graph>(std::move(other.graph_v));
+        } else {
+          auto *graph_ptr = utils::Allocator<Graph>(memory_).new_object<Graph>(std::move(*other.graph_v));
+          new (&graph_v) std::unique_ptr<Graph>(graph_ptr);
+        }
         break;
     }
     other.DestroyValue();
@@ -625,32 +654,37 @@ void TypedValue::DestroyValue() {
       // we need to call destructors for non primitive types since we used
       // placement new
     case Type::String:
-      string_v.~TString();
+      std::destroy_at(&string_v);
       break;
     case Type::List:
-      list_v.~TVector();
+      std::destroy_at(&list_v);
       break;
     case Type::Map:
-      map_v.~TMap();
+      std::destroy_at(&map_v);
       break;
     case Type::Vertex:
-      vertex_v.~VertexAccessor();
+      std::destroy_at(&vertex_v);
       break;
     case Type::Edge:
-      edge_v.~EdgeAccessor();
+      std::destroy_at(&edge_v);
       break;
     case Type::Path:
-      path_v.~Path();
+      std::destroy_at(&path_v);
       break;
     case Type::Date:
     case Type::LocalTime:
     case Type::LocalDateTime:
     case Type::Duration:
       break;
-    case Type::Graph:
+    case Type::Graph: {
-      graph_v.~Graph();
+      auto *graph = graph_v.release();
+      std::destroy_at(&graph_v);
+      if (graph) {
+        utils::Allocator<Graph>(memory_).delete_object(graph);
+      }
       break;
     }
+  }
 
   type_ = TypedValue::Type::Null;
 }

src/query/typed_value.hpp

@@ -416,7 +416,8 @@ class TypedValue {
    * element-wise move and graph is not guaranteed to be empty.
    */
   TypedValue(Graph &&graph, utils::MemoryResource *memory) : memory_(memory), type_(Type::Graph) {
-    new (&graph_v) Graph(std::move(graph), memory_);
+    auto *graph_ptr = utils::Allocator<Graph>(memory_).new_object<Graph>(std::move(graph));
+    new (&graph_v) std::unique_ptr<Graph>(graph_ptr);
   }
 
   /**
@@ -547,7 +548,8 @@ class TypedValue {
     utils::LocalTime local_time_v;
     utils::LocalDateTime local_date_time_v;
     utils::Duration duration_v;
-    Graph graph_v;
+    // As the unique_ptr is not allocator aware, it requires special attention when copying or moving graphs
+    std::unique_ptr<Graph> graph_v;
   };
 
   /**

tests/stress/long_running.cpp

@@ -254,6 +254,22 @@ class GraphSession {
     Execute(fmt::format("MATCH ()-[e]->() WHERE e.id > {} AND e.id < {} SET e.value = {}", lo, hi, num));
   }
 
+  void CheckGraphProjection() {
+    uint64_t vertex_id = *vertices_.rbegin();
+    uint64_t lo = std::floor(GetRandom() * vertex_id);
+    uint64_t hi = std::floor(lo + vertex_id * 0.01);
+
+    Execute(fmt::format(
+        "MATCH p=()-[e]->() WHERE e.id > {} AND e.id < {} WITH project(p) as graph WITH graph.nodes as nodes "
+        "UNWIND nodes as n RETURN n.x "
+        "as x ORDER BY x DESC",
+        lo, hi));
+    Execute(fmt::format(
+        "MATCH p=()-[e]->() WHERE e.id > {} AND e.id < {} WITH project(p) as graph WITH graph.edges as edges "
+        "UNWIND edges as e RETURN e.prop as y ORDER BY y DESC",
+        lo, hi));
+  }
+
   /** Checks if the local info corresponds to DB state */
   void VerifyGraph() {
     // helper lambda for set verification
@@ -357,6 +373,7 @@ class GraphSession {
       } else {
         CreateVertices(1);
       }
+      CheckGraphProjection();
     }
 
     // final verification

tests/unit/typed_value.cpp

@@ -20,6 +20,7 @@
 
 #include "gtest/gtest.h"
 
+#include "query/graph.hpp"
 #include "query/typed_value.hpp"
 #include "storage/v2/storage.hpp"
 
@@ -48,8 +49,13 @@ class AllTypesFixture : public testing::Test {
                                                            {"e", TypedValue()}});
     auto vertex = dba.InsertVertex();
     values_.emplace_back(vertex);
-    values_.emplace_back(*dba.InsertEdge(&vertex, &vertex, dba.NameToEdgeType("et")));
+    auto edge = *dba.InsertEdge(&vertex, &vertex, dba.NameToEdgeType("et"));
+    values_.emplace_back(edge);
     values_.emplace_back(memgraph::query::Path(dba.InsertVertex()));
+    memgraph::query::Graph graph{memgraph::utils::NewDeleteResource()};
+    graph.InsertVertex(vertex);
+    graph.InsertEdge(edge);
+    values_.emplace_back(std::move(graph));
   }
 };
 
@@ -533,6 +539,18 @@ TEST_F(AllTypesFixture, PropagationOfMemoryOnConstruction) {
       EXPECT_EQ(copied.vertices(), original.vertices());
       EXPECT_EQ(copied.edges(), original.edges());
       EXPECT_EQ(copied.GetMemoryResource(), &monotonic_memory);
+    } else if (value.type() == TypedValue::Type::Graph) {
+      ASSERT_EQ(move_constructed_value.type(), value.type());
+      const auto &original = value.ValueGraph();
+      const auto &moved = move_constructed_value.ValueGraph();
+      const auto &copied = copy_constructed_value.ValueGraph();
+      EXPECT_EQ(original.GetMemoryResource(), memgraph::utils::NewDeleteResource());
+      EXPECT_EQ(moved.vertices(), original.vertices());
+      EXPECT_EQ(moved.edges(), original.edges());
+      EXPECT_EQ(moved.GetMemoryResource(), &monotonic_memory);
+      EXPECT_EQ(copied.vertices(), original.vertices());
+      EXPECT_EQ(copied.edges(), original.edges());
+      EXPECT_EQ(copied.GetMemoryResource(), &monotonic_memory);
     }
   }
 }