Use utils::Allocator in query::Path

Reviewers: mtomic, llugovic, mferencevic

Reviewed By: mtomic, llugovic

Subscribers: pullbot

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

src/query/distributed/serialization.cpp

@@ -141,19 +141,21 @@ void Load(query::TypedValue *value, slk::Reader *reader,
     case static_cast<uint8_t>(9): {
       size_t v_size;
       slk::Load(&v_size, reader);
-      std::vector<VertexAccessor> vertices;
+      auto *memory = value->GetMemoryResource();
+      std::vector<VertexAccessor, utils::Allocator<VertexAccessor>> vertices(
+          memory);
       vertices.reserve(v_size);
       for (size_t i = 0; i < v_size; ++i) {
         vertices.push_back(slk::LoadVertexAccessor(reader, dba, data_manager));
       }
       size_t e_size;
       slk::Load(&e_size, reader);
-      std::vector<EdgeAccessor> edges;
+      std::vector<EdgeAccessor, utils::Allocator<EdgeAccessor>> edges(memory);
       edges.reserve(e_size);
       for (size_t i = 0; i < e_size; ++i) {
         edges.push_back(slk::LoadEdgeAccessor(reader, dba, data_manager));
       }
-      query::Path path(vertices[0]);
+      query::Path path(vertices[0], memory);
       path.vertices() = std::move(vertices);
       path.edges() = std::move(edges);
       *value = std::move(path);

src/query/path.hpp

@@ -7,6 +7,7 @@
 
 #include "storage/edge_accessor.hpp"
 #include "storage/vertex_accessor.hpp"
+#include "utils/memory.hpp"
 
 namespace query {
 
@@ -17,17 +18,89 @@ namespace query {
  */
 class Path {
  public:
-  /** Creates the path starting with the given vertex. */
+  /** Allocator type so that STL containers are aware that we need one */
-  explicit Path(const VertexAccessor &vertex) { Expand(vertex); }
+  using allocator_type = utils::Allocator<char>;
 
-  /** Creates the path starting with the given vertex and containing all other
+  /**
-   * elements. */
+   * Create the path starting with the given vertex.
+   * Allocations are done using the given MemoryResource.
+   */
+  explicit Path(const VertexAccessor &vertex,
+                utils::MemoryResource *memory = utils::NewDeleteResource())
+      : vertices_(memory), edges_(memory) {
+    Expand(vertex);
+  }
+
+  /**
+   * Create the path starting with the given vertex and containing all other
+   * elements.
+   * Allocations are done using the default utils::NewDeleteResource().
+   */
   template <typename... TOthers>
-  Path(const VertexAccessor &vertex, const TOthers &... others) {
+  explicit Path(const VertexAccessor &vertex, const TOthers &... others)
+      : vertices_(utils::NewDeleteResource()),
+        edges_(utils::NewDeleteResource()) {
     Expand(vertex);
     Expand(others...);
   }
 
+  /**
+   * Create the path starting with the given vertex and containing all other
+   * elements.
+   * Allocations are done using the given MemoryResource.
+   */
+  template <typename... TOthers>
+  Path(std::allocator_arg_t, utils::MemoryResource *memory,
+       const VertexAccessor &vertex, const TOthers &... others)
+      : vertices_(memory), edges_(memory) {
+    Expand(vertex);
+    Expand(others...);
+  }
+
+  /**
+   * Construct a copy of other.
+   * utils::MemoryResource is obtained by calling
+   * std::allocator_traits<>::
+   *     select_on_container_copy_construction(other.GetMemoryResource()).
+   * Since we use utils::Allocator, which does not propagate, this means that we
+   * will default to utils::NewDeleteResource().
+   */
+  Path(const Path &other)
+      : Path(other, std::allocator_traits<allocator_type>::
+                        select_on_container_copy_construction(
+                            other.GetMemoryResource())
+                            .GetMemoryResource()) {}
+
+  /** Construct a copy using the given utils::MemoryResource */
+  Path(const Path &other, utils::MemoryResource *memory)
+      : vertices_(other.vertices_, memory), edges_(other.edges_, memory) {}
+
+  /**
+   * Construct with the value of other.
+   * utils::MemoryResource is obtained from other. After the move, other will be
+   * empty.
+   */
+  Path(Path &&other) noexcept
+      : Path(std::move(other), other.GetMemoryResource()) {}
+
+  /**
+   * Construct with the value of other, but use the given utils::MemoryResource.
+   * After the move, other may not be empty if `*memory !=
+   * *other.GetMemoryResource()`, because an element-wise move will be
+   * performed.
+   */
+  Path(Path &&other, utils::MemoryResource *memory)
+      : vertices_(std::move(other.vertices_), memory),
+        edges_(std::move(other.edges_), memory) {}
+
+  /** Copy assign other, utils::MemoryResource of `this` is used */
+  Path &operator=(const Path &) = default;
+
+  /** Move assign other, utils::MemoryResource of `this` is used. */
+  Path &operator=(Path &&) = default;
+
+  ~Path() = default;
+
   /** Expands the path with the given vertex. */
   void Expand(const VertexAccessor &vertex) {
     DCHECK(vertices_.size() == edges_.size())
@@ -57,6 +130,10 @@ class Path {
   const auto &vertices() const { return vertices_; }
   const auto &edges() const { return edges_; }
 
+  utils::MemoryResource *GetMemoryResource() const {
+    return vertices_.get_allocator().GetMemoryResource();
+  }
+
   bool operator==(const Path &other) const {
     return vertices_ == other.vertices_ && edges_ == other.edges_;
   }
@@ -90,8 +167,9 @@ class Path {
 
  private:
   // Contains all the vertices in the path.
-  std::vector<VertexAccessor> vertices_;
+  std::vector<VertexAccessor, utils::Allocator<VertexAccessor>> vertices_;
   // Contains all the edges in the path (one less then there are vertices).
-  std::vector<EdgeAccessor> edges_;
+  std::vector<EdgeAccessor, utils::Allocator<EdgeAccessor>> edges_;
 };
+
 }  // namespace query

src/query/typed_value.cpp

@@ -143,7 +143,7 @@ TypedValue::TypedValue(const TypedValue &other, utils::MemoryResource *memory)
       new (&edge_v) EdgeAccessor(other.edge_v);
       return;
     case Type::Path:
-      new (&path_v) Path(other.path_v);
+      new (&path_v) Path(other.path_v, memory_);
       return;
   }
   LOG(FATAL) << "Unsupported TypedValue::Type";
@@ -182,7 +182,7 @@ TypedValue::TypedValue(TypedValue &&other, utils::MemoryResource *memory)
       new (&edge_v) EdgeAccessor(std::move(other.edge_v));
       break;
     case Type::Path:
-      new (&path_v) Path(std::move(other.path_v));
+      new (&path_v) Path(std::move(other.path_v), memory_);
       break;
   }
   other = TypedValue::Null;
@@ -457,7 +457,7 @@ TypedValue &TypedValue::operator=(const TypedValue &other) {
         new (&edge_v) EdgeAccessor(other.edge_v);
         return *this;
       case TypedValue::Type::Path:
-        new (&path_v) Path(other.path_v);
+        new (&path_v) Path(other.path_v, memory_);
         return *this;
     }
     LOG(FATAL) << "Unsupported TypedValue::Type";
@@ -505,7 +505,7 @@ TypedValue &TypedValue::operator=(TypedValue &&other) noexcept(false) {
         new (&edge_v) EdgeAccessor(std::move(other.edge_v));
         break;
       case TypedValue::Type::Path:
-        new (&path_v) Path(std::move(other.path_v));
+        new (&path_v) Path(std::move(other.path_v), memory_);
         break;
     }
     other = TypedValue::Null;
@@ -927,12 +927,13 @@ size_t TypedValue::Hash::operator()(const TypedValue &value) const {
       return value.Value<VertexAccessor>().gid();
     case TypedValue::Type::Edge:
       return value.Value<EdgeAccessor>().gid();
-    case TypedValue::Type::Path:
+    case TypedValue::Type::Path: {
-      return utils::FnvCollection<std::vector<VertexAccessor>,
+      const auto &vertices = value.ValuePath().vertices();
-                                  VertexAccessor>{}(
+      const auto &edges = value.ValuePath().edges();
-                 value.ValuePath().vertices()) ^
+      return utils::FnvCollection<decltype(vertices), VertexAccessor>{}(
-             utils::FnvCollection<std::vector<EdgeAccessor>, EdgeAccessor>{}(
+                 vertices) ^
-                 value.ValuePath().edges());
+             utils::FnvCollection<decltype(edges), EdgeAccessor>{}(edges);
+    }
   }
   LOG(FATAL) << "Unhandled TypedValue.type() in hash function";
 }

src/query/typed_value.hpp

@@ -266,7 +266,7 @@ class TypedValue {
   TypedValue(const Path &path,
              utils::MemoryResource *memory = utils::NewDeleteResource())
       : memory_(memory), type_(Type::Path) {
-    new (&path_v) Path(path);
+    new (&path_v) Path(path, memory_);
   }
 
   /** Construct a copy using default utils::NewDeleteResource() */
@@ -397,13 +397,22 @@ class TypedValue {
     new (&edge_v) EdgeAccessor(std::move(edge));
   }
 
-  TypedValue(Path &&path) noexcept : type_(Type::Path) {
+  /**
-    new (&path_v) Path(std::move(path));
+   * Construct with the value of path.
-  }
+   * utils::MemoryResource is obtained from path. After the move, path will be
+   * left empty.
+   */
+  TypedValue(Path &&path) noexcept
+      : TypedValue(std::move(path), path.GetMemoryResource()) {}
 
-  TypedValue(Path &&path, utils::MemoryResource *memory) noexcept
+  /**
+   * Construct with the value of path and use the given MemoryResource.
+   * If `*path.GetMemoryResource() != *memory`, this call will perform an
+   * element-wise move and path is not guaranteed to be empty.
+   */
+  TypedValue(Path &&path, utils::MemoryResource *memory)
       : memory_(memory), type_(Type::Path) {
-    new (&path_v) Path(std::move(path));
+    new (&path_v) Path(std::move(path), memory_);
   }
 
   /**

tests/unit/query_plan_match_filter_return.cpp

@@ -447,7 +447,8 @@ TEST_F(ExpandFixture, ExpandPath) {
           ->MapTo(symbol_table.CreateSymbol("named_expression_1", true));
   auto produce = MakeProduce(path, output);
 
-  std::vector<query::Path> expected_paths{{v1, r2, v3}, {v1, r1, v2}};
+  std::vector<query::Path> expected_paths{query::Path(v1, r2, v3),
+                                          query::Path(v1, r1, v2)};
   auto context = MakeContext(storage, symbol_table, &dba_);
   auto results = CollectProduce(*produce, &context);
   ASSERT_EQ(results.size(), 2);

tests/unit/typed_value.cpp

@@ -511,6 +511,18 @@ TEST_F(AllTypesFixture, PropagationOfMemoryOnConstruction) {
         EXPECT_TRUE(TypedValue::BoolEqual{}(kv.second, moved_it->second));
         EXPECT_TRUE(TypedValue::BoolEqual{}(kv.second, copied_it->second));
       }
+    } else if (value.type() == TypedValue::Type::Path) {
+      ASSERT_EQ(move_constructed_value.type(), value.type());
+      const auto &original = value.ValuePath();
+      const auto &moved = move_constructed_value.ValuePath();
+      const auto &copied = copy_constructed_value.ValuePath();
+      EXPECT_EQ(original.GetMemoryResource(), 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);
     }
   }
 }