replaces Dataset fields vector to a map

This refactor aims to increase the performance of
AddField / Getfield at the expense of memory usage and
some bits of performances when only few fields are used

Moving to non-contiguous memory will impact cpu cache benefits,
however, since each of the Field has again another pointer to
its actual data, this benefits where actually just small.

Also the choice of map v.s. unordered_map is about the number of
elements, very likely few rehashing happenings from until <100

This will also reduce the memory fragmentation caused by vectors.

Signed-off-by: Vicente Adolfo Bolea Sanchez <vicente.bolea@kitware.com>

vtkm/cont/DataSet.cxx

@@ -44,13 +44,17 @@ void DataSet::CopyStructure(const vtkm::cont::DataSet& source)
 const vtkm::cont::Field& DataSet::GetField(vtkm::Id index) const
 {
   VTKM_ASSERT((index >= 0) && (index < this->GetNumberOfFields()));
-  return this->Fields[static_cast<std::size_t>(index)];
+  auto it = this->Fields.cbegin();
+  std::advance(it, index);
+  return it->second;
 }
 
 vtkm::cont::Field& DataSet::GetField(vtkm::Id index)
 {
   VTKM_ASSERT((index >= 0) && (index < this->GetNumberOfFields()));
-  return this->Fields[static_cast<std::size_t>(index)];
+  auto it = this->Fields.begin();
+  std::advance(it, index);
+  return it->second;
 }
 
 vtkm::Id DataSet::GetFieldIndex(const std::string& name, vtkm::cont::Field::Association assoc) const
@@ -150,19 +154,22 @@ vtkm::Id DataSet::FindFieldIndex(const std::string& name,
                                  vtkm::cont::Field::Association association,
                                  bool& found) const
 {
-  for (std::size_t index = 0; index < this->Fields.size(); ++index)
+  const auto it = this->Fields.find({ name, association });
+  if (it != this->Fields.end())
   {
-    if ((association == vtkm::cont::Field::Association::ANY ||
-         association == this->Fields[index].GetAssociation()) &&
-        this->Fields[index].GetName() == name)
-    {
-      found = true;
-      return static_cast<vtkm::Id>(index);
-    }
+    found = true;
+    return static_cast<vtkm::Id>(std::distance(this->Fields.begin(), it));
   }
+
   found = false;
   return -1;
 }
 
+VTKM_CONT void DataSet::AddField(const Field& field)
+{
+  // map::insert will not replace the duplicated element
+  this->Fields[{ field.GetName(), field.GetAssociation() }] = field;
+}
+
 } // namespace cont
 } // namespace vtkm

vtkm/cont/DataSet.h

@@ -39,7 +39,7 @@ public:
   /// to have the same number of points.
   VTKM_CONT vtkm::Id GetNumberOfPoints() const;
 
-  VTKM_CONT void AddField(const Field& field) { this->Fields.push_back(field); }
+  VTKM_CONT void AddField(const Field& field);
 
   VTKM_CONT
   const vtkm::cont::Field& GetField(vtkm::Id index) const;
@@ -73,14 +73,14 @@ public:
   }
 
 
-  /// Returns the first field that matches the provided name and association
+  /// Returns the field that matches the provided name and association
   /// Will return -1 if no match is found
   VTKM_CONT
   vtkm::Id GetFieldIndex(
     const std::string& name,
     vtkm::cont::Field::Association assoc = vtkm::cont::Field::Association::ANY) const;
 
-  /// Returns the first field that matches the provided name and association
+  /// Returns the field that matches the provided name and association
   /// Will throw an exception if no match is found
   //@{
   VTKM_CONT
@@ -203,8 +203,23 @@ public:
   void PrintSummary(std::ostream& out) const;
 
 private:
+  struct FieldCompare
+  {
+    using Key = std::pair<std::string, vtkm::cont::Field::Association>;
+
+    template <typename T>
+    bool operator()(const T& a, const T& b) const
+    {
+      if (a.first == b.first)
+        return a.second < b.second && a.second != vtkm::cont::Field::Association::ANY &&
+          b.second != vtkm::cont::Field::Association::ANY;
+
+      return a.first < b.first;
+    }
+  };
+
   std::vector<vtkm::cont::CoordinateSystem> CoordSystems;
-  std::vector<vtkm::cont::Field> Fields;
+  std::map<FieldCompare::Key, vtkm::cont::Field, FieldCompare> Fields;
   vtkm::cont::DynamicCellSet CellSet;
 
   VTKM_CONT

vtkm/cont/testing/UnitTestMoveConstructors.cxx

@@ -40,6 +40,26 @@ void is_noexcept_movable()
   VTKM_TEST_ASSERT(valid, msg);
 }
 
+// DataSet uses a map which is not nothrow constructible/assignable in the
+// following implementations
+template<>
+void is_noexcept_movable<vtkm::cont::DataSet>()
+{
+  using T = vtkm::cont::DataSet;
+
+  constexpr bool valid =
+#if ((defined(__GNUC__) && (__GNUC__ <= 5)) || defined(_WIN32))
+    std::is_move_constructible<T>::value &&
+    std::is_move_assignable<T>::value;
+#else
+    std::is_nothrow_move_constructible<T>::value &&
+    std::is_nothrow_move_assignable<T>::value;
+#endif
+
+  std::string msg = typeid(T).name() + std::string(" should be noexcept moveable");
+  VTKM_TEST_ASSERT(valid, msg);
+}
+
 template<typename T>
 void is_triv_noexcept_movable()
 {