GH-44491: [C++] StatusConstant- cheaply copied const Status #44493

bkietz · 2024-10-21T20:36:51Z

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

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: [C++] allocation free error Status #44491

github-actions · 2024-10-21T20:37:17Z

⚠️ GitHub issue #44491 has been automatically assigned in GitHub to PR creator.

cpp/src/arrow/status.h

bkietz · 2024-10-25T15:17:49Z

cpp/src/arrow/status_internal.h

+    ARROW_CHECK_NE(code, StatusCode::OK) << "Cannot construct ok status constant";
+  }
+
+  operator Status() const {  // NOLINT(runtime/explicit)


We could alternatively make StatusConstant a subclass of Status

bkietz · 2024-10-25T15:46:35Z

@pitrou @felipecrv which benchmark would be convincing as a demonstration this doesn't slow us down? I don't think we have an explicit status or result benchmark

For now (since it seems to me to have the deepest Status bubbling stack),

@ursabot please benchmark command=cpp-micro --suite-filter=grouper

pitrou · 2024-10-25T15:48:20Z

I don't think we have an explicit status or result benchmark

We do, but for some reason I had stuffed it in type_benchmark.cc

arrow/cpp/src/arrow/type_benchmark.cc

Lines 545 to 555 in 1b40800

    
           BENCHMARK(ErrorSchemeNoError); 
        
           BENCHMARK(ErrorSchemeBool); 
        
           BENCHMARK(ErrorSchemeStatus); 
        
           BENCHMARK(ErrorSchemeResult); 
        
           BENCHMARK(ErrorSchemeException); 
        
           BENCHMARK(ErrorSchemeNoErrorNoInline); 
        
           BENCHMARK(ErrorSchemeBoolNoInline); 
        
           BENCHMARK(ErrorSchemeStatusNoInline); 
        
           BENCHMARK(ErrorSchemeResultNoInline); 
        
           BENCHMARK(ErrorSchemeExceptionNoInline);

bkietz · 2024-10-25T15:55:57Z

@ursabot please benchmark command=cpp-micro --suite-filter=type

ursabot · 2024-10-25T15:56:04Z

Benchmark runs are scheduled for commit f7d3fff. Watch https://buildkite.com/apache-arrow and https://conbench.ursa.dev for updates. A comment will be posted here when the runs are complete.

conbench-apache-arrow · 2024-10-25T22:23:19Z

Thanks for your patience. Conbench analyzed the 3 benchmarking runs that have been run so far on PR commit f7d3fff.

There were 4 benchmark results indicating a performance regression:

Pull Request Run on amd64-m5-4xlarge-linux at 2024-10-25 16:10:52Z
- ErrorSchemeBoolNoInline (C++) with source=cpp-micro, suite=arrow-type-benchmark
Pull Request Run on amd64-c6a-4xlarge-linux at 2024-10-25 16:06:02Z
- ErrorSchemeBoolNoInline (C++) with source=cpp-micro, suite=arrow-type-benchmark
and 2 more (see the report linked below)

The full Conbench report has more details.

felipecrv · 2024-10-30T22:12:17Z

@pitrou @felipecrv which benchmark would be convincing as a demonstration this doesn't slow us down? I don't think we have an explicit status or result benchmark

Can you check the binary size impact with bloaty?

The performance impact is very tiny but everywhere a Status gets destroyed (all over the place). I think checking binary size is a good proxy to keeping the shape of the code the same.

cpp/src/arrow/status.cc

cpp/src/arrow/status_internal.h

pitrou · 2024-11-04T17:37:40Z

cpp/src/arrow/status_internal.h

+  StatusConstant(StatusCode code, std::string msg,
+                 std::shared_ptr<StatusDetail> detail = nullptr)
+      : state_{code, std::move(msg), std::move(detail), /*is_constant=*/true} {
+    ARROW_CHECK_NE(code, StatusCode::OK)


Note that ARROW_CHECK_NE is always executed even in non-debug builds, do you want that here?

It's probably unnecessary for constants; I was copying from Status::Status

cpp/src/arrow/status.h

pitrou · 2024-11-04T17:42:18Z

cpp/src/arrow/status.cc

+    // We can't add context lines to a StatusConstant's state, so copy it now
+    state_ = new State{code(), message(), detail()};
+  }
+  const_cast<State*>(state_)->msg += ss.str();


Why did we make the state const if we need the const_cast here?

I did this to satisfy myself that this line was the only place we ever mutate state_. I can remove the const now that I'm sure, but... since this line is the only place we ever mutate state_, I thought it'd be better to leave it so that in the future if we add another mutation we won't forget to CopyOnWrite any constants

bkietz · 2024-11-04T17:42:54Z

Can you check the binary size impact with bloaty?

$ $ bloaty build/relwithdebinfo/libarrow.so -- main-build/relwithdebinfo/libarrow.so
    FILE SIZE        VM SIZE
 --------------  --------------
  +0.1% +14.2Ki  [ = ]       0    .debug_str
   +14% +1.17Ki  [ = ]       0    [Unmapped]
  +0.0% +1.14Ki  [ = ]       0    .strtab
  +0.0%    +250  +0.0%    +250    .dynstr
  [ = ]       0  +0.0%     +64    .bss
 -100.0%     +50 -100.0%     +30    [7 Others]
  -0.1%     -40  -0.1%     -40    .got.plt
  -0.0%     -72  -0.0%     -72    .dynsym
  -0.1%     -80  -0.1%     -80    .plt
  -0.1%    -120  -0.1%    -120    .rela.plt
  -0.0%    -288  [ = ]       0    .symtab
  -0.1%    -533  [ = ]       0    .debug_abbrev
  -1.2% -3.74Ki  -1.2% -3.74Ki    .gcc_except_table
  -0.4% -10.00Ki  [ = ]       0    .debug_str_offsets
  -1.3% -13.4Ki  -1.3% -13.4Ki    .eh_frame

  # This seems the most relevant data point; no significant change
  -0.6% -48.0Ki  -0.6% -48.0Ki    .text

  -2.2% -86.1Ki  [ = ]       0    .debug_addr
  -1.7%  -146Ki  [ = ]       0    .debug_line
  -3.4%  -164Ki  [ = ]       0    .debug_rnglists
  -2.1%  -347Ki  [ = ]       0    .debug_loclists
  -1.3%  -539Ki  [ = ]       0    .debug_info
  -1.2% -1.31Mi  -0.4% -65.1Ki    TOTAL```

pitrou

+1 on the principle, see comments

bkietz · 2024-11-04T17:57:48Z

@ursabot please benchmark command=cpp-micro --suite-filter=type --benchmark-filter=Error

ursabot · 2024-11-04T17:57:53Z

Benchmark runs are scheduled for commit 86741d6. Watch https://buildkite.com/apache-arrow and https://conbench.ursa.dev for updates. A comment will be posted here when the runs are complete.

conbench-apache-arrow · 2024-11-04T18:19:18Z

Thanks for your patience. Conbench analyzed the 3 benchmarking runs that have been run so far on PR commit 86741d6.

There were 2 benchmark results indicating a performance regression:

Pull Request Run on amd64-c6a-4xlarge-linux at 2024-11-04 18:08:18Z
- ErrorSchemeBoolNoInline (C++) with source=cpp-micro, suite=arrow-type-benchmark
Pull Request Run on amd64-m5-4xlarge-linux at 2024-11-04 18:12:34Z
- ErrorSchemeExceptionNoInline (C++) with source=cpp-micro, suite=arrow-type-benchmark

The full Conbench report has more details.

bkietz · 2024-11-04T19:20:47Z

Those regressions have got to be flukes; they don't even touch Status

pitrou · 2024-11-04T19:26:14Z

Yes, the code changes probably trigger random variation in code placement in these benchmarks, that could explain the false regressions.

github-actions bot added the Component: C++ label Oct 21, 2024

bkietz mentioned this pull request Oct 21, 2024

GH-44464: [C++] Added rvalue-reference-qualified overload for arrow::Result::status() returning value instead of reference #44477

Merged

github-actions bot added the awaiting committer review Awaiting committer review label Oct 21, 2024

felipecrv reviewed Oct 21, 2024

View reviewed changes

cpp/src/arrow/status.h Outdated Show resolved Hide resolved

cpp/src/arrow/status.h Outdated Show resolved Hide resolved

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Oct 21, 2024

bkietz force-pushed the 44491-static-status branch from 0634658 to 0a5005e Compare October 25, 2024 15:11

bkietz marked this pull request as ready for review October 25, 2024 15:12

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Oct 25, 2024

bkietz changed the title ~~GH-44491: [C++] Static Status draft~~ GH-44491: [C++] StatusConstant- cheaply copied const Status Oct 25, 2024

bkietz commented Oct 25, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting change review Awaiting change review awaiting changes Awaiting changes labels Oct 25, 2024

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Oct 25, 2024

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Oct 25, 2024

github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Oct 30, 2024

felipecrv reviewed Oct 30, 2024

View reviewed changes

cpp/src/arrow/status.cc Outdated Show resolved Hide resolved

cpp/src/arrow/status_internal.h Outdated Show resolved Hide resolved

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Nov 4, 2024

pitrou reviewed Nov 4, 2024

View reviewed changes

cpp/src/arrow/status.h Outdated Show resolved Hide resolved

pitrou reviewed Nov 4, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes awaiting change review Awaiting change review and removed awaiting change review Awaiting change review awaiting changes Awaiting changes labels Nov 4, 2024

bkietz added 6 commits November 8, 2024 11:14

apacheGH-44491: [C++] StatusConstant- cheaply copied const Status

bcacd67

DeleteState()->noexcept

db21e72

clang-format

70e13f4

msvc: try fixing visibility for State::~State

417fbde

review comments

0ca52d1

Pack Struct::State a little with reordered is_constant

d13d04e

bkietz force-pushed the 44491-static-status branch from 86741d6 to d13d04e Compare November 8, 2024 17:17

bkietz requested a review from pitrou November 12, 2024 16:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-44491: [C++] StatusConstant- cheaply copied const Status #44493

GH-44491: [C++] StatusConstant- cheaply copied const Status #44493

bkietz commented Oct 21, 2024 •

edited

Loading

github-actions bot commented Oct 21, 2024

bkietz Oct 25, 2024

bkietz commented Oct 25, 2024

pitrou commented Oct 25, 2024 •

edited

Loading

bkietz commented Oct 25, 2024

ursabot commented Oct 25, 2024

conbench-apache-arrow bot commented Oct 25, 2024

felipecrv commented Oct 30, 2024

pitrou Nov 4, 2024

bkietz Nov 4, 2024

pitrou Nov 4, 2024

bkietz Nov 4, 2024

bkietz commented Nov 4, 2024

pitrou left a comment

bkietz commented Nov 4, 2024

ursabot commented Nov 4, 2024

conbench-apache-arrow bot commented Nov 4, 2024

bkietz commented Nov 4, 2024

pitrou commented Nov 4, 2024

GH-44491: [C++] StatusConstant- cheaply copied const Status #44493

Are you sure you want to change the base?

GH-44491: [C++] StatusConstant- cheaply copied const Status #44493

Conversation

bkietz commented Oct 21, 2024 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Oct 21, 2024

bkietz Oct 25, 2024

Choose a reason for hiding this comment

bkietz commented Oct 25, 2024

pitrou commented Oct 25, 2024 • edited Loading

bkietz commented Oct 25, 2024

ursabot commented Oct 25, 2024

conbench-apache-arrow bot commented Oct 25, 2024

felipecrv commented Oct 30, 2024

pitrou Nov 4, 2024

Choose a reason for hiding this comment

bkietz Nov 4, 2024

Choose a reason for hiding this comment

pitrou Nov 4, 2024

Choose a reason for hiding this comment

bkietz Nov 4, 2024

Choose a reason for hiding this comment

bkietz commented Nov 4, 2024

pitrou left a comment

Choose a reason for hiding this comment

bkietz commented Nov 4, 2024

ursabot commented Nov 4, 2024

conbench-apache-arrow bot commented Nov 4, 2024

bkietz commented Nov 4, 2024

pitrou commented Nov 4, 2024

bkietz commented Oct 21, 2024 •

edited

Loading

pitrou commented Oct 25, 2024 •

edited

Loading