perfetto: hold StatusOr's value in a union (#8126) **Summary** `StatusOr<T>` holds its value in a union rather than a `std::optional`, and `ASSIGN_OR_RETURN` moves an error out rather than copying it. Stacked on #8121. **Why** The optional's "has value" flag duplicated what the status already says. It cost a byte plus padding in every StatusOr, and a check on every access. `ASSIGN_OR_RETURN` also copied the error before returning it. Since #8120 holds errors on the heap, that copy was an allocation for every error propagated. **Changes** - The value lives in an anonymous union, as `std::optional` keeps its own, and exists exactly while the status is ok. - A value is only constructed where there was none, and only destroyed where there will be none. One value replaces another by `T`'s own assignment, so no object is ever replaced in place. This needs no casts and no `std::launder`. - A moved-from StatusOr keeps its status, and so keeps its moved-from value, which it still destroys. - `StatusOr<const T>` is no longer allowed: constructing into a const member would need a cast. Its two uses, `StatusOr<const char* const>` in the winscope plugins, now return `StatusOr<const char*>`. - `status() &&` moves an error out, and leaves the value of an ok StatusOr in place. `ASSIGN_OR_RETURN` returns through it. **Binary size** Stripped, against #8121: `trace_processor_shell` -40 KB, `perfetto` -0.9 KB, `libperfetto.so` -0.6 KB. **Benchmarks** Both builds alternated three times, pinned to one CPU. | Benchmark | #8121 | this | | | ----------------------- | -------- | -------- | ------ | | BM_StatusOrOkChain | 15.0 ns | 11.8 ns | -21% | | BM_StatusOrErrorChain | 77.2 ns | 53.5 ns | -31% | | BM_StatusOkChain | 4.4 ns | 4.4 ns | | | Others | | | +/-2% | Moving rather than copying the error takes `StatusOr` error propagation below `main` (63.9 ns before the stack). **Testing** - New `StatusOrTest` cases use a type which counts its live objects. They cover copy and move, every assignment between holding a value and holding an error, self-assignment, `status() &&`, and `ASSIGN_OR_RETURN`. - All unit, diff and integration tests pass locally, and the whole Linux clang build compiles.
diff --git a/include/perfetto/ext/base/status_macros.h b/include/perfetto/ext/base/status_macros.h index 12a4cb3..0accc25 100644 --- a/include/perfetto/ext/base/status_macros.h +++ b/include/perfetto/ext/base/status_macros.h
@@ -33,11 +33,12 @@ // Evalues |rhs| which should return a base::StatusOr<T> and assigns this // to |lhs|. If the status is an error status, returns the status from the -// current function. -#define ASSIGN_OR_RETURN(lhs, rhs) \ - PERFETTO_INTERNAL_MACRO_CONCAT(auto status_or, __LINE__) = rhs; \ - RETURN_IF_ERROR( \ - PERFETTO_INTERNAL_MACRO_CONCAT(status_or, __LINE__).status()); \ +// current function, moved rather than copied out. +#define ASSIGN_OR_RETURN(lhs, rhs) \ + PERFETTO_INTERNAL_MACRO_CONCAT(auto status_or, __LINE__) = rhs; \ + if (!PERFETTO_INTERNAL_MACRO_CONCAT(status_or, __LINE__).ok()) \ + return std::move(PERFETTO_INTERNAL_MACRO_CONCAT(status_or, __LINE__)) \ + .status(); \ lhs = std::move(PERFETTO_INTERNAL_MACRO_CONCAT(status_or, __LINE__).value()) #endif // INCLUDE_PERFETTO_EXT_BASE_STATUS_MACROS_H_
diff --git a/include/perfetto/ext/base/status_or.h b/include/perfetto/ext/base/status_or.h index 81b6449..16f6b79 100644 --- a/include/perfetto/ext/base/status_or.h +++ b/include/perfetto/ext/base/status_or.h
@@ -17,7 +17,8 @@ #ifndef INCLUDE_PERFETTO_EXT_BASE_STATUS_OR_H_ #define INCLUDE_PERFETTO_EXT_BASE_STATUS_OR_H_ -#include <optional> +#include <new> +#include <type_traits> #include <utility> #include "perfetto/base/status.h" @@ -31,26 +32,56 @@ // // This class is modelled closely on absl::Status and should essentially 1:1 // match it's API. +// +// The value is held only while the status is ok, in a union, as +// std::optional holds its value: there is no separate flag for whether there +// is a value. A value is only ever constructed where there was none and +// destroyed where there will be none; one value replaces another by T's own +// assignment. template <typename T> class StatusOr { public: + static_assert(!std::is_const<T>::value, + "StatusOr<const T> is not supported: use StatusOr<T>"); + // Matches naming of declarations in similar types e.g. std::optional, // std::variant. using value_type = T; // Intentionally implicit to allow idomatic usage (e.g. returning value/status // from base::StatusOr returning function). - StatusOr(base::Status status) : StatusOr(std::move(status), std::nullopt) { + StatusOr(base::Status status) : status_(std::move(status)) { if (status_.ok()) { // Matches what Abseil's approach towards OkStatus being passed to // absl::StatusOr<T>. PERFETTO_FATAL("base::OkStatus passed to StatusOr: this is not allowed"); } } - StatusOr(T value) : StatusOr(base::OkStatus(), std::move(value)) {} + StatusOr(T value) { new (&value_) T(std::move(value)); } + + StatusOr(const StatusOr& other) { ConstructFrom(other); } + StatusOr(StatusOr&& other) noexcept( + std::is_nothrow_move_constructible<T>::value) { + ConstructFrom(std::move(other)); + } + StatusOr& operator=(const StatusOr& other) { + AssignFrom(other); + return *this; + } + StatusOr& operator=(StatusOr&& other) noexcept( + std::is_nothrow_move_assignable<T>::value && + std::is_nothrow_move_constructible<T>::value) { + AssignFrom(std::move(other)); + return *this; + } + ~StatusOr() { Destroy(); } bool ok() const { return status_.ok(); } - const base::Status& status() const { return status_; } + const base::Status& status() const& { return status_; } + // Moves an error out, leaving the value of an ok StatusOr in place. + base::Status status() && { + return ok() ? base::OkStatus() : std::move(status_); + } // For an lvalue caller, if ok(), returns a reference to the value, so the // StatusOr keeps owning it. @@ -58,7 +89,7 @@ T& ValueOrDie() & { if (!ok()) PERFETTO_FATAL("%s", status_.c_message()); - return *value_; + return value_; } // For an rvalue caller, if ok(), moves the value out because the StatusOr @@ -67,14 +98,14 @@ T ValueOrDie() && { if (!ok()) PERFETTO_FATAL("%s", status_.c_message()); - return std::move(*value_); + return std::move(value_); } T& value() { PERFETTO_DCHECK(status_.ok()); - return *value_; + return value_; } - const T& value() const { return *value_; } + const T& value() const { return value_; } T& operator*() { return value(); } const T& operator*() const { return value(); } @@ -83,13 +114,40 @@ const T* operator->() const { return &value(); } private: - StatusOr(base::Status status, std::optional<T> value) - : status_(std::move(status)), value_(std::move(value)) { - PERFETTO_DCHECK(!status_.ok() || value_.has_value()); + // Copies or moves |other| into a StatusOr with no value. A moved-from + // StatusOr keeps its status, and so a moved-from value which it still + // destroys. + template <typename Other> + void ConstructFrom(Other&& other) { + if (other.ok()) { + new (&value_) T(std::forward<Other>(other).value_); + } else { + status_ = std::forward<Other>(other).status_; + } + } + + template <typename Other> + void AssignFrom(Other&& other) { + if (ok() && other.ok()) { + value_ = std::forward<Other>(other).value_; + } else if (this != &other) { + Destroy(); + status_ = base::OkStatus(); + ConstructFrom(std::forward<Other>(other)); + } + } + + // Ends the value's lifetime, if there is one. + void Destroy() { + if (ok()) { + value_.~T(); + } } base::Status status_; - std::optional<T> value_; + union { + T value_; + }; }; // Deduction guide to make returning StatusOr less verbose.
diff --git a/src/base/status_or_unittest.cc b/src/base/status_or_unittest.cc index 16489b3..f1b7721 100644 --- a/src/base/status_or_unittest.cc +++ b/src/base/status_or_unittest.cc
@@ -17,7 +17,11 @@ #include "perfetto/ext/base/status_or.h" #include <memory> +#include <string> #include <utility> +#include <vector> + +#include "perfetto/ext/base/status_macros.h" #include "test/gtest_and_gmock.h" @@ -66,5 +70,114 @@ ASSERT_DEATH_IF_SUPPORTED(std::move(result).ValueOrDie(), ""); } +// Counts the live objects of its type, so that a value constructed or +// destroyed the wrong number of times shows. +struct Counted { + static int live; + explicit Counted(int v) : value(v) { ++live; } + Counted(const Counted& other) : value(other.value) { ++live; } + Counted(Counted&& other) noexcept : value(other.value) { ++live; } + Counted& operator=(const Counted&) = default; + Counted& operator=(Counted&&) = default; + ~Counted() { --live; } + int value; +}; +int Counted::live = 0; + +TEST(StatusOrTest, CopyAndMove) { + { + StatusOr<Counted> ok(Counted(1)); + StatusOr<Counted> err(ErrStatus("bad")); + EXPECT_EQ(Counted::live, 1); + + StatusOr<Counted> ok_copy(ok); + StatusOr<Counted> err_copy(err); + EXPECT_EQ(Counted::live, 2); + EXPECT_EQ(ok_copy->value, 1); + EXPECT_EQ(err_copy.status().message(), "bad"); + + // A moved-from StatusOr keeps its moved-from value. + StatusOr<Counted> ok_moved(std::move(ok)); + StatusOr<Counted> err_moved(std::move(err)); + EXPECT_EQ(Counted::live, 3); + EXPECT_EQ(ok_moved->value, 1); + EXPECT_EQ(err_moved.status().message(), "bad"); + } + EXPECT_EQ(Counted::live, 0); +} + +// Every transition between holding a value and holding an error. +TEST(StatusOrTest, Assign) { + { + StatusOr<Counted> a(Counted(1)); + StatusOr<Counted> b(Counted(2)); + StatusOr<Counted> err(ErrStatus("bad")); + + a = b; // Value over value. + EXPECT_EQ(a->value, 2); + EXPECT_EQ(Counted::live, 2); + + a = err; // Error over value. + EXPECT_FALSE(a.ok()); + EXPECT_EQ(a.status().message(), "bad"); + EXPECT_EQ(Counted::live, 1); + + a = b; // Value over error. + EXPECT_EQ(a->value, 2); + EXPECT_EQ(Counted::live, 2); + + a = StatusOr<Counted>(Counted(3)); // Value over value, moved. + EXPECT_EQ(a->value, 3); + EXPECT_EQ(Counted::live, 2); + + a = StatusOr<Counted>(ErrStatus("worse")); // Error over value, moved. + EXPECT_EQ(a.status().message(), "worse"); + EXPECT_EQ(Counted::live, 1); + + a = StatusOr<Counted>(Counted(4)); // Value over error, moved. + EXPECT_EQ(a->value, 4); + EXPECT_EQ(Counted::live, 2); + + StatusOr<Counted>& self = a; + a = self; // Self assignment. + EXPECT_EQ(a->value, 4); + EXPECT_EQ(Counted::live, 2); + } + EXPECT_EQ(Counted::live, 0); +} + +TEST(StatusOrTest, StatusMovesErrorOut) { + StatusOr<std::string> err(ErrStatus("bad")); + Status status = std::move(err).status(); + EXPECT_EQ(status.message(), "bad"); + + // An ok StatusOr keeps its value. + StatusOr<std::string> ok(std::string("value")); + EXPECT_TRUE(std::move(ok).status().ok()); + EXPECT_EQ(ok.value(), "value"); +} + +namespace { + +StatusOr<int> Half(int x) { + if (x % 2 != 0) { + return ErrStatus("odd: %d", x); + } + return x / 2; +} + +StatusOr<int> Quarter(int x) { + ASSIGN_OR_RETURN(int half, Half(x)); + ASSIGN_OR_RETURN(int quarter, Half(half)); + return quarter; +} + +} // namespace + +TEST(StatusOrTest, AssignOrReturn) { + EXPECT_EQ(Quarter(12).value(), 3); + EXPECT_EQ(Quarter(6).status().message(), "odd: 3"); +} + } // namespace base } // namespace perfetto
diff --git a/src/trace_processor/plugins/winscope_importer/winscope_proto_mapping.h b/src/trace_processor/plugins/winscope_importer/winscope_proto_mapping.h index 024b06e..e9493c8 100644 --- a/src/trace_processor/plugins/winscope_importer/winscope_proto_mapping.h +++ b/src/trace_processor/plugins/winscope_importer/winscope_proto_mapping.h
@@ -74,8 +74,7 @@ } } // namespace -inline base::StatusOr<const char* const> GetProtoName( - const std::string& table_name) { +inline base::StatusOr<const char*> GetProtoName(const std::string& table_name) { if (table_name == tables::SurfaceFlingerLayerTable::Name()) { return ".com.android.internal.LayerProto"; }
diff --git a/src/trace_processor/plugins/winscope_proto_to_args_with_defaults/winscope_proto_to_args_with_defaults.cc b/src/trace_processor/plugins/winscope_proto_to_args_with_defaults/winscope_proto_to_args_with_defaults.cc index cdd6c9f..becf927 100644 --- a/src/trace_processor/plugins/winscope_proto_to_args_with_defaults/winscope_proto_to_args_with_defaults.cc +++ b/src/trace_processor/plugins/winscope_proto_to_args_with_defaults/winscope_proto_to_args_with_defaults.cc
@@ -416,7 +416,7 @@ base::ErrStatus("Failed to find %s table.", table_name_str.c_str())); } - base::StatusOr<const char* const> proto_name = + base::StatusOr<const char*> proto_name = util::winscope_proto_mapping::GetProtoName(table_name_str); if (!proto_name.ok()) { return OnFailure(proto_name.status());