mirror of
https://github.com/JakeHillion/object-introspection.git
synced 2024-09-19 02:59:05 +01:00
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
This commit is contained in:
parent
f076b34a35
commit
89b230395f
@ -45,7 +45,9 @@ inline IntrospectionResult::const_iterator IntrospectionResult::begin() const {
|
|||||||
return cbegin();
|
return cbegin();
|
||||||
}
|
}
|
||||||
inline IntrospectionResult::const_iterator IntrospectionResult::cbegin() const {
|
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 {
|
inline IntrospectionResult::const_iterator IntrospectionResult::end() const {
|
||||||
return cend();
|
return cend();
|
||||||
@ -70,6 +72,18 @@ inline const result::Element* IntrospectionResult::const_iterator::operator->()
|
|||||||
return &*next_;
|
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==(
|
inline bool IntrospectionResult::const_iterator::operator==(
|
||||||
const IntrospectionResult::const_iterator& that) const {
|
const IntrospectionResult::const_iterator& that) const {
|
||||||
// Case 1: The next data to read differs, thus the iterators are different.
|
// Case 1: The next data to read differs, thus the iterators are different.
|
||||||
|
@ -43,6 +43,16 @@ class IntrospectionResult {
|
|||||||
const_iterator& operator++();
|
const_iterator& operator++();
|
||||||
const_iterator operator++(int);
|
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:
|
private:
|
||||||
using stack_t =
|
using stack_t =
|
||||||
std::stack<exporters::inst::Inst, std::vector<exporters::inst::Inst>>;
|
std::stack<exporters::inst::Inst, std::vector<exporters::inst::Inst>>;
|
||||||
|
@ -41,8 +41,7 @@ struct Element {
|
|||||||
};
|
};
|
||||||
|
|
||||||
std::string_view name;
|
std::string_view name;
|
||||||
std::vector<std::string_view>
|
std::span<const std::string_view> type_path;
|
||||||
type_path; // TODO: should be span<const std::string_view>
|
|
||||||
std::span<const std::string_view> type_names;
|
std::span<const std::string_view> type_names;
|
||||||
size_t static_size;
|
size_t static_size;
|
||||||
size_t exclusive_size;
|
size_t exclusive_size;
|
||||||
|
@ -35,7 +35,9 @@ SizedResult<Res>::SizedResult(Res res) : res_{std::move(res)} {
|
|||||||
|
|
||||||
template <typename Res>
|
template <typename Res>
|
||||||
typename SizedResult<Res>::const_iterator SizedResult<Res>::begin() const {
|
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>
|
template <typename Res>
|
||||||
typename SizedResult<Res>::const_iterator SizedResult<Res>::end() const {
|
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>
|
template <typename Res>
|
||||||
SizedResult<Res>::const_iterator::const_iterator(It it, const It& end)
|
SizedResult<Res>::const_iterator::const_iterator(It it, const It& end)
|
||||||
: data_{it} {
|
: data_{it.clone()} {
|
||||||
struct StackEntry {
|
struct StackEntry {
|
||||||
size_t index;
|
size_t index;
|
||||||
size_t depth;
|
size_t depth;
|
||||||
@ -75,7 +77,8 @@ SizedResult<Res>::const_iterator::const_iterator(It it, const It& end)
|
|||||||
}
|
}
|
||||||
|
|
||||||
template <typename Res>
|
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>
|
template <typename Res>
|
||||||
|
@ -70,7 +70,7 @@ IntrospectionResult::const_iterator::operator++() {
|
|||||||
if constexpr (std::is_same_v<T, exporters::inst::Field>) {
|
if constexpr (std::is_same_v<T, exporters::inst::Field>) {
|
||||||
type_path_.emplace_back(ty.name);
|
type_path_.emplace_back(ty.name);
|
||||||
stack_.emplace(exporters::inst::PopTypePath{});
|
stack_.emplace(exporters::inst::PopTypePath{});
|
||||||
next_ = result::Element{
|
next_.emplace(result::Element{
|
||||||
.name = ty.name,
|
.name = ty.name,
|
||||||
.type_path = type_path_,
|
.type_path = type_path_,
|
||||||
.type_names = ty.type_names,
|
.type_names = ty.type_names,
|
||||||
@ -79,7 +79,7 @@ IntrospectionResult::const_iterator::operator++() {
|
|||||||
.container_stats = std::nullopt,
|
.container_stats = std::nullopt,
|
||||||
.is_set_stats = std::nullopt,
|
.is_set_stats = std::nullopt,
|
||||||
.is_primitive = ty.is_primitive,
|
.is_primitive = ty.is_primitive,
|
||||||
};
|
});
|
||||||
|
|
||||||
for (const auto& [dy, handler] : ty.processors) {
|
for (const auto& [dy, handler] : ty.processors) {
|
||||||
auto parsed = exporters::ParsedData::parse(data_, dy);
|
auto parsed = exporters::ParsedData::parse(data_, dy);
|
||||||
@ -94,7 +94,6 @@ IntrospectionResult::const_iterator::operator++() {
|
|||||||
.second;
|
.second;
|
||||||
|
|
||||||
type_path_.back() = new_name_ref;
|
type_path_.back() = new_name_ref;
|
||||||
next_->type_path.back() = new_name_ref;
|
|
||||||
next_->name = new_name_ref;
|
next_->name = new_name_ref;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
Loading…
Reference in New Issue
Block a user