From 0a5005ef1364a318f99cbe967b46aa546f2239e0 Mon Sep 17 00:00:00 2001 From: Benjamin Kietzman Date: Mon, 21 Oct 2024 15:34:07 -0500 Subject: [PATCH 1/4] GH-44491: [C++] StatusConstant- cheaply copied const Status --- cpp/src/arrow/result.cc | 13 ++++++---- cpp/src/arrow/result.h | 17 +++++++++++-- cpp/src/arrow/status.cc | 25 ++++++++++++------- cpp/src/arrow/status.h | 31 ++++++++++++++---------- cpp/src/arrow/status_internal.h | 43 +++++++++++++++++++++++++++++++++ cpp/src/arrow/status_test.cc | 13 ++++++++++ 6 files changed, 113 insertions(+), 29 deletions(-) create mode 100644 cpp/src/arrow/status_internal.h diff --git a/cpp/src/arrow/result.cc b/cpp/src/arrow/result.cc index 0bb65acb83181..e76231a411d49 100644 --- a/cpp/src/arrow/result.cc +++ b/cpp/src/arrow/result.cc @@ -20,10 +20,9 @@ #include #include "arrow/util/logging.h" +#include "arrow/status_internal.h" -namespace arrow { - -namespace internal { +namespace arrow::internal { void DieWithMessage(const std::string& msg) { ARROW_LOG(FATAL) << msg; } @@ -31,6 +30,10 @@ void InvalidValueOrDie(const Status& st) { DieWithMessage(std::string("ValueOrDie called on an error: ") + st.ToString()); } -} // namespace internal +Status UninitializedResult() { + static StatusConstant uninitialized_result{StatusCode::UnknownError, + "Uninitialized Result"}; + return uninitialized_result; +} -} // namespace arrow +} // namespace arrow::internal diff --git a/cpp/src/arrow/result.h b/cpp/src/arrow/result.h index 6786d2b3fcbfd..b50b025fdf6a8 100644 --- a/cpp/src/arrow/result.h +++ b/cpp/src/arrow/result.h @@ -39,6 +39,8 @@ ARROW_EXPORT void DieWithMessage(const std::string& msg); ARROW_EXPORT void InvalidValueOrDie(const Status& st); +ARROW_EXPORT Status UninitializedResult(); + } // namespace internal /// A class for representing either a usable value, or an error. @@ -112,7 +114,7 @@ class [[nodiscard]] Result : public util::EqualityComparable> { /// an empty vector, it will actually invoke the default constructor of /// Result. explicit Result() noexcept // NOLINT(runtime/explicit) - : status_(Status::UnknownError("Uninitialized Result")) {} + : status_(internal::UninitializedResult()) {} ~Result() noexcept { Destroy(); } @@ -294,7 +296,18 @@ class [[nodiscard]] Result : public util::EqualityComparable> { /// /// \return The stored non-OK status object, or an OK status if this object /// has a value. - constexpr const Status& status() const { return status_; } + constexpr const Status& status() const& { return status_; } + + /// Gets the stored status object, or an OK status if a `T` value is stored. + /// + /// \return The stored non-OK status object, or an OK status if this object + /// has a value. + Status status() && { + if (ARROW_PREDICT_TRUE(ok())) return Status::OK(); + Status st = std::move(status_); + status_ = internal::UninitializedResult(); + return st; + } /// Gets the stored `T` value. /// diff --git a/cpp/src/arrow/status.cc b/cpp/src/arrow/status.cc index 368e03cac0bd2..34b6336c72911 100644 --- a/cpp/src/arrow/status.cc +++ b/cpp/src/arrow/status.cc @@ -15,7 +15,9 @@ #include #include #include -#include +#ifdef ARROW_EXTRA_ERROR_CONTEXT +# include +#endif #include "arrow/util/logging.h" @@ -26,18 +28,19 @@ Status::Status(StatusCode code, const std::string& msg) Status::Status(StatusCode code, std::string msg, std::shared_ptr detail) { ARROW_CHECK_NE(code, StatusCode::OK) << "Cannot construct ok status with message"; - state_ = new State; - state_->code = code; - state_->msg = std::move(msg); - if (detail != nullptr) { - state_->detail = std::move(detail); - } + state_ = new State{code, std::move(msg), std::move(detail)}; } +// We would prefer that this destructor *not* be inlined, since the vast majority of +// Statuses are OK and so inlining would superflously increase binary size. +Status::State::~State() = default; + void Status::CopyFrom(const Status& s) { - delete state_; + DeleteState(); if (s.state_ == nullptr) { state_ = nullptr; + } else if (s.state_->is_constant) { + state_ = s.state_; } else { state_ = new State(*s.state_); } @@ -160,7 +163,11 @@ void Status::AddContextLine(const char* filename, int line, const char* expr) { ARROW_CHECK(!ok()) << "Cannot add context line to ok status"; std::stringstream ss; ss << "\n" << filename << ":" << line << " " << expr; - state_->msg += ss.str(); + if (state_->is_constant) { + // We can't add context lines to a StatusConstant's state, so copy it now + state_ = new State{code(), message(), detail()}; + } + const_cast(state_)->msg += ss.str(); } #endif diff --git a/cpp/src/arrow/status.h b/cpp/src/arrow/status.h index fb75d963f3a3c..a9570d8126e64 100644 --- a/cpp/src/arrow/status.h +++ b/cpp/src/arrow/status.h @@ -82,6 +82,9 @@ #endif namespace arrow { +namespace internal { +class StatusConstant; +} enum class StatusCode : char { OK = 0, @@ -134,13 +137,7 @@ class ARROW_EXPORT [[nodiscard]] Status : public util::EqualityComparable detail; + bool is_constant = false; + ~State(); }; // OK status has a `NULL` state_. Otherwise, `state_` points to // a `State` structure containing the error code and message(s) - State* state_; + const State* state_; void DeleteState() { - delete state_; - state_ = NULLPTR; + // ARROW-2400: On certain compilers, splitting off the slow path improves + // performance significantly. + if (ARROW_PREDICT_FALSE(state_ != NULLPTR)) { + if (!state_->is_constant) { + delete state_; + } + } } void CopyFrom(const Status& s); inline void MoveFrom(Status& s); + + friend class internal::StatusConstant; }; void Status::MoveFrom(Status& s) { - delete state_; + DeleteState(); state_ = s.state_; s.state_ = NULLPTR; } -Status::Status(const Status& s) - : state_((s.state_ == NULLPTR) ? NULLPTR : new State(*s.state_)) {} +Status::Status(const Status& s) : state_{NULLPTR} { CopyFrom(s); } Status& Status::operator=(const Status& s) { // The following condition catches both aliasing (when this == &s), diff --git a/cpp/src/arrow/status_internal.h b/cpp/src/arrow/status_internal.h new file mode 100644 index 0000000000000..9474716551ab2 --- /dev/null +++ b/cpp/src/arrow/status_internal.h @@ -0,0 +1,43 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#pragma once + +#include "arrow/status.h" +#include "arrow/util/logging.h" + +namespace arrow::internal { + +class StatusConstant { + public: + StatusConstant(StatusCode code, std::string msg, + std::shared_ptr detail = nullptr) + : state_{code, std::move(msg), std::move(detail), /*is_constant=*/true} { + ARROW_CHECK_NE(code, StatusCode::OK) << "Cannot construct ok status constant"; + } + + operator Status() const { // NOLINT(runtime/explicit) + Status st; + st.state_ = &state_; + return st; + } + + private: + Status::State state_; +}; + +} // namespace arrow::internal diff --git a/cpp/src/arrow/status_test.cc b/cpp/src/arrow/status_test.cc index b446705933285..990aadabec8d1 100644 --- a/cpp/src/arrow/status_test.cc +++ b/cpp/src/arrow/status_test.cc @@ -21,6 +21,7 @@ #include #include "arrow/status.h" +#include "arrow/status_internal.h" #include "arrow/testing/gtest_util.h" #include "arrow/testing/matchers.h" @@ -76,6 +77,18 @@ TEST(StatusTest, TestCoverageWarnNotOK) { ARROW_WARN_NOT_OK(Status::Invalid("invalid"), "Expected warning"); } +TEST(StatusTest, StatusConstant) { + internal::StatusConstant constant{StatusCode::Invalid, "default error"}; + Status st = constant; + + ASSERT_EQ(st.code(), StatusCode::Invalid); + ASSERT_EQ(st.message(), "default error"); + ASSERT_EQ(st.detail(), nullptr); + + Status copy = st; + ASSERT_EQ(&st.message(), ©.message()); +} + TEST(StatusTest, AndStatus) { Status a = Status::OK(); Status b = Status::OK(); From 602757f10c98ffcc3b30eb63aafc3b33579608ca Mon Sep 17 00:00:00 2001 From: Benjamin Kietzman Date: Fri, 25 Oct 2024 10:22:54 -0500 Subject: [PATCH 2/4] DeleteState()->noexcept --- cpp/src/arrow/status.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/src/arrow/status.h b/cpp/src/arrow/status.h index a9570d8126e64..1cfaf7cff2081 100644 --- a/cpp/src/arrow/status.h +++ b/cpp/src/arrow/status.h @@ -372,7 +372,7 @@ class ARROW_EXPORT [[nodiscard]] Status : public util::EqualityComparable Date: Fri, 25 Oct 2024 10:34:28 -0500 Subject: [PATCH 3/4] clang-format --- cpp/src/arrow/result.cc | 2 +- cpp/src/arrow/result.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/src/arrow/result.cc b/cpp/src/arrow/result.cc index e76231a411d49..e8c5a57ee7bc0 100644 --- a/cpp/src/arrow/result.cc +++ b/cpp/src/arrow/result.cc @@ -19,8 +19,8 @@ #include -#include "arrow/util/logging.h" #include "arrow/status_internal.h" +#include "arrow/util/logging.h" namespace arrow::internal { diff --git a/cpp/src/arrow/result.h b/cpp/src/arrow/result.h index b50b025fdf6a8..d933c80748fc6 100644 --- a/cpp/src/arrow/result.h +++ b/cpp/src/arrow/result.h @@ -302,7 +302,7 @@ class [[nodiscard]] Result : public util::EqualityComparable> { /// /// \return The stored non-OK status object, or an OK status if this object /// has a value. - Status status() && { + Status status() && { if (ARROW_PREDICT_TRUE(ok())) return Status::OK(); Status st = std::move(status_); status_ = internal::UninitializedResult(); From f7d3fffa53a07548fa42fc15d48a001bf141b5b6 Mon Sep 17 00:00:00 2001 From: Benjamin Kietzman Date: Fri, 25 Oct 2024 10:49:06 -0500 Subject: [PATCH 4/4] msvc: try fixing visibility for State::~State --- cpp/src/arrow/status.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/src/arrow/status.h b/cpp/src/arrow/status.h index 1cfaf7cff2081..81b6175d3ce7d 100644 --- a/cpp/src/arrow/status.h +++ b/cpp/src/arrow/status.h @@ -361,7 +361,7 @@ class ARROW_EXPORT [[nodiscard]] Status : public util::EqualityComparable detail;