Use Allocator for map in TypedValue

Reviewers: mtomic, llugovic, mferencevic

Reviewed By: llugovic

Subscribers: pullbot

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

src/query/distributed/serialization.cpp

@@ -43,7 +43,7 @@ void Save(const query::TypedValue &value, slk::Builder *builder,
       size_t size = map.size();
       slk::Save(size, builder);
       for (const auto &kv : map) {
-        slk::Save(kv.first, builder);
+        slk::Save(std::string(kv.first), builder);
         slk::Save(kv.second, builder, versions, worker_id);
       }
       return;

src/query/interpret/eval.hpp

@@ -184,7 +184,7 @@ class ExpressionEvaluator : public ExpressionVisitor<TypedValue> {
         throw QueryRuntimeException("Expected a string as a map index, got {}.",
                                     index.type());
       const auto &map = lhs.ValueMap();
-      auto found = map.find(std::string(index.ValueString()));
+      auto found = map.find(index.ValueString());
       if (found == map.end()) return TypedValue::Null;
       return found->second;
     }
@@ -276,8 +276,8 @@ class ExpressionEvaluator : public ExpressionVisitor<TypedValue> {
         return expression_result.Value<EdgeAccessor>().PropsAt(
             GetProperty(property_lookup.property_));
       case TypedValue::Type::Map: {
-        auto &map = expression_result.ValueMap();
+        const auto &map = expression_result.ValueMap();
-        auto found = map.find(property_lookup.property_.name);
+        auto found = map.find(property_lookup.property_.name.c_str());
         if (found == map.end()) return TypedValue::Null;
         return found->second;
       }

src/query/plan/operator.cpp

@@ -2035,7 +2035,8 @@ void SetProperties::SetPropertiesCursor::Set(TRecordAccessor &record,
       break;
     case TypedValue::Type::Map: {
       for (const auto &kv : rhs.ValueMap())
-        PropsSetChecked(&record, db_.Property(kv.first), kv.second);
+        PropsSetChecked(&record, db_.Property(std::string(kv.first)),
+                        kv.second);
       break;
     }
     default:

src/query/typed_value.cpp

@@ -52,8 +52,9 @@ TypedValue::TypedValue(const PropertyValue &value,
     }
     case PropertyValue::Type::Map: {
       type_ = Type::Map;
-      auto map = value.Value<std::map<std::string, PropertyValue>>();
+      const auto &map = value.Value<std::map<std::string, PropertyValue>>();
-      new (&map_v) std::map<std::string, TypedValue>(map.begin(), map.end());
+      new (&map_v) TMap(memory_);
+      for (const auto &kv : map) map_v.emplace(kv.first, kv.second);
       return;
     }
   }
@@ -98,9 +99,8 @@ TypedValue::TypedValue(PropertyValue &&other, utils::MemoryResource *memory)
     case PropertyValue::Type::Map: {
       type_ = Type::Map;
       auto &map = other.Value<std::map<std::string, PropertyValue>>();
-      new (&map_v) std::map<std::string, TypedValue>(
+      new (&map_v) TMap(memory_);
-          std::make_move_iterator(map.begin()),
+      for (auto &kv : map) map_v.emplace(kv.first, std::move(kv.second));
-          std::make_move_iterator(map.end()));
       break;
     }
   }
@@ -134,7 +134,7 @@ TypedValue::TypedValue(const TypedValue &other, utils::MemoryResource *memory)
       new (&list_v) TVector(other.list_v, memory_);
       return;
     case Type::Map:
-      new (&map_v) std::map<std::string, TypedValue>(other.map_v);
+      new (&map_v) TMap(other.map_v, memory_);
       return;
     case Type::Vertex:
       new (&vertex_v) VertexAccessor(other.vertex_v);
@@ -173,7 +173,7 @@ TypedValue::TypedValue(TypedValue &&other, utils::MemoryResource *memory)
       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));
+      new (&map_v) TMap(std::move(other.map_v), memory_);
       break;
     case Type::Vertex:
       new (&vertex_v) VertexAccessor(std::move(other.vertex_v));
