You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "stenlarsson (via GitHub)" <gi...@apache.org> on 2024/02/08 19:50:59 UTC
[PR] GH-39582: [C++][Acero] Increase size of Acero TempStack [arrow]
stenlarsson opened a new pull request, #40007:
URL: https://github.com/apache/arrow/pull/40007
Certain Acero execution plans can cause an overflow of the TempVectorStack initialized by the QueryContext, and increasing the size of the stack fixes the problem. I don't know exactly what causes the overflow, so I haven't written a test for it.
Fixes #39582.
**This PR contains a "Critical Fix".**
--
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-39582: [C++][Acero] Increase size of Acero TempStack [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #40007:
URL: https://github.com/apache/arrow/pull/40007
--
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-39582: [C++][Acero] Increase size of Acero TempStack [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #40007:
URL: https://github.com/apache/arrow/pull/40007#issuecomment-1964493410
@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-39582: [C++][Acero] Increase size of Acero TempStack [arrow]
Posted by "westonpace (via GitHub)" <gi...@apache.org>.
westonpace commented on PR #40007:
URL: https://github.com/apache/arrow/pull/40007#issuecomment-1936686262
Hmm, this temp stack is only (I think) used in the hash-join. It's basically a stack allocator. Allocations made on the allocator should be RAII guarded to release when they finish. So either there is a bug and these are leaking somehow (I feel like this would be more reproducible) or maybe some of these stack variables are being help across a future boundary and that's causing some re-entrancy which causes the stack to run out of space (I like this explanation because it seems like it's environment specific and threading issues can often be environment statistic). Or, maybe it's only caused by certain input data?
Either way, this stack doesn't take up very much memory in the grand scheme of things. I don't think there is much harm in increasing this value.
--
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-39582: [C++][Acero] Increase size of Acero TempStack [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #40007:
URL: https://github.com/apache/arrow/pull/40007#issuecomment-1959691532
You could either create a static constructor:
```c++
template <typename T>
class TempVectorHolder {
friend class TempVectorStack;
public:
static Result<TempVectorHolder> Make(TempVectorStack* stack, uint32_t num_elements) {
TempVectorHolder holder{stack, nullptr, 0, num_elements};
ARROW_RETURN_NOT_OK(stack->alloc(num_elements * sizeof(T), &holder.data_, &holder.id_));
return holder;
}
```
or, conversely, move the typed allocation API into TempVectorStack:
```c++
class ARROW_EXPORT TempVectorStack {
template <typename>
friend class TempVectorHolder;
public:
template <typename T>
Result<TempVectorHolder<T>> AllocateVector(uint32_t num_elements) {
TempVectorHolder holder{this, nullptr, 0, num_elements};
ARROW_RETURN_NOT_OK(alloc(num_elements * sizeof(T), &holder.data_, &holder.id_));
return holder;
}
```
--
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-39582: [C++][Acero] Increase size of Acero TempStack [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #40007:
URL: https://github.com/apache/arrow/pull/40007#issuecomment-1964501029
Revision: ec3fd3bc2400fb32893306e14744af7af0b73b08
Submitted crossbow builds: [ursacomputing/crossbow @ actions-fe3111b10f](https://github.com/ursacomputing/crossbow/branches/all?query=actions-fe3111b10f)
|Task|Status|
|----|------|
|test-alpine-linux-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/8051825132/job/21990608244)|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions/runs/8051825130/job/21990608176)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/8051825254/job/21990608710)|
|test-conda-cpp-valgrind|[](https://github.com/ursacomputing/crossbow/runs/21990611744)|
|test-cuda-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/8051825626/job/21990610336)|
|test-debian-11-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions/runs/8051825288/job/21990608791)|
|test-debian-11-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions/runs/8051825311/job/21990609171)|
|test-fedora-39-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/8051825604/job/21990610178)|
|test-ubuntu-20.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/8051825454/job/21990609625)|
|test-ubuntu-20.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions/runs/8051825290/job/21990608831)|
|test-ubuntu-20.04-cpp-minimal-with-formats|[](https://github.com/ursacomputing/crossbow/actions/runs/8051825114/job/21990608250)|
|test-ubuntu-20.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions/runs/8051825471/job/21990609590)|
|test-ubuntu-22.04-cpp|[](https://github.com/ursacomputing/crossbow/actions/runs/8051825608/job/21990610180)|
|test-ubuntu-22.04-cpp-20|[](https://github.com/ursacomputing/crossbow/actions/runs/8051826002/job/21990611283)|
|test-ubuntu-22.04-cpp-no-threading|[](https://github.com/ursacomputing/crossbow/actions/runs/8051825369/job/21990609532)|
--
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-39582: [C++][Acero] Increase size of Acero TempStack [arrow]
Posted by "stenlarsson (via GitHub)" <gi...@apache.org>.
stenlarsson commented on PR #40007:
URL: https://github.com/apache/arrow/pull/40007#issuecomment-1961092716
I tried to implement this, but it is unfortunately beyond my abilities. If the TempVectorHolder returns a status, so does every method using it, and all methods using those methods, and so on. It is a huge change, and I got lost along the way.
Raising an exception will have to do.
--
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-39582: [C++][Acero] Increase size of Acero TempStack [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #40007:
URL: https://github.com/apache/arrow/pull/40007#issuecomment-1964560352
I'd post a non-binding +1.
--
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-39582: [C++][Acero] Increase size of Acero TempStack [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #40007:
URL: https://github.com/apache/arrow/pull/40007#issuecomment-1964491598
Ok, fair enough. I've now turned the exception into a regular check.
--
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-39582: [C++][Acero] Increase size of Acero TempStack [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #40007:
URL: https://github.com/apache/arrow/pull/40007#issuecomment-1936990925
Thanks for reviewing this!
OK. Let's merge this for now. Let's investigating this later. (I hope that this can be reproducible on my environment...)
@stenlarsson Could you update the PR description to describe this problem more deeper like the associated issue, this is not a real fix and we should investigate/fix this later before we merge this? We will use the PR title and description for commit message.
--
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-39582: [C++][Acero] Increase size of Acero TempStack [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #40007:
URL: https://github.com/apache/arrow/pull/40007#issuecomment-1947984130
A regular error if possible, or at least a controlled abort rather than memory corruption.
--
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-39582: [C++][Acero] Increase size of Acero TempStack [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #40007:
URL: https://github.com/apache/arrow/pull/40007#issuecomment-1934833213
:warning: GitHub issue #39582 **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
Re: [PR] GH-39582: [C++][Acero] Increase size of Acero TempStack [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #40007:
URL: https://github.com/apache/arrow/pull/40007#issuecomment-1940811004
> We can work on it in this PR or a separated PR.
Let's do it in 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-39582: [C++][Acero] Increase size of Acero TempStack [arrow]
Posted by "zanmato1984 (via GitHub)" <gi...@apache.org>.
zanmato1984 commented on PR #40007:
URL: https://github.com/apache/arrow/pull/40007#issuecomment-1943609318
Another issue of crash caused by this stack overflow has been reported in #39951.
--
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-39582: [C++][Acero] Increase size of Acero TempStack [arrow]
Posted by "stenlarsson (via GitHub)" <gi...@apache.org>.
stenlarsson commented on PR #40007:
URL: https://github.com/apache/arrow/pull/40007#issuecomment-1959658995
Returning an error status is not an option since the `TempVectorStack::alloc` function is used in the `TempVectorHolder` constructor, so I decided to throw an `std::runtime_error` exception. I couldn't find any examples of throwing exceptions in the compute module, so please let me know if there is a better way abort.
--
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-39582: [C++][Acero] Increase size of Acero TempStack [arrow]
Posted by "stenlarsson (via GitHub)" <gi...@apache.org>.
stenlarsson commented on PR #40007:
URL: https://github.com/apache/arrow/pull/40007#issuecomment-1959729654
Can you really return a `TempVectorHolder` like that? I think that is beyond my limited C++ skills unfortunately.
--
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-39582: [C++][Acero] Increase size of Acero TempStack [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #40007:
URL: https://github.com/apache/arrow/pull/40007#issuecomment-1959779533
By defining a [move constructor](https://en.cppreference.com/w/cpp/language/move_constructor) and [assignment operator](https://en.cppreference.com/w/cpp/language/move_assignment), like this:
```c++
template <typename T>
class TempVectorHolder {
friend class TempVectorStack;
public:
~TempVectorHolder() {
if (stack_) {
stack_->release(id_, num_elements_ * sizeof(T));
}
}
TempVectorHolder& operator=(TempVectorHolder&& other) {
stack_ = other.stack_;
other.stack_ = NULLPTR;
data_ = other.data_;
other.data_ = NULLPTR;
id_ = other.id_;
num_elements_ = other.num_elements_;
return *this;
}
TempVectorHolder(TempVectorHolder&& other) {
*this = std::move(other);
}
T* mutable_data() { return reinterpret_cast<T*>(data_); }
private:
TempVectorStack* stack_ = NULLPTR;
uint8_t* data_;
int id_;
uint32_t num_elements_;
};
```
--
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-39582: [C++][Acero] Increase size of Acero TempStack [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #40007:
URL: https://github.com/apache/arrow/pull/40007#issuecomment-1964572925
Thank you @zanmato1984 !
--
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-39582: [C++][Acero] Increase size of Acero TempStack [arrow]
Posted by "stenlarsson (via GitHub)" <gi...@apache.org>.
stenlarsson commented on PR #40007:
URL: https://github.com/apache/arrow/pull/40007#issuecomment-1940531791
I have updated the description.
It is unfortunately that you cannot reproduce the issue, because in a debug build the assertion fails reliably every time on my computer.
--
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-39582: [C++][Acero] Increase size of Acero TempStack [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #40007:
URL: https://github.com/apache/arrow/pull/40007#issuecomment-1965625849
After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 9a7662b41b77a40a76b07435b3eff4fec7454596.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/22010566089) has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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-39582: [C++][Acero] Increase size of Acero TempStack [arrow]
Posted by "stenlarsson (via GitHub)" <gi...@apache.org>.
stenlarsson commented on PR #40007:
URL: https://github.com/apache/arrow/pull/40007#issuecomment-1959754364
If I understand this correctly, the purpose of the TempVectorHolder is to release the memory in the destructor, as if the vector was allocated on the stack. How can you return such an object?
--
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-39582: [C++][Acero] Increase size of Acero TempStack [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #40007:
URL: https://github.com/apache/arrow/pull/40007#issuecomment-1959734495
It's probably possible, yes. It's just a bunch of pointers and integers.
--
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-39582: [C++][Acero] Increase size of Acero TempStack [arrow]
Posted by "stenlarsson (via GitHub)" <gi...@apache.org>.
stenlarsson commented on PR #40007:
URL: https://github.com/apache/arrow/pull/40007#issuecomment-1947821369
> Regardless of this, can we please check for stack overflows instead of letting them hang the process?
What exactly should happen in case of a stack overflow?
--
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-39582: [C++][Acero] Increase size of Acero TempStack [arrow]
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #40007:
URL: https://github.com/apache/arrow/pull/40007#issuecomment-1939073708
Regardless of this, can we please check for stack overflows instead of letting them hang the process?
--
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-39582: [C++][Acero] Increase size of Acero TempStack [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #40007:
URL: https://github.com/apache/arrow/pull/40007#issuecomment-1939953829
Ah, it's a good idea.
We can work on it in this PR or a separated 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