You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "taepper (via GitHub)" <gi...@apache.org> on 2026/10/12 13:22:59 UTC
[PR] GH-51834: [C++] Reduce `ARROW_ASSIGN_OR_RAISE` stack consumption with gcc/clang [arrow]
taepper opened a new pull request, #53314:
URL: https://github.com/apache/arrow/pull/53314
### Rationale for this change
@pitrou found in [investigations](https://github.com/apache/arrow/issues/51834) that repeated uses of `ARROW_ASSIGN_OR_RAISE` can lead to very large stack consumptions.
### What changes are included in this PR?
This PR adds a new definition of the `ARROW_ASSIGN_OR_RAISE` macro, which avoids intermediate results using a gcc/clang compiler extension.
### Are these changes tested?
No new feature, all tests still pass.
### Are there any user-facing changes?
No.
### Was AI used for this PR?
In accordance to the [AI generation guidelines](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code), please disclose below whether and how AI was used in this PR.
**PR code and description written by:**
- [X] Human
- [ ] AI
**Reviewed before submission by:**
- [X] Human
- [X] AI
- [ ] Not reviewed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51834: [C++] Reduce `ARROW_ASSIGN_OR_RAISE` stack consumption with gcc/clang [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #53314:
URL: https://github.com/apache/arrow/pull/53314#issuecomment-6145897161
@github-actions crossbow submit -g cpp
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51834: [C++] Reduce `ARROW_ASSIGN_OR_RAISE` stack consumption with gcc/clang [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #53314:
URL: https://github.com/apache/arrow/pull/53314#discussion_r4257916500
##########
cpp/src/arrow/dataset/file_ipc.cc:
##########
@@ -145,9 +145,9 @@ Result<RecordBatchGenerator> IpcFileFormat::ScanBatchesAsync(
auto reopen_reader = [self, options,
source](std::shared_ptr<ipc::RecordBatchFileReader> reader)
-> Future<std::shared_ptr<ipc::RecordBatchFileReader>> {
- ARROW_ASSIGN_OR_RAISE(auto options,
+ ARROW_ASSIGN_OR_RAISE(auto read_options,
Review Comment:
edit: good call.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51834: [C++] Reduce `ARROW_ASSIGN_OR_RAISE` stack consumption with gcc/clang [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #53314:
URL: https://github.com/apache/arrow/pull/53314#issuecomment-6146342916
Revision: f96d9fa622d5564f35bb34d73b26e2d0e856e5e7
Submitted crossbow builds: [ursacomputing/crossbow @ actions-9760cf6651](https://github.com/ursacomputing/crossbow/branches/all?query=actions-9760cf6651)
|Task|Status|
|----|------|
|example-cpp-minimal-build-static|[](https://github.com/ursacomputing/crossbow/actions/runs/38402854781/job/115314665643)|
|example-cpp-minimal-build-static-system-dependency|[](https://github.com/ursacomputing/crossbow/actions/runs/38402853099/job/115314660462)|
|example-cpp-tutorial|[](https://github.com/ursacomputing/crossbow/actions/runs/38402847465/job/115314641390)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/38402848491/job/115314645441)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/38402847145/job/115314641178)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/actions/runs/38402855466/job/115314668087)|
|test-debian-13-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/38402849606/job/115314650635)|
|test-debian-13-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/38402853420/job/115314660562)|
|test-debian-experimental-cpp-gcc-15|[](https://github.com/ursacomputing/crossbow/actions/runs/38402846467/job/115314639099)|
|test-fedora-42-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/38402850804/job/115314653913)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/38402851110/job/115314654451)|
|test-ubuntu-22.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/38402849622/job/115314648899)|
|test-ubuntu-22.04-cpp-emscripten|[](https://github.com/ursacomputing/crossbow/actions/runs/38402846657/job/115314639864)|
|test-ubuntu-22.04-cpp-no-threading|[](https://github.com/ursacomputing/crossbow/actions/runs/38402852797/job/115314659067)|
|test-ubuntu-24.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/38402851871/job/115314656492)|
|test-ubuntu-24.04-cpp-gcc-13-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/38402851729/job/115314656239)|
|test-ubuntu-24.04-cpp-gcc-14|[](https://github.com/ursacomputing/crossbow/actions/runs/38402849905/job/115314650529)|
|test-ubuntu-24.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/38402848291/job/115314645751)|
|test-ubuntu-24.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/38402852156/job/115314657944)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51834: [C++] Reduce `ARROW_ASSIGN_OR_RAISE` stack consumption with gcc/clang [arrow]
Posted by "createcentury (via GitHub)" <gi...@apache.org>.
createcentury commented on PR #53314:
URL: https://github.com/apache/arrow/pull/53314#issuecomment-6128058509
The gcc/clang specialization looks good to me.
I checked the two things this expansion could have quietly broken, and both come out clean:
- **Shadowing.** Moving `lhs` to the outside of the statement expression changes name
lookup inside the initializer, which is what the `file_ipc.cc` rename is about. I went
through the rest of `cpp/src` for the same pattern and that is the only site, so nothing
else needs renaming.
- **Lifetime.** The intermediate `Result` now dies at the end of the statement rather than
the enclosing scope. Nothing binds it by reference — there are no `auto&&` or `auto&` left
hand sides — and the `std::string_view` ones view into storage owned elsewhere, so they're
unaffected.
clang at `-O0` stays where it was, as your table shows, but killing the growth in gcc `-O0`
and in both release builds covers most of the pain.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51834: [C++] Reduce `ARROW_ASSIGN_OR_RAISE` stack consumption with gcc/clang [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #53314:
URL: https://github.com/apache/arrow/pull/53314#issuecomment-6164714129
I'm going to merge now. Thanks @taepper for submitting this PR!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51834: [C++] Reduce `ARROW_ASSIGN_OR_RAISE` stack consumption with gcc/clang [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #53314:
URL: https://github.com/apache/arrow/pull/53314
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51834: [C++] Reduce `ARROW_ASSIGN_OR_RAISE` stack consumption with gcc/clang [arrow]
Posted by "taepper (via GitHub)" <gi...@apache.org>.
taepper commented on PR #53314:
URL: https://github.com/apache/arrow/pull/53314#issuecomment-6141482687
> In the first code snippet, the string_view references a temporary value created via extended syntax, resulting in a dangling reference.
I think you are correct that our Warnings do not currently cover this case explicitly:
```
/// WARNING: ARROW_ASSIGN_OR_RAISE `std::move`s its right operand. If you have
/// an lvalue Result which you *don't* want to move out of cast appropriately.
///
/// WARNING: ARROW_ASSIGN_OR_RAISE is not a single expression; it will not
/// maintain lifetimes of all temporaries in `rexpr` (e.g.
/// `ARROW_ASSIGN_OR_RAISE(auto x, MakeTemp().GetResultRef());`
/// will most likely segfault)!
```
If I understand correctly, the issue arises in your example, because the `std::move(rexpr)` cannot move the `Result<std::string>` and will therefore copy the string to the temporary which will not outlive the statement expression.
IMO this is implicitly covered by this statement
> ARROW_ASSIGN_OR_RAISE is not a single expression; it will not maintain lifetimes of all temporaries in `rexpr`
Because the caller provides a `const Result` it is not unexpected that the evaluation will perform a copy of `rexpr` to a temporary. And the doc already warns regarding this temporary's lifetime
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51834: [C++] Reduce `ARROW_ASSIGN_OR_RAISE` stack consumption with gcc/clang [arrow]
Posted by "rok (via GitHub)" <gi...@apache.org>.
rok commented on code in PR #53314:
URL: https://github.com/apache/arrow/pull/53314#discussion_r4257916500
##########
cpp/src/arrow/dataset/file_ipc.cc:
##########
@@ -145,9 +145,9 @@ Result<RecordBatchGenerator> IpcFileFormat::ScanBatchesAsync(
auto reopen_reader = [self, options,
source](std::shared_ptr<ipc::RecordBatchFileReader> reader)
-> Future<std::shared_ptr<ipc::RecordBatchFileReader>> {
- ARROW_ASSIGN_OR_RAISE(auto options,
+ ARROW_ASSIGN_OR_RAISE(auto read_options,
Review Comment:
Why rename this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51834: [C++] Reduce `ARROW_ASSIGN_OR_RAISE` stack consumption with gcc/clang [arrow]
Posted by "taepper (via GitHub)" <gi...@apache.org>.
taepper commented on code in PR #53314:
URL: https://github.com/apache/arrow/pull/53314#discussion_r4258721635
##########
cpp/src/arrow/dataset/file_ipc.cc:
##########
@@ -145,9 +145,9 @@ Result<RecordBatchGenerator> IpcFileFormat::ScanBatchesAsync(
auto reopen_reader = [self, options,
source](std::shared_ptr<ipc::RecordBatchFileReader> reader)
-> Future<std::shared_ptr<ipc::RecordBatchFileReader>> {
- ARROW_ASSIGN_OR_RAISE(auto options,
+ ARROW_ASSIGN_OR_RAISE(auto read_options,
Review Comment:
Well, because we use `auto` here, we got a compile-time error. Otherwise, `rexpr` would use the `lhs`'s declaration as an uninitialized object.
Even when keeping in mind that
```
auto options = GetReadOptions(*reader->schema(), *self, *options)).ValueUnsafe();
```
would obviously already have been broken before, this breakage is definitely something to consider, given that it can introduce potentially subtle bugs in _broken but working_ consumers of the macro
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-51834: [C++] Reduce `ARROW_ASSIGN_OR_RAISE` stack consumption with gcc/clang [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #53314:
URL: https://github.com/apache/arrow/pull/53314#issuecomment-6125214737
:warning: GitHub issue #51834 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org