tbv2: remove unnecessary copy in Element (#457)

Summary:
tbv2: remove unnecessary copy in Element

`IntrospectionResult::const_iterator` iterates through the `Element`s in an
`IntrospectionResult`. `Element` currently copies the `type_path` which is a
`std::vector<string_view>` every time the iterator is incremented. This is
unnecessary as the data in the vector only changes slightly between iterations.

This change changes the `type_path` field in `Element` to a
`std::span<const std::string_view>`. Doing this previously caused SEGVs because
of the iterator's potential to be copied. To make it possible we do two things:
1. Make all copies explicit using a clone interface as in `ContainerInfo`. This
   prevents accidental copies of an expensive structure.
2. After calling the copy constructor in `clone()` update the `span` in `next_`
   to point at the newly copied structure.

Moves are fine because the `span` points at the allocation of the `vector`, not
the vector itself.


Test Plan:
- CI
- `FILTER='OilIntegration.*' make test`
- Ran `OilgenIntegration.std_vector_vector_int_some` which SEGVd with the
  `span` change before and now doesn't. This now passes cleanly with ASAN
  enabled on the target, though isn't available in `main` (only works on my
  machine).

Differential Revision: D53472595

Pulled By: JakeHillion

Author: JakeHillion

include/oi/IntrospectionResult-inl.h

@@ -45,7 +45,9 @@ inline IntrospectionResult::const_iterator IntrospectionResult::begin() const {
   return cbegin();
 }
 inline IntrospectionResult::const_iterator IntrospectionResult::cbegin() const {
-  return ++const_iterator{buf_.cbegin(), inst_};
+  auto it = const_iterator{buf_.cbegin(), inst_};
+  ++it;
+  return it;
 }
 inline IntrospectionResult::const_iterator IntrospectionResult::end() const {
   return cend();
@@ -70,6 +72,18 @@ inline const result::Element* IntrospectionResult::const_iterator::operator->()
   return &*next_;
 }
 
+inline IntrospectionResult::const_iterator
+IntrospectionResult::const_iterator::clone() const {
+  auto ret{*this};
+
+  // Fix the self referential type_path_ field in next_
+  if (ret.next_.has_value()) {
+    ret.next_->type_path = ret.type_path_;
+  }
+
+  return ret;
+}
+
 inline bool IntrospectionResult::const_iterator::operator==(
     const IntrospectionResult::const_iterator& that) const {
   // Case 1: The next data to read differs, thus the iterators are different.

include/oi/IntrospectionResult.h

@@ -43,6 +43,16 @@ class IntrospectionResult {
     const_iterator& operator++();
     const_iterator operator++(int);
 
+   private:
+    const_iterator(const const_iterator&) = default;
+    const_iterator& operator=(const const_iterator& other) = default;
+
+   public:
+    const_iterator(const_iterator&&) = default;
+    const_iterator& operator=(const_iterator&&) = default;
+    // Explicit interface for copying
+    const_iterator clone() const;
+
    private:
     using stack_t =
         std::stack<exporters::inst::Inst, std::vector<exporters::inst::Inst>>;

include/oi/result/Element.h

@@ -41,8 +41,7 @@ struct Element {
   };
 
   std::string_view name;
-  std::vector<std::string_view>
-      type_path;  // TODO: should be span<const std::string_view>
+  std::span<const std::string_view> type_path;
   std::span<const std::string_view> type_names;
   size_t static_size;
   size_t exclusive_size;

include/oi/result/SizedResult-inl.h

@@ -35,7 +35,9 @@ SizedResult<Res>::SizedResult(Res res) : res_{std::move(res)} {
 
 template <typename Res>
 typename SizedResult<Res>::const_iterator SizedResult<Res>::begin() const {
-  return ++const_iterator{res_.begin(), res_.end()};
+  const_iterator it{res_.begin(), res_.end()};
+  ++it;
+  return it;
 }
 template <typename Res>
 typename SizedResult<Res>::const_iterator SizedResult<Res>::end() const {
@@ -44,7 +46,7 @@ typename SizedResult<Res>::const_iterator SizedResult<Res>::end() const {
 
 template <typename Res>
 SizedResult<Res>::const_iterator::const_iterator(It it, const It& end)
-    : data_{it} {
+    : data_{it.clone()} {
   struct StackEntry {
     size_t index;
     size_t depth;
@@ -75,7 +77,8 @@ SizedResult<Res>::const_iterator::const_iterator(It it, const It& end)
 }
 
 template <typename Res>
-SizedResult<Res>::const_iterator::const_iterator(It end) : data_{end} {
+SizedResult<Res>::const_iterator::const_iterator(It end)
+    : data_{std::move(end)} {
 }
 
 template <typename Res>

oi/IntrospectionResult.cpp

@@ -70,7 +70,7 @@ IntrospectionResult::const_iterator::operator++() {
           if constexpr (std::is_same_v<T, exporters::inst::Field>) {
             type_path_.emplace_back(ty.name);
             stack_.emplace(exporters::inst::PopTypePath{});
-            next_ = result::Element{
+            next_.emplace(result::Element{
                 .name = ty.name,
                 .type_path = type_path_,
                 .type_names = ty.type_names,
@@ -79,7 +79,7 @@ IntrospectionResult::const_iterator::operator++() {
                 .container_stats = std::nullopt,
                 .is_set_stats = std::nullopt,
                 .is_primitive = ty.is_primitive,
-            };
+            });
 
             for (const auto& [dy, handler] : ty.processors) {
               auto parsed = exporters::ParsedData::parse(data_, dy);
@@ -94,7 +94,6 @@ IntrospectionResult::const_iterator::operator++() {
                       .second;
 
               type_path_.back() = new_name_ref;
-              next_->type_path.back() = new_name_ref;
               next_->name = new_name_ref;
             }