Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[C++] allocation free error Status #44491

Closed
bkietz opened this issue Oct 21, 2024 · 2 comments
Closed

[C++] allocation free error Status #44491

bkietz opened this issue Oct 21, 2024 · 2 comments

Comments

@bkietz
Copy link
Member

bkietz commented Oct 21, 2024

Describe the enhancement requested

In many situations, it'd be beneficial to have an error Status which was copyable without using the heap. For example, this is a frequent pain point with Result. We want Result to be default constructible even if T isn't, so a defaulted Result is an error but this requires heap allocating a Status::State which we immediately overwrite. Similarly, if the value or error is moved out of a Result we'd like to avoid managing the stored object and wind up heap allocating a flag.

Allowing some Status objects to have static lifetime (for example, by tagging the state_ pointer or adding bool Status::State::is_static) should not have an effect on the critical hot paths of moving a status or skipping deletion of ok status. Copying will get slightly slower since we need to branch on state_->is_static, but that's already quite slow due to heap interaction.

Component(s)

C++

@bkietz
Copy link
Member Author

bkietz commented Oct 21, 2024

FWIW, absl::Status uses pointer tagging for its junk value: https://github.com/abseil/abseil-cpp/blob/master/absl/status/status.h#L901-L903

bkietz added a commit to bkietz/arrow that referenced this issue Oct 21, 2024
pitrou added a commit that referenced this issue Nov 14, 2024
### 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]>
@pitrou
Copy link
Member

pitrou commented Nov 14, 2024

Issue resolved by pull request 44493
#44493

@pitrou pitrou added this to the 19.0.0 milestone Nov 14, 2024
@pitrou pitrou closed this as completed Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants