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 091351154251e..bb3e49d0ca933 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(); } @@ -302,7 +304,7 @@ class [[nodiscard]] Result : public util::EqualityComparable> { /// has a value. Status status() && { if (ok()) return Status::OK(); - auto tmp = Status::UnknownError("Uninitialized Result"); + auto tmp = internal::UninitializedResult(); std::swap(status_, tmp); return tmp; } 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();