You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "paleolimbot (via GitHub)" <gi...@apache.org> on 2023/09/05 13:06:13 UTC
[GitHub] [arrow] paleolimbot opened a new pull request, #37565: GH-37513: [R] Use `SafeCallIntoR()` to call garbage collector after a failed allocation
paleolimbot opened a new pull request, #37565:
URL: https://github.com/apache/arrow/pull/37565
### Rationale for this change
The `gc_memory_pool()` is the one we use almost everywhere in the R package. It uses a special allocation mechanism that calls into R to run the garbage collector after a failed allocation (in case there are any large objects that can be removed). In the case where an allocation happens on another thread (most of the time when running exec plans), the call into R may cause a crash: even though the memory pool was ensuring serialized access using a mutex, this is not sufficient for R (for reasons I don't understand).
### What changes are included in this PR?
Use `SafeCallIntoR()` to run the garbage collector instead. This ensures that the calling thread is used for any call into R (or errors if this is not possible).
### Are these changes tested?
Yes: there is an existing test that ensures this code path occurs at least once.
### Are there any user-facing changes?
No.
Before, the following code would crash the R session every time. After this PR, I cannot reproduce a crash:
```r
library(arrow, warn.conflicts = FALSE)
for(i in 1:100) {
open_dataset("~/Desktop/nyc-taxi/") |>
head()
}
```
See https://github.com/apache/arrow/issues/37513#issuecomment-1702756063 for how to get `nyc-taxi`.
--
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
[GitHub] [arrow] amoeba commented on pull request #37565: GH-37576: [R] Use `SafeCallIntoR()` to call garbage collector after a failed allocation
Posted by "amoeba (via GitHub)" <gi...@apache.org>.
amoeba commented on PR #37565:
URL: https://github.com/apache/arrow/pull/37565#issuecomment-1721883168
Hey @paleolimbot, this looks reasonable to me.
PS: And it got me thinking, are there any other `cpp11:package("foo")["bar"]` calls that need to get wrapped in `SafeCallIntoR`? I see a few that aren't wrapped but I didn't know quite how to evaluate whether they needed it or not.
--
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
[GitHub] [arrow] paleolimbot commented on pull request #37565: GH-37576: [R] Use `SafeCallIntoR()` to call garbage collector after a failed allocation
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #37565:
URL: https://github.com/apache/arrow/pull/37565#issuecomment-1706970904
Bummer! I created a new issue for this then since it needs fixing anyway.
--
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
[GitHub] [arrow] paleolimbot merged pull request #37565: GH-37576: [R] Use `SafeCallIntoR()` to call garbage collector after a failed allocation
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot merged PR #37565:
URL: https://github.com/apache/arrow/pull/37565
--
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
[GitHub] [arrow] amoeba commented on pull request #37565: GH-37576: [R] Use `SafeCallIntoR()` to call garbage collector after a failed allocation
Posted by "amoeba (via GitHub)" <gi...@apache.org>.
amoeba commented on PR #37565:
URL: https://github.com/apache/arrow/pull/37565#issuecomment-1718354995
Sure @paleolimbot. @thisisnic do you have a good reproducer for the crash?
--
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
[GitHub] [arrow] paleolimbot commented on pull request #37565: GH-37576: [R] Use `SafeCallIntoR()` to call garbage collector after a failed allocation
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #37565:
URL: https://github.com/apache/arrow/pull/37565#issuecomment-1712898461
@amoeba do you mind having a look at 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
[GitHub] [arrow] conbench-apache-arrow[bot] commented on pull request #37565: GH-37576: [R] Use `SafeCallIntoR()` to call garbage collector after a failed allocation
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #37565:
URL: https://github.com/apache/arrow/pull/37565#issuecomment-1724896250
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 440dc92caa73ca67c8ca98cebfb74f33788150bf.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/16914204913) has more details.
--
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
[GitHub] [arrow] paleolimbot commented on pull request #37565: GH-37576: [R] Use `SafeCallIntoR()` to call garbage collector after a failed allocation
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #37565:
URL: https://github.com/apache/arrow/pull/37565#issuecomment-1719994248
I don't think this PR actually solves the original issue (so I made a new issue since this is still a good change that may have caused crashes in other situations).
That said, a reprex to create the original crash is:
```
library(arrow)
library(dplyr)
# once...takes some time
open_dataset("s3://voltrondata-labs-datasets/nyc-taxi") |>
filter(year %in% 2012:2021) |>
write_dataset("nyc-taxi", partitioning = c("year", "month"))
# should cause the crash
for(i in 1:100){
open_dataset("~/data/nyc-taxi/") |>
head()
}
```
--
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
[GitHub] [arrow] amoeba commented on pull request #37565: GH-37576: [R] Use `SafeCallIntoR()` to call garbage collector after a failed allocation
Posted by "amoeba (via GitHub)" <gi...@apache.org>.
amoeba commented on PR #37565:
URL: https://github.com/apache/arrow/pull/37565#issuecomment-1720393818
@ursabot please benchmark
--
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
[GitHub] [arrow] amoeba commented on pull request #37565: GH-37576: [R] Use `SafeCallIntoR()` to call garbage collector after a failed allocation
Posted by "amoeba (via GitHub)" <gi...@apache.org>.
amoeba commented on PR #37565:
URL: https://github.com/apache/arrow/pull/37565#issuecomment-1724587511
Makes sense, thanks @paleolimbot .
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #37565: GH-37513: [R] Use `SafeCallIntoR()` to call garbage collector after a failed allocation
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #37565:
URL: https://github.com/apache/arrow/pull/37565#issuecomment-1706586566
:warning: GitHub issue #37513 **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
[GitHub] [arrow] thisisnic commented on pull request #37565: GH-37513: [R] Use `SafeCallIntoR()` to call garbage collector after a failed allocation
Posted by "thisisnic (via GitHub)" <gi...@apache.org>.
thisisnic commented on PR #37565:
URL: https://github.com/apache/arrow/pull/37565#issuecomment-1706948954
I'm afraid I'm still getting segfaults even on this branch:
```
Thread 24 "R" received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fff98ffb6c0 (LWP 13229)]
0x00007fffeee9c7aa in arrow::internal::Executor::Submit<parquet::arrow::(anonymous namespace)::FileReaderImpl::DecodeRowGroups(std::shared_ptr<parquet::arrow::(anonymous namespace)::FileReaderImpl>, const std::vector<int>&, const std::vector<int>&, arrow::internal::Executor*)::<lambda(size_t, std::shared_ptr<parquet::arrow::ColumnReaderImpl>)>&, long unsigned int&, std::shared_ptr<parquet::arrow::ColumnReaderImpl> >(arrow::internal::TaskHints, arrow::StopToken, struct {...} &) (this=0x555500000001, hints=..., stop_token=..., func=...)
at /home/nic/arrow/cpp/src/arrow/util/thread_pool.h:161
161 ARROW_RETURN_NOT_OK(SpawnReal(hints, std::move(task), std::move(stop_token),
```
--
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
[GitHub] [arrow] github-actions[bot] commented on pull request #37565: GH-37576: [R] Use `SafeCallIntoR()` to call garbage collector after a failed allocation
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #37565:
URL: https://github.com/apache/arrow/pull/37565#issuecomment-1706970255
:warning: GitHub issue #37576 **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
[GitHub] [arrow] paleolimbot commented on pull request #37565: GH-37576: [R] Use `SafeCallIntoR()` to call garbage collector after a failed allocation
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #37565:
URL: https://github.com/apache/arrow/pull/37565#issuecomment-1724385661
> PS: And it got me thinking, are there any other `cpp11:package("foo")["bar"]` calls that need to get wrapped in SafeCallIntoR? I see a few that aren't wrapped but I didn't know quite how to evaluate whether they needed it or not.
It's definitely confusing! It's not about the `cpp11:package("foo")["bar"]`, it's about the "calling the R API from another thread". You can't call *any* cpp11 code from another thread, but most of our cpp11 usage in the package is setting up an Arrow C++ call or wrapping the results (and these both happen on the main thread). Reading from R connections, executing user-defined functions, and running the garbage collector are special because Arrow is calling them very specifically from some other thread.
--
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
[GitHub] [arrow] ursabot commented on pull request #37565: GH-37576: [R] Use `SafeCallIntoR()` to call garbage collector after a failed allocation
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #37565:
URL: https://github.com/apache/arrow/pull/37565#issuecomment-1720393868
Benchmark runs are scheduled for commit 959f6a06480f180b748385d18ed1d929d82ef2eb. Watch https://buildkite.com/apache-arrow and https://conbench.ursa.dev for updates. A comment will be posted here when the runs are complete.
--
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
[GitHub] [arrow] conbench-apache-arrow[bot] commented on pull request #37565: GH-37576: [R] Use `SafeCallIntoR()` to call garbage collector after a failed allocation
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #37565:
URL: https://github.com/apache/arrow/pull/37565#issuecomment-1720697224
Thanks for your patience. Conbench analyzed the 6 benchmarking runs that have been run so far on PR commit 959f6a06480f180b748385d18ed1d929d82ef2eb.
There were 9 benchmark results indicating a performance regression:
- Pull Request Run on `arm64-m6g-linux-compute` at [2023-09-15 03:55:13Z](http://conbench.ursa.dev/compare/runs/a327ada130f34e3fafe27752a9e21034...2be96eff797e44a7907e9472a7295e08/)
- [`FloatStridedMatrixConversionFixture` (C++) with params=<Int64Type>/ConvertToSparseCSRMatrixInt64, source=cpp-micro, suite=arrow-tensor-conversion-benchmark](http://conbench.ursa.dev/compare/benchmarks/064f76d5f8f179a98000b2505ff6806d...06503d5d89cd7bb68000cd697e930afc)
- [`ConstructFromShortStdVector` (C++) with params=<STATIC_VECTOR(int)>, source=cpp-micro, suite=arrow-small-vector-benchmark](http://conbench.ursa.dev/compare/benchmarks/064f76d62e447c3480001b7c159d264a...06503d5e29e773108000f066cfdd8fd1)
- and 7 more (see the report linked below)
The [full Conbench report](https://github.com/apache/arrow/runs/16816999886) has more details.
--
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