@@ -203,9 +203,11 @@ TypedValue::operator PropertyValue() const {
     case TypedValue::Type::List:
       return PropertyValue(
           std::vector<PropertyValue>(list_v.begin(), list_v.end()));
-    case TypedValue::Type::Map:
+    case TypedValue::Type::Map: {
-      return PropertyValue(
+      std::map<std::string, PropertyValue> map;
-          std::map<std::string, PropertyValue>(map_v.begin(), map_v.end()));
+      for (const auto &kv : map_v) map.emplace(kv.first, kv.second);
+      return PropertyValue(std::move(map));
+    }
     default:
       break;
   }
@@ -245,7 +247,7 @@ DEFINE_VALUE_AND_TYPE_GETTERS(int64_t, Int, int_v)
 DEFINE_VALUE_AND_TYPE_GETTERS(double, Double, double_v)
 DEFINE_VALUE_AND_TYPE_GETTERS(TypedValue::TString, String, string_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(TypedValue::TMap, Map, map_v)
 DEFINE_VALUE_AND_TYPE_GETTERS(VertexAccessor, Vertex, vertex_v)
 DEFINE_VALUE_AND_TYPE_GETTERS(EdgeAccessor, Edge, edge_v)
 DEFINE_VALUE_AND_TYPE_GETTERS(Path, Path, path_v)
@@ -363,7 +365,14 @@ TypedValue &TypedValue::operator=(const std::vector<TypedValue> &other) {
   return *this;
 }
 
-DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(const TypedValue::value_map_t &, Map, map_v)
+DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(const TypedValue::TMap &, Map, map_v)
+
+TypedValue &TypedValue::operator=(
+    const std::map<std::string, TypedValue> &other) {
+  *this = TypedValue(other, memory_);
+  return *this;
+}
+
 DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(const VertexAccessor &, Vertex, vertex_v)
 DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(const EdgeAccessor &, Edge, edge_v)
 DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(const Path &, Path, path_v)
@@ -395,7 +404,13 @@ TypedValue &TypedValue::operator=(std::vector<TypedValue> &&other) {
   return *this;
 }
 
-DEFINE_TYPED_VALUE_MOVE_ASSIGNMENT(TypedValue::value_map_t, Map, map_v)
+DEFINE_TYPED_VALUE_MOVE_ASSIGNMENT(TypedValue::TMap, Map, map_v)
+
+TypedValue &TypedValue::operator=(std::map<std::string, TypedValue> &&other) {
+  *this = TypedValue(std::move(other), memory_);
+  return *this;
+}
+
 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)
@@ -433,7 +448,7 @@ TypedValue &TypedValue::operator=(const TypedValue &other) {
         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);
+        new (&map_v) TMap(other.map_v, memory_);
         return *this;
       case TypedValue::Type::Vertex:
         new (&vertex_v) VertexAccessor(other.vertex_v);
@@ -481,7 +496,7 @@ TypedValue &TypedValue::operator=(TypedValue &&other) noexcept(false) {
         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));
+        new (&map_v) TMap(std::move(other.map_v), memory_);
         break;
       case TypedValue::Type::Vertex:
         new (&vertex_v) VertexAccessor(std::move(other.vertex_v));
