tursom/memgraph
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Use Allocator for vector in TypedValue

Browse Source 
Reviewers: mtomic, llugovic, mferencevic

Reviewed By: mtomic

Subscribers: pullbot

Differential Revision: https://phabricator.memgraph.io/D2086
This commit is contained in:
Teon Banek 2019-05-24 14:22:35 +02:00
parent 0152ac2dfe
commit bb2dbc290f
6 changed files with 187 additions and 59 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

28
src/query/plan/operator.cpp
View File

@ -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<TypedValue> &edges_on_frame) {
void AppendEdge(
const EdgeAccessor &new_edge,
std::vector<TypedValue, utils::Allocator<TypedValue>> *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<TypedValue> &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<TypedValue> &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();

68
src/query/typed_value.cpp
View File

@ -43,8 +43,10 @@ TypedValue::TypedValue(const PropertyValue &value,
return;
case PropertyValue::Type::List: {
type_ = Type::List;
auto vec = value.Value<std::vector<PropertyValue>>();
new (&list_v) std::vector<TypedValue>(vec.begin(), vec.end());
const auto &vec = value.Value<std::vector<PropertyValue>>();
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<std::vector<PropertyValue>>();
new (&list_v)
std::vector<TypedValue>(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<TypedValue>(other.list_v);
new (&list_v) TVector(other.list_v, memory_);
return;
case Type::Map:
new (&map_v) std::map<std::string, TypedValue>(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<TypedValue>(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::string, TypedValue>(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<TypedValue>, 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<TypedValue> &, List,
list_v)
DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(const TypedValue::TVector &, List, list_v)
TypedValue &TypedValue::operator=(const std::vector<TypedValue> &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<TypedValue>, List, list_v)
DEFINE_TYPED_VALUE_MOVE_ASSIGNMENT(TypedValue::TVector, List, list_v)
TypedValue &TypedValue::operator=(std::vector<TypedValue> &&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<TypedValue>(other.list_v);
new (&list_v) TVector(other.list_v, memory_);
return *this;
case TypedValue::Type::Map:
new (&map_v) std::map<std::string, TypedValue>(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<TypedValue>(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::string, TypedValue>(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<TypedValue>();
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<TypedValue> 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<std::string>{}(value.Value<std::string>());
case TypedValue::Type::List: {
return utils::FnvCollection<std::vector<TypedValue>, TypedValue, Hash>{}(
return utils::FnvCollection<TypedValue::TVector, TypedValue, Hash>{}(
value.ValueList());
}
case TypedValue::Type::Map: {

109
src/query/typed_value.hpp
View File

@ -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<TypedValue, Hash, BoolEqual>;
using value_map_t = std::map<std::string, TypedValue>;
using TVector = std::vector<TypedValue, utils::Allocator<TypedValue>>;
/** Allocator type so that STL containers are aware that we need one */
using allocator_type = utils::Allocator<TypedValue>;
@ -154,10 +159,33 @@ class TypedValue {
new (&string_v) std::string(value);
}
/** Construct a copy using the given utils::MemoryResource */
TypedValue(const std::vector<TypedValue> &value,
utils::MemoryResource *memory = utils::NewDeleteResource())
: memory_(memory), type_(Type::List) {
new (&list_v) std::vector<TypedValue>(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<utils::Allocator<TypedValue>>::
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<std::string, TypedValue> &value,
@ -200,14 +228,48 @@ class TypedValue {
new (&string_v) std::string(std::move(value));
}
TypedValue(std::vector<TypedValue> &&value) noexcept : type_(Type::List) {
new (&list_v) std::vector<TypedValue>(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<TypedValue> &&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<TypedValue> &&other, utils::MemoryResource *memory)
: memory_(memory), type_(Type::List) {
new (&list_v) TVector(memory_);
list_v.reserve(other.size());
// std::vector<TypedValue> 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<TypedValue> &&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<TypedValue>(std::move(value));
new (&list_v) TVector(std::move(other), memory_);
}
TypedValue(std::map<std::string, TypedValue> &&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> &);
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<TypedValue> &&) 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> &&);
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<TypedValue>, 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<TypedValue> list_v;
TVector list_v;
// clang doesn't allow unordered_map to have incomplete type as value so we
// we use map.
std::map<std::string, TypedValue> map_v;

5
tests/unit/bfs_common.hpp
View File

@ -266,9 +266,10 @@ auto GetProp(const RecordAccessor<TRecord> &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 <class TPathAllocator>
void CheckPath(database::GraphDbAccessor *dba, const VertexAccessor &source,
const VertexAccessor &sink,
const std::vector<query::TypedValue> &path,
const std::vector<query::TypedValue, TPathAllocator> &path,
const std::vector<std::pair<int, int>> &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<int64_t>();
int to = GetProp(next, "id", dba).Value<int64_t>();

3
tests/unit/query_plan_match_filter_return.cpp
View File

@ -604,7 +604,8 @@ class QueryPlanExpandVariable : public testing::Test {
Symbol symbol) {
map_int count_per_length;
for (const auto &edge_list :
GetResults<std::vector<TypedValue>>(input_op, symbol)) {
GetResults<std::vector<TypedValue, utils::Allocator<TypedValue>>>(
input_op, symbol)) {
auto length = edge_list.size();
auto found = count_per_length.find(length);
if (found == count_per_length.end())

33
tests/unit/typed_value.cpp
View File

@ -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<TypedValue, utils::Allocator<TypedValue>>
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 &copy_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]));
}
}
}
}