Skip to content

Commit

Permalink
GH-44491: [C++] StatusConstant- cheaply copied const Status (#44493)
Browse files Browse the repository at this point in the history
### Rationale for this change

It'd be convenient to construct placeholder error Status cheaply.

### What changes are included in this PR?

A constant error Status can now be constructed wrapped in a function like
```c++
Status UninitializedResult() {
  static StatusConstant uninitialized_result{StatusCode::UnknownError,
                                             "Uninitialized Result<T>"};
  return uninitialized_result;
}
```

Copying the constant status is relatively cheap (no heap interaction), so it's suitable for use as a placeholder error status.

Added `bool Status::State::is_constant` which causes copies to be shallow and skips destruction. Also `Status::state_` is a const pointer now; this helps to ensure that it is obvious when we mutate state_ (as in AddContextLine).

### Are these changes tested?

Minimal unit test added. The main consideration is probably benchmarks to make sure hot paths don't get much slower.

### Are there any user-facing changes?

This API is not currently public; no user-facing changes.

* GitHub Issue: #44491

Lead-authored-by: Benjamin Kietzman <[email protected]>
Co-authored-by: Antoine Pitrou <[email protected]>
Signed-off-by: Antoine Pitrou <[email protected]>
  • Loading branch information
bkietz and pitrou authored Nov 14, 2024
1 parent 474c675 commit 29e8ea0
Show file tree
Hide file tree
Showing 7 changed files with 114 additions and 31 deletions.
9 changes: 2 additions & 7 deletions cpp/src/arrow/buffer_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -511,13 +511,8 @@ TEST(TestBuffer, CopySliceEmpty) {
}

TEST(TestBuffer, ToHexString) {
const uint8_t data_array[] = "\a0hex string\xa9";
std::basic_string<uint8_t> data_str = data_array;

auto data = reinterpret_cast<const uint8_t*>(data_str.c_str());

Buffer buf(data, data_str.size());

const std::string data_str = "\a0hex string\xa9";
Buffer buf(data_str);
ASSERT_EQ(buf.ToHexString(), std::string("073068657820737472696E67A9"));
}

Expand Down
13 changes: 8 additions & 5 deletions cpp/src/arrow/result.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,18 +19,21 @@

#include <string>

#include "arrow/status_internal.h"
#include "arrow/util/logging.h"

namespace arrow {

namespace internal {
namespace arrow::internal {

void DieWithMessage(const std::string& msg) { ARROW_LOG(FATAL) << msg; }

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<T>"};
return uninitialized_result;
}

} // namespace arrow
} // namespace arrow::internal
8 changes: 5 additions & 3 deletions cpp/src/arrow/result.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -112,7 +114,7 @@ class [[nodiscard]] Result : public util::EqualityComparable<Result<T>> {
/// an empty vector, it will actually invoke the default constructor of
/// Result.
explicit Result() noexcept // NOLINT(runtime/explicit)
: status_(Status::UnknownError("Uninitialized Result<T>")) {}
: status_(internal::UninitializedResult()) {}

~Result() noexcept { Destroy(); }

Expand Down Expand Up @@ -301,8 +303,8 @@ class [[nodiscard]] Result : public util::EqualityComparable<Result<T>> {
/// \return The stored non-OK status object, or an OK status if this object
/// has a value.
Status status() && {
if (ok()) return Status::OK();
auto tmp = Status::UnknownError("Uninitialized Result<T>");
if (ARROW_PREDICT_TRUE(ok())) return Status::OK();
auto tmp = internal::UninitializedResult();
std::swap(status_, tmp);
return tmp;
}
Expand Down
23 changes: 15 additions & 8 deletions cpp/src/arrow/status.cc
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@
#include <cassert>
#include <cstdlib>
#include <iostream>
#include <sstream>
#ifdef ARROW_EXTRA_ERROR_CONTEXT
# include <sstream>
#endif

#include "arrow/util/logging.h"

Expand All @@ -26,18 +28,19 @@ Status::Status(StatusCode code, const std::string& msg)

Status::Status(StatusCode code, std::string msg, std::shared_ptr<StatusDetail> 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, /*is_constant=*/false, std::move(msg), std::move(detail)};
}

void Status::CopyFrom(const Status& s) {
delete state_;
if (ARROW_PREDICT_FALSE(state_ != NULL)) {
if (!state_->is_constant) {
DeleteState();
}
}
if (s.state_ == nullptr) {
state_ = nullptr;
} else if (s.state_->is_constant) {
state_ = s.state_;
} else {
state_ = new State(*s.state_);
}
Expand Down Expand Up @@ -160,6 +163,10 @@ 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;
if (state_->is_constant) {
// We can't add context lines to a StatusConstant's state, so copy it now
state_ = new State{code(), /*is_constant=*/false, message(), detail()};
}
state_->msg += ss.str();
}
#endif
Expand Down
26 changes: 18 additions & 8 deletions cpp/src/arrow/status.h
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,9 @@
#endif

namespace arrow {
namespace internal {
class StatusConstant;
}

enum class StatusCode : char {
OK = 0,
Expand Down Expand Up @@ -135,10 +138,10 @@ class ARROW_EXPORT [[nodiscard]] Status : public util::EqualityComparable<Status
// Create a success status.
constexpr Status() noexcept : state_(NULLPTR) {}
~Status() noexcept {
// ARROW-2400: On certain compilers, splitting off the slow path improves
// performance significantly.
if (ARROW_PREDICT_FALSE(state_ != NULL)) {
DeleteState();
if (!state_->is_constant) {
DeleteState();
}
}
}

Expand Down Expand Up @@ -366,29 +369,36 @@ class ARROW_EXPORT [[nodiscard]] Status : public util::EqualityComparable<Status
private:
struct State {
StatusCode code;
bool is_constant;
std::string msg;
std::shared_ptr<StatusDetail> detail;
};
// OK status has a `NULL` state_. Otherwise, `state_` points to
// a `State` structure containing the error code and message(s)
State* state_;

void DeleteState() {
void DeleteState() noexcept {
// ARROW-2400: On certain compilers, splitting off the slow path improves
// performance significantly.
delete state_;
state_ = NULLPTR;
}
void CopyFrom(const Status& s);
inline void MoveFrom(Status& s);

friend class internal::StatusConstant;
};

void Status::MoveFrom(Status& s) {
delete state_;
if (ARROW_PREDICT_FALSE(state_ != NULL)) {
if (!state_->is_constant) {
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),
Expand Down
44 changes: 44 additions & 0 deletions cpp/src/arrow/status_internal.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// 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<StatusDetail> detail = nullptr)
: state_{code, /*is_constant=*/true, std::move(msg), std::move(detail)} {
ARROW_CHECK_NE(code, StatusCode::OK)
<< "StatusConstant is not intended for use with OK status codes";
}

operator Status() { // NOLINT(runtime/explicit)
Status st;
st.state_ = &state_;
return st;
}

private:
Status::State state_;
};

} // namespace arrow::internal
22 changes: 22 additions & 0 deletions cpp/src/arrow/status_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include <gtest/gtest.h>

#include "arrow/status.h"
#include "arrow/status_internal.h"
#include "arrow/testing/gtest_util.h"
#include "arrow/testing/matchers.h"

Expand Down Expand Up @@ -76,6 +77,27 @@ 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(), &copy.message());
Status moved = std::move(st);
ASSERT_EQ(&copy.message(), &moved.message());
ASSERT_OK(st);

Status other = constant;
ASSERT_EQ(other.code(), StatusCode::Invalid);
ASSERT_EQ(other.message(), "default error");
ASSERT_EQ(other.detail(), nullptr);
ASSERT_EQ(&other.message(), &moved.message());
}

TEST(StatusTest, AndStatus) {
Status a = Status::OK();
Status b = Status::OK();
Expand Down

0 comments on commit 29e8ea0

Please sign in to comment.