@@ -518,8 +533,7 @@ void TypedValue::DestroyValue() {
     list_v.~TVector();
     break;
   case Type::Map:
-    using namespace std;
+    map_v.~TMap();
-    map_v.~map<std::string, TypedValue>();
     break;
   case Type::Vertex:
     vertex_v.~VertexAccessor();
@@ -904,7 +918,7 @@ size_t TypedValue::Hash::operator()(const TypedValue &value) const {
     case TypedValue::Type::Map: {
       size_t hash = 6543457;
       for (const auto &kv : value.ValueMap()) {
-        hash ^= std::hash<std::string>{}(kv.first);
+        hash ^= std::hash<std::string_view>{}(kv.first);
         hash ^= this->operator()(kv.second);
       }
       return hash;

src/query/typed_value.hpp

@@ -6,7 +6,6 @@
 #include <memory>
 #include <string>
 #include <string_view>
-#include <unordered_set>
 #include <utility>
 #include <vector>
 
@@ -72,13 +71,17 @@ class TypedValue {
   using TString =
       std::basic_string<char, std::char_traits<char>, utils::Allocator<char>>;
 
-  /**
+  // TypedValue at this exact moment of compilation is an incomplete type, and
-   * Unordered set of TypedValue items. Can contain at most one Null element,
+  // the standard says that instantiating a container with an incomplete type
-   * and treats an integral and floating point value as same if they are equal
+  // invokes undefined behaviour. The libstdc++-8.3.0 we are using supports
-   * in the floating point domain (TypedValue operator== behaves the same).
+  // std::map with incomplete type, but this is still murky territory. Note that
-   */
+  // since C++17, std::vector is explicitly said to support incomplete types.
-  using unordered_set = std::unordered_set<TypedValue, Hash, BoolEqual>;
+
-  using value_map_t = std::map<std::string, TypedValue>;
+  // Use transparent std::less<void> which forwards to `operator<`, so that it's
+  // possible to use `find` with C-style (null terminated) strings without
+  // actually constructing (and allocating) a key.
+  using TMap = std::map<TString, TypedValue, std::less<void>,
+                        utils::Allocator<std::pair<const TString, TypedValue>>>;
   using TVector = std::vector<TypedValue, utils::Allocator<TypedValue>>;
 
   /** Allocator type so that STL containers are aware that we need one */
@@ -220,10 +223,32 @@ class TypedValue {
     new (&list_v) TVector(value, memory_);
   }
 
+  /** Construct a copy using the given utils::MemoryResource */
   TypedValue(const std::map<std::string, TypedValue> &value,
              utils::MemoryResource *memory = utils::NewDeleteResource())
       : memory_(memory), type_(Type::Map) {
-    new (&map_v) std::map<std::string, TypedValue>(value);
+    new (&map_v) TMap(memory_);
+    for (const auto &kv : value) map_v.emplace(kv.first, kv.second);
+  }
+
+  /**
+   * 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 TMap &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 TMap &value, utils::MemoryResource *memory)
+      : memory_(memory), type_(Type::Map) {
+    new (&map_v) TMap(value, memory_);
   }
 
   TypedValue(const VertexAccessor &vertex,
@@ -279,7 +304,7 @@ class TypedValue {
 
   /**
    * Perform an element-wise move of the other and use the given MemoryResource.
-   * Other will be not be empty, though elements may be Null.
+   * Other will be not be left empty, though elements may be Null.
    */
   TypedValue(std::vector<TypedValue> &&other, utils::MemoryResource *memory)
       : memory_(memory), type_(Type::List) {
@@ -314,15 +339,44 @@ class TypedValue {
     new (&list_v) TVector(std::move(other), memory_);
   }
 
-  TypedValue(std::map<std::string, TypedValue> &&value) noexcept
+  /**
-      : type_(Type::Map) {
+   * Perform an element-wise move using default utils::NewDeleteResource().
-    new (&map_v) std::map<std::string, TypedValue>(std::move(value));
+   * Other will not be left empty, i.e. keys will exist but their values may
+   * be Null.
+   */
+  TypedValue(std::map<std::string, TypedValue> &&other)
+      : TypedValue(std::move(other), utils::NewDeleteResource()) {}
+
+  /**
+   * Perform an element-wise move using the given MemoryResource.
+   * Other will not be left empty, i.e. keys will exist but their values may
+   * be Null.
+   */
+  TypedValue(std::map<std::string, TypedValue> &&other,
+             utils::MemoryResource *memory)
+      : memory_(memory), type_(Type::Map) {
+    new (&map_v) TMap(memory_);
+    for (auto &kv : other) map_v.emplace(kv.first, std::move(kv.second));
   }
 
-  TypedValue(std::map<std::string, 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(TMap &&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, i.e. keys may
+   * exist but their values may be Null.
+   */
+  TypedValue(TMap &&other, utils::MemoryResource *memory)
       : memory_(memory), type_(Type::Map) {
-    new (&map_v) std::map<std::string, TypedValue>(std::move(value));
+    new (&map_v) TMap(std::move(other), memory_);
   }
 
   TypedValue(VertexAccessor &&vertex) noexcept : type_(Type::Vertex) {
@@ -377,7 +431,8 @@ class TypedValue {
   TypedValue &operator=(const std::string_view &);
   TypedValue &operator=(const TVector &);
   TypedValue &operator=(const std::vector<TypedValue> &);
-  TypedValue &operator=(const TypedValue::value_map_t &);
+  TypedValue &operator=(const TMap &);
+  TypedValue &operator=(const std::map<std::string, TypedValue> &);
   TypedValue &operator=(const VertexAccessor &);
   TypedValue &operator=(const EdgeAccessor &);
   TypedValue &operator=(const Path &);
@@ -389,7 +444,8 @@ class TypedValue {
   TypedValue &operator=(TString &&);
   TypedValue &operator=(TVector &&);
   TypedValue &operator=(std::vector<TypedValue> &&);
-  TypedValue &operator=(TypedValue::value_map_t &&);
+  TypedValue &operator=(TMap &&);
+  TypedValue &operator=(std::map<std::string, TypedValue> &&);
   TypedValue &operator=(VertexAccessor &&);
   TypedValue &operator=(EdgeAccessor &&);
   TypedValue &operator=(Path &&);
@@ -440,7 +496,7 @@ class TypedValue {
   /** 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(TMap, Map)
   DECLARE_VALUE_AND_TYPE_GETTERS(VertexAccessor, Vertex)
   DECLARE_VALUE_AND_TYPE_GETTERS(EdgeAccessor, Edge)
   DECLARE_VALUE_AND_TYPE_GETTERS(Path, Path)
@@ -478,9 +534,7 @@ class TypedValue {
     // because of data locality.
     TString string_v;
     TVector list_v;
-    // clang doesn't allow unordered_map to have incomplete type as value so we
+    TMap map_v;
-    // we use map.
-    std::map<std::string, TypedValue> map_v;
     VertexAccessor vertex_v;
     EdgeAccessor edge_v;
     Path path_v;

tests/unit/query_expression_evaluator.cpp

@@ -1009,7 +1009,7 @@ TEST_F(FunctionTest, Properties) {
   auto prop_values_to_int = [](TypedValue t) {
     std::unordered_map<std::string, int> properties;
     for (auto property : t.ValueMap()) {
-      properties[property.first] = property.second.ValueInt();
+      properties[std::string(property.first)] = property.second.ValueInt();
     }
     return properties;
   };

tests/unit/query_plan_accumulate_aggregate.cpp

@@ -327,7 +327,8 @@ TEST(QueryPlan, AggregateGroupByValues) {
   auto context = MakeContext(storage, symbol_table, &dba);
   auto results = CollectProduce(*produce, &context);
   ASSERT_EQ(results.size(), group_by_vals.size() - 2);
-  TypedValue::unordered_set result_group_bys;
+  std::unordered_set<TypedValue, TypedValue::Hash, TypedValue::BoolEqual>
+      result_group_bys;
   for (const auto &row : results) {
     ASSERT_EQ(2, row.size());
     result_group_bys.insert(row[1]);

tests/unit/typed_value.cpp

@@ -490,6 +490,27 @@ TEST_F(AllTypesFixture, PropagationOfMemoryOnConstruction) {
         EXPECT_TRUE(TypedValue::BoolEqual{}(original[i], moved[i]));
         EXPECT_TRUE(TypedValue::BoolEqual{}(original[i], copied[i]));
       }
+    } else if (value.type() == TypedValue::Type::Map) {
+      ASSERT_EQ(move_constructed_value.type(), value.type());
+      const auto &original = value.ValueMap();
+      const auto &moved = move_constructed_value.ValueMap();
+      const auto &copied = copy_constructed_value.ValueMap();
+      auto expect_allocator = [](const auto &kv, auto *memory_resource) {
+        EXPECT_EQ(*kv.first.get_allocator().GetMemoryResource(),
+                  *memory_resource);
+        EXPECT_EQ(*kv.second.GetMemoryResource(), *memory_resource);
+      };
+      for (const auto &kv : original) {
+        expect_allocator(kv, utils::NewDeleteResource());
+        auto moved_it = moved.find(kv.first);
+        ASSERT_NE(moved_it, moved.end());
+        expect_allocator(*moved_it, &monotonic_memory);
+        auto copied_it = copied.find(kv.first);
+        ASSERT_NE(copied_it, copied.end());
+        expect_allocator(*copied_it, &monotonic_memory);
+        EXPECT_TRUE(TypedValue::BoolEqual{}(kv.second, moved_it->second));
+        EXPECT_TRUE(TypedValue::BoolEqual{}(kv.second, copied_it->second));
+      }
     }
   }
 }