Skip to content

Commit

Permalink
tbv2: remove unnecessary copy in Element
Browse files Browse the repository at this point in the history
`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).
  • Loading branch information
JakeHillion committed Jan 11, 2024
1 parent a9afb25 commit 5c78c1d
Show file tree
Hide file tree
Showing 5 changed files with 34 additions and 9 deletions.
16 changes: 15 additions & 1 deletion include/oi/IntrospectionResult-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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.
Expand Down
10 changes: 10 additions & 0 deletions include/oi/IntrospectionResult.h
Original file line number Diff line number Diff line change
Expand Up @@ -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>>;
Expand Down
3 changes: 1 addition & 2 deletions include/oi/result/Element.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
9 changes: 6 additions & 3 deletions include/oi/result/SizedResult-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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;
Expand Down Expand Up @@ -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>
Expand Down
5 changes: 2 additions & 3 deletions oi/IntrospectionResult.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -70,15 +70,15 @@ 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,
.static_size = ty.static_size,
.exclusive_size = ty.exclusive_size,
.container_stats = std::nullopt,
.is_set_stats = std::nullopt,
};
});

for (const auto& [dy, handler] : ty.processors) {
auto parsed = exporters::ParsedData::parse(data_, dy);
Expand All @@ -93,7 +93,6 @@ IntrospectionResult::const_iterator::operator++() {
.second;

type_path_.back() = new_name_ref;
next_->type_path.back() = new_name_ref;
next_->name = new_name_ref;
}

Expand Down

0 comments on commit 5c78c1d

Please sign in to comment.