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).
This commit is contained in:
Jake Hillion 2024-01-18 16:23:28 +00:00
parent 31ba8659f0
commit 8428cb97da
5 changed files with 34 additions and 9 deletions

View File

@ -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.

View File

@ -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>>;

View File

@ -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;

View File

@ -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>

View File

@ -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;
}