diff --git a/src/query/plan/operator.cpp b/src/query/plan/operator.cpp index f52b76ef9..44cbbdd81 100644 --- a/src/query/plan/operator.cpp +++ b/src/query/plan/operator.cpp @@ -822,24 +822,25 @@ class ExpandVariableCursor : public Cursor { } // Helper function for appending an edge to the list on the frame. - void AppendEdge(const EdgeAccessor &new_edge, - std::vector &edges_on_frame) { + void AppendEdge( + const EdgeAccessor &new_edge, + std::vector> *edges_on_frame) { // We are placing an edge on the frame. It is possible that there already // exists an edge on the frame for this level. If so first remove it. DCHECK(edges_.size() > 0) << "Edges are empty"; if (self_.is_reverse_) { // TODO: This is innefficient, we should look into replacing // vector with something else for TypedValue::List. - size_t diff = edges_on_frame.size() - - std::min(edges_on_frame.size(), edges_.size() - 1U); + size_t diff = edges_on_frame->size() - + std::min(edges_on_frame->size(), edges_.size() - 1U); if (diff > 0U) - edges_on_frame.erase(edges_on_frame.begin(), - edges_on_frame.begin() + diff); - edges_on_frame.insert(edges_on_frame.begin(), new_edge); + edges_on_frame->erase(edges_on_frame->begin(), + edges_on_frame->begin() + diff); + edges_on_frame->insert(edges_on_frame->begin(), new_edge); } else { - edges_on_frame.resize( - std::min(edges_on_frame.size(), edges_.size() - 1U)); - edges_on_frame.emplace_back(new_edge); + edges_on_frame->resize( + std::min(edges_on_frame->size(), edges_.size() - 1U)); + edges_on_frame->emplace_back(new_edge); } } @@ -872,8 +873,7 @@ class ExpandVariableCursor : public Cursor { if (edges_.empty()) return false; // we use this a lot - std::vector &edges_on_frame = - frame[self_.common_.edge_symbol].ValueList(); + auto &edges_on_frame = frame[self_.common_.edge_symbol].ValueList(); // it is possible that edges_on_frame does not contain as many // elements as edges_ due to edge-uniqueness (when a whole layer @@ -903,7 +903,7 @@ class ExpandVariableCursor : public Cursor { }); if (found_existing) continue; - AppendEdge(current_edge.first, edges_on_frame); + AppendEdge(current_edge.first, &edges_on_frame); VertexAccessor current_vertex = current_edge.second == EdgeAtom::Direction::IN ? current_edge.first.from() @@ -1670,7 +1670,7 @@ class ConstructNamedPathCursor : public Cursor { last_was_edge_list = true; // We need to expand all edges in the list and intermediary // vertices. - const std::vector &edges = expansion.ValueList(); + const auto &edges = expansion.ValueList(); for (const auto &edge_value : edges) { const EdgeAccessor &edge = edge_value.ValueEdge(); const VertexAccessor from = edge.from(); diff --git a/src/query/typed_value.cpp b/src/query/typed_value.cpp index e38e318c8..70cefb9c0 100644 --- a/src/query/typed_value.cpp +++ b/src/query/typed_value.cpp @@ -43,8 +43,10 @@ TypedValue::TypedValue(const PropertyValue &value, return; case PropertyValue::Type::List: { type_ = Type::List; - auto vec = value.Value>(); - new (&list_v) std::vector(vec.begin(), vec.end()); + const auto &vec = value.Value>(); + new (&list_v) TVector(memory_); + list_v.reserve(vec.size()); + list_v.assign(vec.begin(), vec.end()); return; } case PropertyValue::Type::Map: { @@ -86,9 +88,10 @@ TypedValue::TypedValue(PropertyValue &&other, utils::MemoryResource *memory) case PropertyValue::Type::List: { type_ = Type::List; auto &vec = other.Value>(); - new (&list_v) - std::vector(std::make_move_iterator(vec.begin()), - std::make_move_iterator(vec.end())); + new (&list_v) TVector(memory_); + list_v.reserve(vec.size()); + list_v.assign(std::make_move_iterator(vec.begin()), + std::make_move_iterator(vec.end())); break; } case PropertyValue::Type::Map: { @@ -127,7 +130,7 @@ TypedValue::TypedValue(const TypedValue &other, utils::MemoryResource *memory) new (&string_v) std::string(other.string_v); return; case Type::List: - new (&list_v) std::vector(other.list_v); + new (&list_v) TVector(other.list_v, memory_); return; case Type::Map: new (&map_v) std::map(other.map_v); @@ -166,7 +169,7 @@ TypedValue::TypedValue(TypedValue &&other, utils::MemoryResource *memory) new (&string_v) std::string(std::move(other.string_v)); break; case Type::List: - new (&list_v) std::vector(std::move(other.list_v)); + new (&list_v) TVector(std::move(other.list_v), memory_); break; case Type::Map: new (&map_v) std::map(std::move(other.map_v)); @@ -181,7 +184,6 @@ TypedValue::TypedValue(TypedValue &&other, utils::MemoryResource *memory) new (&path_v) Path(std::move(other.path_v)); break; } - other = TypedValue::Null; } @@ -241,7 +243,7 @@ DEFINE_VALUE_AND_TYPE_GETTERS(bool, Bool, bool_v) DEFINE_VALUE_AND_TYPE_GETTERS(int64_t, Int, int_v) DEFINE_VALUE_AND_TYPE_GETTERS(double, Double, double_v) DEFINE_VALUE_AND_TYPE_GETTERS(std::string, String, string_v) -DEFINE_VALUE_AND_TYPE_GETTERS(std::vector, List, list_v) +DEFINE_VALUE_AND_TYPE_GETTERS(TypedValue::TVector, List, list_v) DEFINE_VALUE_AND_TYPE_GETTERS(TypedValue::value_map_t, Map, map_v) DEFINE_VALUE_AND_TYPE_GETTERS(VertexAccessor, Vertex, vertex_v) DEFINE_VALUE_AND_TYPE_GETTERS(EdgeAccessor, Edge, edge_v) @@ -345,8 +347,18 @@ DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(bool, Bool, bool_v) DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(int64_t, Int, int_v) DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(double, Double, double_v) DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(const std::string &, String, string_v) -DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(const std::vector &, List, - list_v) +DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(const TypedValue::TVector &, List, list_v) + +TypedValue &TypedValue::operator=(const std::vector &other) { + if (type_ == Type::List) { + list_v.reserve(other.size()); + list_v.assign(other.begin(), other.end()); + } else { + *this = TypedValue(other, memory_); + } + return *this; +} + DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(const TypedValue::value_map_t &, Map, map_v) DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(const VertexAccessor &, Vertex, vertex_v) DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(const EdgeAccessor &, Edge, edge_v) @@ -356,7 +368,7 @@ DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(const Path &, Path, path_v) #define DEFINE_TYPED_VALUE_MOVE_ASSIGNMENT(type_param, typed_value_type, \ member) \ - TypedValue &TypedValue::operator=(type_param &&other) noexcept { \ + TypedValue &TypedValue::operator=(type_param &&other) { \ if (this->type_ == TypedValue::Type::typed_value_type) { \ this->member = std::move(other); \ } else { \ @@ -367,7 +379,18 @@ DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(const Path &, Path, path_v) } DEFINE_TYPED_VALUE_MOVE_ASSIGNMENT(std::string, String, string_v) -DEFINE_TYPED_VALUE_MOVE_ASSIGNMENT(std::vector, List, list_v) +DEFINE_TYPED_VALUE_MOVE_ASSIGNMENT(TypedValue::TVector, List, list_v) + +TypedValue &TypedValue::operator=(std::vector &&other) { + if (type_ == Type::List) { + list_v.assign(std::make_move_iterator(other.begin()), + std::make_move_iterator(other.end())); + } else { + *this = TypedValue(std::move(other), memory_); + } + return *this; +} + 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) @@ -387,7 +410,6 @@ TypedValue &TypedValue::operator=(const TypedValue &other) { "Allocator propagation not implemented"); DestroyValue(); type_ = other.type_; - switch (other.type_) { case TypedValue::Type::Null: return *this; @@ -404,7 +426,7 @@ TypedValue &TypedValue::operator=(const TypedValue &other) { new (&string_v) std::string(other.string_v); return *this; case TypedValue::Type::List: - new (&list_v) std::vector(other.list_v); + new (&list_v) TVector(other.list_v, memory_); return *this; case TypedValue::Type::Map: new (&map_v) std::map(other.map_v); @@ -424,7 +446,7 @@ TypedValue &TypedValue::operator=(const TypedValue &other) { return *this; } -TypedValue &TypedValue::operator=(TypedValue &&other) noexcept { +TypedValue &TypedValue::operator=(TypedValue &&other) noexcept(false) { if (this != &other) { DestroyValue(); // NOTE: STL uses @@ -436,7 +458,6 @@ TypedValue &TypedValue::operator=(TypedValue &&other) noexcept { propagate_on_container_move_assignment::value, "Allocator propagation not implemented"); type_ = other.type_; - switch (other.type_) { case TypedValue::Type::Null: break; @@ -453,7 +474,7 @@ TypedValue &TypedValue::operator=(TypedValue &&other) noexcept { new (&string_v) std::string(std::move(other.string_v)); break; case TypedValue::Type::List: - new (&list_v) std::vector(std::move(other.list_v)); + new (&list_v) TVector(std::move(other.list_v), memory_); break; case TypedValue::Type::Map: new (&map_v) std::map(std::move(other.map_v)); @@ -468,10 +489,8 @@ TypedValue &TypedValue::operator=(TypedValue &&other) noexcept { new (&path_v) Path(std::move(other.path_v)); break; } - other = TypedValue::Null; } - return *this; } @@ -495,8 +514,7 @@ void TypedValue::DestroyValue() { string_v.~string(); break; case Type::List: - using namespace std; - list_v.~vector(); + list_v.~TVector(); break; case Type::Map: using namespace std; @@ -717,7 +735,7 @@ TypedValue operator+(const TypedValue &a, const TypedValue &b) { if (a.IsNull() || b.IsNull()) return TypedValue(a.GetMemoryResource()); if (a.IsList() || b.IsList()) { - std::vector list; + TypedValue::TVector list(a.GetMemoryResource()); auto append_list = [&list](const TypedValue &v) { if (v.IsList()) { auto list2 = v.ValueList(); @@ -728,7 +746,7 @@ TypedValue operator+(const TypedValue &a, const TypedValue &b) { }; append_list(a); append_list(b); - return TypedValue(list, a.GetMemoryResource()); + return TypedValue(std::move(list), a.GetMemoryResource()); } EnsureArithmeticallyOk(a, b, true, "addition"); @@ -878,7 +896,7 @@ size_t TypedValue::Hash::operator()(const TypedValue &value) const { case TypedValue::Type::String: return std::hash{}(value.Value()); case TypedValue::Type::List: { - return utils::FnvCollection, TypedValue, Hash>{}( + return utils::FnvCollection{}( value.ValueList()); } case TypedValue::Type::Map: { diff --git a/src/query/typed_value.hpp b/src/query/typed_value.hpp index cbb38463e..3af89628a 100644 --- a/src/query/typed_value.hpp +++ b/src/query/typed_value.hpp @@ -20,11 +20,15 @@ namespace query { // TODO: Neo4j does overflow checking. Should we also implement it? /** - * Encapsulation of a value and it's type encapsulated in a class that has no - * compiled-time info about that type. + * Stores a query runtime value and its type. * * Values can be of a number of predefined types that are enumerated in * TypedValue::Type. Each such type corresponds to exactly one C++ type. + * + * Non-primitive value types perform additional memory allocations. To tune the + * allocation scheme, each TypedValue stores a MemoryResource for said + * allocations. When copying and moving TypedValue instances, take care that the + * appropriate MemoryResource is used. */ class TypedValue { public: @@ -70,6 +74,7 @@ class TypedValue { */ using unordered_set = std::unordered_set; using value_map_t = std::map; + using TVector = std::vector>; /** Allocator type so that STL containers are aware that we need one */ using allocator_type = utils::Allocator; @@ -154,10 +159,33 @@ class TypedValue { new (&string_v) std::string(value); } + /** Construct a copy using the given utils::MemoryResource */ TypedValue(const std::vector &value, utils::MemoryResource *memory = utils::NewDeleteResource()) : memory_(memory), type_(Type::List) { - new (&list_v) std::vector(value); + new (&list_v) TVector(memory_); + list_v.reserve(value.size()); + list_v.assign(value.begin(), value.end()); + } + + /** + * Construct a copy of other. + * utils::MemoryResource is obtained by calling + * std::allocator_traits<>:: + * select_on_container_copy_construction(other.get_allocator()). + * Since we use utils::Allocator, which does not propagate, this means that + * memory_ will be the default utils::NewDeleteResource(). + */ + TypedValue(const TVector &other) + : TypedValue(other, std::allocator_traits>:: + select_on_container_copy_construction( + other.get_allocator()) + .GetMemoryResource()) {} + + /** Construct a copy using the given utils::MemoryResource */ + TypedValue(const TVector &value, utils::MemoryResource *memory) + : memory_(memory), type_(Type::List) { + new (&list_v) TVector(value, memory_); } TypedValue(const std::map &value, @@ -200,14 +228,48 @@ class TypedValue { new (&string_v) std::string(std::move(value)); } - TypedValue(std::vector &&value) noexcept : type_(Type::List) { - new (&list_v) std::vector(std::move(value)); + /** + * Perform an element-wise move using default utils::NewDeleteResource(). + * Other will be not be empty, though elements may be Null. + */ + TypedValue(std::vector &&other) + : TypedValue(std::move(other), utils::NewDeleteResource()) {} + + /** + * Perform an element-wise move of the other and use the given MemoryResource. + * Other will be not be empty, though elements may be Null. + */ + TypedValue(std::vector &&other, utils::MemoryResource *memory) + : memory_(memory), type_(Type::List) { + new (&list_v) TVector(memory_); + list_v.reserve(other.size()); + // std::vector has std::allocator and there's no move + // constructor for std::vector using different allocator types. Since + // std::allocator is not propagated to elements, it is possible that some + // TypedValue elements have a MemoryResource that is the same as the one we + // are given. In such a case we would like to move those TypedValue + // instances, so we use move_iterator. + list_v.assign(std::make_move_iterator(other.begin()), + std::make_move_iterator(other.end())); } - TypedValue(std::vector &&value, - utils::MemoryResource *memory) noexcept + /** + * Construct with the value of other. + * utils::MemoryResource is obtained from other. After the move, other will be + * left empty. + */ + TypedValue(TVector &&other) noexcept + : TypedValue(std::move(other), + other.get_allocator().GetMemoryResource()) {} + + /** + * Construct with the value of other and use the given MemoryResource. + * If `other.get_allocator() != *memory`, this call will perform an + * element-wise move and other is not guaranteed to be empty. + */ + TypedValue(TVector &&other, utils::MemoryResource *memory) : memory_(memory), type_(Type::List) { - new (&list_v) std::vector(std::move(value)); + new (&list_v) TVector(std::move(other), memory_); } TypedValue(std::map &&value) noexcept @@ -269,6 +331,7 @@ class TypedValue { TypedValue &operator=(int64_t); TypedValue &operator=(double); TypedValue &operator=(const std::string &); + TypedValue &operator=(const TVector &); TypedValue &operator=(const std::vector &); TypedValue &operator=(const TypedValue::value_map_t &); TypedValue &operator=(const VertexAccessor &); @@ -279,15 +342,16 @@ class TypedValue { TypedValue &operator=(const TypedValue &other); // move assignment operators - TypedValue &operator=(std::string &&) noexcept; - TypedValue &operator=(std::vector &&) noexcept; - TypedValue &operator=(TypedValue::value_map_t &&) noexcept; - TypedValue &operator=(VertexAccessor &&) noexcept; - TypedValue &operator=(EdgeAccessor &&) noexcept; - TypedValue &operator=(Path &&) noexcept; + TypedValue &operator=(std::string &&); + TypedValue &operator=(TVector &&); + TypedValue &operator=(std::vector &&); + TypedValue &operator=(TypedValue::value_map_t &&); + TypedValue &operator=(VertexAccessor &&); + TypedValue &operator=(EdgeAccessor &&); + TypedValue &operator=(Path &&); /** Move assign other, utils::MemoryResource of `this` is used. */ - TypedValue &operator=(TypedValue &&other) noexcept; + TypedValue &operator=(TypedValue &&other) noexcept(false); ~TypedValue(); @@ -320,7 +384,18 @@ class TypedValue { DECLARE_VALUE_AND_TYPE_GETTERS(int64_t, Int) DECLARE_VALUE_AND_TYPE_GETTERS(double, Double) DECLARE_VALUE_AND_TYPE_GETTERS(std::string, String) - DECLARE_VALUE_AND_TYPE_GETTERS(std::vector, List) + + /** + * Get the list value. + * @throw TypedValueException if stored value is not a list. + */ + TVector &ValueList(); + + const TVector &ValueList() const; + + /** Check if the stored value is a list value */ + bool IsList() const; + DECLARE_VALUE_AND_TYPE_GETTERS(value_map_t, Map) DECLARE_VALUE_AND_TYPE_GETTERS(VertexAccessor, Vertex) DECLARE_VALUE_AND_TYPE_GETTERS(EdgeAccessor, Edge) @@ -358,7 +433,7 @@ class TypedValue { // complexity so it shouldn't be a problem. This is maybe even faster // because of data locality. std::string string_v; - std::vector list_v; + TVector list_v; // clang doesn't allow unordered_map to have incomplete type as value so we // we use map. std::map map_v; diff --git a/tests/unit/bfs_common.hpp b/tests/unit/bfs_common.hpp index 219b01ab5..41fa7f840 100644 --- a/tests/unit/bfs_common.hpp +++ b/tests/unit/bfs_common.hpp @@ -266,9 +266,10 @@ auto GetProp(const RecordAccessor &rec, std::string prop, // Checks if the given path is actually a path from source to sink and if all // of its edges exist in the given edge list. +template void CheckPath(database::GraphDbAccessor *dba, const VertexAccessor &source, const VertexAccessor &sink, - const std::vector &path, + const std::vector &path, const std::vector> &edges) { VertexAccessor curr = source; for (const auto &edge_tv : path) { @@ -276,7 +277,7 @@ void CheckPath(database::GraphDbAccessor *dba, const VertexAccessor &source, auto edge = edge_tv.ValueEdge(); ASSERT_TRUE(edge.from() == curr || edge.to() == curr); - auto next = edge.from_is(curr) ? edge.to() : edge.from(); + VertexAccessor next = edge.from_is(curr) ? edge.to() : edge.from(); int from = GetProp(curr, "id", dba).Value(); int to = GetProp(next, "id", dba).Value(); diff --git a/tests/unit/query_plan_match_filter_return.cpp b/tests/unit/query_plan_match_filter_return.cpp index 313cb6739..66f0ce01a 100644 --- a/tests/unit/query_plan_match_filter_return.cpp +++ b/tests/unit/query_plan_match_filter_return.cpp @@ -604,7 +604,8 @@ class QueryPlanExpandVariable : public testing::Test { Symbol symbol) { map_int count_per_length; for (const auto &edge_list : - GetResults>(input_op, symbol)) { + GetResults>>( + input_op, symbol)) { auto length = edge_list.size(); auto found = count_per_length.find(length); if (found == count_per_length.end()) diff --git a/tests/unit/typed_value.cpp b/tests/unit/typed_value.cpp index ddebba119..6b708db51 100644 --- a/tests/unit/typed_value.cpp +++ b/tests/unit/typed_value.cpp @@ -460,3 +460,36 @@ TEST_F(AllTypesFixture, AssignmentWithMemoryResource) { utils::NewDeleteResource()); } } + +// NOLINTNEXTLINE(hicpp-special-member-functions) +TEST_F(AllTypesFixture, PropagationOfMemoryOnConstruction) { + utils::MonotonicBufferResource monotonic_memory(1024); + std::vector> + values_with_custom_memory(&monotonic_memory); + for (const auto &value : values_) { + EXPECT_EQ(value.GetMemoryResource(), utils::NewDeleteResource()); + values_with_custom_memory.emplace_back(value); + const auto ©_constructed_value = values_with_custom_memory.back(); + EXPECT_EQ(copy_constructed_value.GetMemoryResource(), &monotonic_memory); + TypedValue copy(values_with_custom_memory.back()); + EXPECT_EQ(copy.GetMemoryResource(), utils::NewDeleteResource()); + values_with_custom_memory.emplace_back(std::move(copy)); + const auto &move_constructed_value = values_with_custom_memory.back(); + EXPECT_EQ(move_constructed_value.GetMemoryResource(), &monotonic_memory); + if (value.type() == TypedValue::Type::List) { + ASSERT_EQ(move_constructed_value.type(), value.type()); + const auto &original = value.ValueList(); + const auto &moved = move_constructed_value.ValueList(); + const auto &copied = copy_constructed_value.ValueList(); + ASSERT_EQ(moved.size(), original.size()); + ASSERT_EQ(copied.size(), original.size()); + for (size_t i = 0; i < value.ValueList().size(); ++i) { + EXPECT_EQ(original[i].GetMemoryResource(), utils::NewDeleteResource()); + EXPECT_EQ(moved[i].GetMemoryResource(), &monotonic_memory); + EXPECT_EQ(copied[i].GetMemoryResource(), &monotonic_memory); + EXPECT_TRUE(TypedValue::BoolEqual{}(original[i], moved[i])); + EXPECT_TRUE(TypedValue::BoolEqual{}(original[i], copied[i])); + } + } + } +}