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

[bkietz]

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]

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

[felipecrv]

Probably better to have a single condition here to make it obvious that the rest of the function is the cold code:

if (ARROW_PREDICT_TRUE(state_ == NULL || state_->is_static)) return;

[bkietz]

I agree that this is more obvious for human readers, but I think the priority here is to make it obvious to the compiler that the hottest condition is state_ == NULL, independent of any other consideration

[felipecrv]

I'm saying it would be better to the compiler as well not only humans. What the previous code achieved with the ARROW_PREDICT_FALSE was telling the compiler that DeleteState() is code and shouldn't be inlined. Since lots of Status are destroyed all over the place, that reduces binary size and avoids pollution of the I-cache with cold code.

    if (ARROW_PREDICT_FALSE(state_ != NULL)) {
      DeleteState();
    }

[bkietz]

I think I see what you mean, how about:

Suggested change

	if (ARROW_PREDICT_TRUE(state_ == NULL)) return;
	if (ARROW_PREDICT_FALSE(state_->is_static)) return;
	if (ARROW_PREDICT_FALSE(state_ != NULL)) {
	if (ARROW_PREDICT_FALSE(state_->is_static)) return;
	delete state_;

Then it should still be clear that State::~State() is cold. I could also explicitly move the definition of State::~State() into status.cc, which should prevent inlining

[bkietz]

aargh visibility

[felipecrv]

I think the previous structure is the best: call DeleteState after a single unlikely condition. And the is_static check lives inside DeleteState.

[bkietz]

We could alternatively make StatusConstant a subclass of Status

[bkietz]

@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]

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]

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

[ursabot]

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]

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]

@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.

[felipecrv]

My guess is that it's better to have this inlined, but avoid inlining DeleteState (which is effectively what the previous implementation was achieving with

    if (ARROW_PREDICT_FALSE(state_ != NULL)) {
      DeleteState();
    }

[bkietz]

I'll try writing out the conditions and keeping DeleteState non-inline

[felipecrv]

Well, we could, but it's counter to the reason of StatusConstant existing. Message should be StatusConstant is not meant to be used to construct an OK status"