You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "joemarshall (via GitHub)" <gi...@apache.org> on 2023/05/07 09:08:46 UTC
[GitHub] [arrow] joemarshall opened a new pull request, #35471: GH-35176: [C++] Arrow_threading
joemarshall opened a new pull request, #35471:
URL: https://github.com/apache/arrow/pull/35471
As previously discussed in #35176 this is a patch that adds an option `ARROW_DISABLE_THREADING`. When it is turned on, arrow threadpool and serial executors don't spawn threads, and instead run tasks in the main thread when futures are waited for.
It doesn't mess with threading in projects included as dependencies, e.g. multithreaded malloc implementations because if you're building for a non threaded environment, you can't use those anyway.
Basically where this is at is that it runs the test suite okay, and I think should work well enough to be a backend for pandas on emscripten/pyodide, so subject to reviews etc. it is worth merging (and would be jolly convenient for me if it was).
What this means is:
1) It is possible to use arrow in non-threaded emscripten/webassembly environments (with some build patches specific to emscripten which I'll put in once this is in)
2) Most of arrow just works, albeit slower in parts.
Things that don't work and probably won't:
1) Server stuff that relies on threads. Not a massive problem I think because environments with threading restrictions are currently typically also restricted from making servers anyway (i.e. they are web browsers)
2) Anything that relies on actually doing two things at once (for obvious reasons)
Things that don't work yet and could be fixed in future:
1) use of asynchronous file/network APIs in emscripten which would mean I/O could work efficiently in one thread.
2) asofjoin - right now the implementation relies on std::thread - it needs refactoring to work with threadpool like everything else in arrow, but I'm not sure I am expert enough in the codebase to do it well.
--
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] joemarshall commented on pull request #35471: GH-35176: [C++] Add support for disabling threading for emscripten
Posted by "joemarshall (via GitHub)" <gi...@apache.org>.
joemarshall commented on PR #35471:
URL: https://github.com/apache/arrow/pull/35471#issuecomment-1541594161
I think I've done the current review comments - so as i understand it I can run the test from tasks.yml by commenting in here like this?
@github-actions crossbow submit test-ubuntu-22.04-cpp-no-threading
--
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] kou commented on a diff in pull request #35471: GH-35176: [C++] Add support for disabling threading for emscripten
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #35471:
URL: https://github.com/apache/arrow/pull/35471#discussion_r1189255233
##########
.github/workflows/cpp.yml:
##########
@@ -57,11 +57,19 @@ jobs:
name: ${{ matrix.title }}
runs-on: ${{ matrix.runs-on }}
if: ${{ !contains(github.event.pull_request.title, 'WIP') }}
- timeout-minutes: 75
+ timeout-minutes: 120
strategy:
fail-fast: false
matrix:
include:
+ - arch: amd64
+ clang-tools: "14"
+ image: ubuntu-cpp
+ llvm: "14"
+ runs-on: ubuntu-latest
+ title: AMD64 ubuntu-cpp no threads
+ ubuntu: "22.04"
+ enable_threading: "OFF"
Review Comment:
The following change will work:
```diff
diff --git a/dev/tasks/tasks.yml b/dev/tasks/tasks.yml
index 6d07c65b4..103c11acc 100644
--- a/dev/tasks/tasks.yml
+++ b/dev/tasks/tasks.yml
@@ -1162,6 +1162,15 @@ tasks:
image: ubuntu-cpp
{% endfor %}
+ test-ubuntu-22.04-cpp-no-threading:
+ ci: github
+ template: docker-tests/github.linux.yml
+ params:
+ env:
+ UBUNTU: 22.04
+ flags: "-e ARROW_ENABLE_THREADING=OFF"
+ image: ubuntu-cpp
+
test-ubuntu-20.04-cpp-thread-sanitizer:
ci: github
template: docker-tests/github.linux.yml
```
You can run the job by commenting `@github-actions crossbow submit test-ubuntu-22.04-cpp-no-threading` in a 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
[GitHub] [arrow] pitrou commented on a diff in pull request #35471: GH-35176: [C++] Add support for disabling threading for emscripten
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #35471:
URL: https://github.com/apache/arrow/pull/35471#discussion_r1188852980
##########
cpp/src/arrow/acero/task_util_test.cc:
##########
@@ -176,11 +180,18 @@ TEST(TaskScheduler, Stress) {
// thread starts a task group while another thread is finishing
// the last of its tasks.
TEST(TaskScheduler, StressTwo) {
+
+ #ifndef ARROW_ENABLE_THREADING
+ GTEST_SKIP() << "Test requires threading support";
+ #endif
+
+
Review Comment:
Nit, but there's superfluous vertical blank space here.
--
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 #35471: GH-35176: [C++] Arrow_threading
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35471:
URL: https://github.com/apache/arrow/pull/35471#issuecomment-1537365508
:warning: GitHub issue #35176 **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] joemarshall commented on pull request #35471: GH-35176: [C++] Add support for disabling threading for emscripten
Posted by "joemarshall (via GitHub)" <gi...@apache.org>.
joemarshall commented on PR #35471:
URL: https://github.com/apache/arrow/pull/35471#issuecomment-1545452634
@kou A feature branch in my repo? Or does someone need to make a feature branch in the main repo which I can target the pull request against?
--
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] kou commented on pull request #35471: GH-35176: [C++] Add support for disabling threading for emscripten
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #35471:
URL: https://github.com/apache/arrow/pull/35471#issuecomment-1537571088
There are many default `ON` options. So `ARROW_ENABLE_*=ON` doesn't imply that the feature is disabled by default.
See also: https://github.com/apache/arrow/blob/main/cpp/cmake_modules/DefineOptions.cmake
--
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] pitrou commented on a diff in pull request #35471: GH-35176: [C++] Add support for disabling threading for emscripten
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #35471:
URL: https://github.com/apache/arrow/pull/35471#discussion_r1188846613
##########
cpp/cmake_modules/DefineOptions.cmake:
##########
@@ -210,6 +210,10 @@ takes precedence over ccache if a storage backend is configured" ON)
define_option(ARROW_WITH_MUSL "Whether the system libc is musl or not" OFF)
+ define_option(ARROW_ENABLE_THREADING "Enable threading in arrow core. "
+ ON)
Review Comment:
```suggestion
define_option(ARROW_ENABLE_THREADING "Enable threading in Arrow core. "
ON)
```
--
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 #35471: GH-35176: [C++] Add support for disabling threading for emscripten
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35471:
URL: https://github.com/apache/arrow/pull/35471#issuecomment-1541609336
```
Command '['git', '-C', '/tmp/tmplwp1r9ca/arrow', 'fetch', 'origin', 'pull/35471/head:main']' returned non-zero exit status 128.
The Archery job run can be found at: https://github.com/apache/arrow/actions/runs/4935043997
```
--
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] pitrou commented on a diff in pull request #35471: GH-35176: [C++] Add support for disabling threading for emscripten
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #35471:
URL: https://github.com/apache/arrow/pull/35471#discussion_r1188850473
##########
cpp/src/arrow/acero/bloom_filter_test.cc:
##########
@@ -472,8 +472,10 @@ TEST(BloomFilter, Basic) {
std::vector<BloomFilterBuildStrategy> strategy;
strategy.push_back(BloomFilterBuildStrategy::SINGLE_THREADED);
-#ifndef ARROW_VALGRIND
+#ifdef ARROW_ENABLE_THREADING
+#ifndef ARROW_VALGRIND
strategy.push_back(BloomFilterBuildStrategy::PARALLEL);
+#endif
#endif
Review Comment:
Or, more tersely
```suggestion
#if defined(ARROW_ENABLE_THREADING) && !defined(ARROW_VALGRIND)
strategy.push_back(BloomFilterBuildStrategy::PARALLEL);
#endif
```
--
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] pitrou commented on a diff in pull request #35471: GH-35176: [C++] Add support for disabling threading for emscripten
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #35471:
URL: https://github.com/apache/arrow/pull/35471#discussion_r1188852366
##########
cpp/src/arrow/acero/task_util.cc:
##########
@@ -328,10 +328,13 @@ Status TaskSchedulerImpl::ScheduleMore(size_t thread_id, int num_tasks_finished)
}
ARROW_DCHECK(register_finished_);
-
+#ifndef ARROW_ENABLE_THREADING
+ return ExecuteMore(thread_id, 1, true);
+#else
if (use_sync_execution_) {
Review Comment:
Shouldn't instead `use_sync_execution_` to be always true when threading is disabled? @westonpace
--
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 #35471: GH-35176: [C++] Arrow_threading
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35471:
URL: https://github.com/apache/arrow/pull/35471#issuecomment-1537365499
* Closes: #35176
--
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] joemarshall commented on pull request #35471: GH-35176: [C++] Add support for disabling threading for emscripten
Posted by "joemarshall (via GitHub)" <gi...@apache.org>.
joemarshall commented on PR #35471:
URL: https://github.com/apache/arrow/pull/35471#issuecomment-1540043819
That's flipped, it's now ARROW_ENABLE_THREADING, and an option is added to defineoptions.cmake
--
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] pitrou commented on a diff in pull request #35471: GH-35176: [C++] Add support for disabling threading for emscripten
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #35471:
URL: https://github.com/apache/arrow/pull/35471#discussion_r1188848035
##########
cpp/src/arrow/acero/asof_join_node.cc:
##########
@@ -1520,6 +1520,9 @@ class AsofJoinNode : public ExecNode {
// Plan has already aborted. Do not start process thread
return Status::OK();
}
+ #ifndef ARROW_ENABLE_THREADING
+ return Status::ExecutionError("ASOF join requires threading enabled");
Review Comment:
Probably move this at the beginning of this method? Also use a `#else` clause around the regular implementation.
And I would probably make this `Status::NotImplemented`.
--
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] pitrou commented on a diff in pull request #35471: GH-35176: [C++] Add support for disabling threading for emscripten
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #35471:
URL: https://github.com/apache/arrow/pull/35471#discussion_r1188848610
##########
cpp/src/arrow/acero/asof_join_node_test.cc:
##########
@@ -1380,7 +1380,7 @@ T GetEnvValue(const std::string& var, T default_value) {
} // namespace
TEST(AsofJoinTest, BackpressureWithBatchesGen) {
- int num_batches = GetEnvValue("ARROW_BACKPRESSURE_DEMO_NUM_BATCHES", 10);
+ int num_batches = GetEnvValue("ARROW_BACKPRESSURE_DEMO_NUM_BATCHES", 2);
Review Comment:
Not sure we want to modify this parameter? cc @westonpace
--
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] kou commented on pull request #35471: GH-35176: [C++] Add support for disabling threading for emscripten
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #35471:
URL: https://github.com/apache/arrow/pull/35471#issuecomment-1542651503
@joemarshall Could you create a feature branch for this instead of using the `main` branch?
`@github-actions crossbow submit test-ubuntu-22.04-cpp-no-threading` doesn't work with the `main` branch.
--
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] joemarshall commented on pull request #35471: GH-35176: [C++] Add support for disabling threading for emscripten
Posted by "joemarshall (via GitHub)" <gi...@apache.org>.
joemarshall commented on PR #35471:
URL: https://github.com/apache/arrow/pull/35471#issuecomment-1539102960
Okay, I'll flip that
--
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] pitrou commented on a diff in pull request #35471: GH-35176: [C++] Add support for disabling threading for emscripten
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #35471:
URL: https://github.com/apache/arrow/pull/35471#discussion_r1188849548
##########
cpp/src/arrow/acero/bloom_filter.cc:
##########
@@ -426,6 +426,9 @@ void BloomFilterBuilder_Parallel::CleanUp() {
std::unique_ptr<BloomFilterBuilder> BloomFilterBuilder::Make(
BloomFilterBuildStrategy strategy) {
+#ifndef ARROW_ENABLE_THREADING
+ strategy=BloomFilterBuildStrategy::SINGLE_THREADED;
Review Comment:
```suggestion
strategy = BloomFilterBuildStrategy::SINGLE_THREADED;
```
--
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 #35471: GH-35176: [C++] Add support for disabling threading for emscripten
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35471:
URL: https://github.com/apache/arrow/pull/35471#issuecomment-1541598857
```
Only contributors can submit requests to this bot. Please ask someone from the community for help with getting the first commit in.
The Archery job run can be found at: https://github.com/apache/arrow/actions/runs/4934990365
```
--
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] joemarshall commented on pull request #35471: GH-35176: [C++] Add support for disabling threading for emscripten
Posted by "joemarshall (via GitHub)" <gi...@apache.org>.
joemarshall commented on PR #35471:
URL: https://github.com/apache/arrow/pull/35471#issuecomment-1537552937
Yeah I was torn on that. I guess I thought disable implies that threading is a default and that this is something for special cases? I'm easy though.
--
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] pitrou commented on a diff in pull request #35471: GH-35176: [C++] Add support for disabling threading for emscripten
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #35471:
URL: https://github.com/apache/arrow/pull/35471#discussion_r1188846963
##########
cpp/src/arrow/acero/CMakeLists.txt:
##########
@@ -236,7 +246,9 @@ if(ARROW_BUILD_BENCHMARKS)
target_link_libraries(arrow-acero-expression-benchmark PUBLIC arrow_acero_static)
target_link_libraries(arrow-acero-filter-benchmark PUBLIC arrow_acero_static)
target_link_libraries(arrow-acero-project-benchmark PUBLIC arrow_acero_static)
- target_link_libraries(arrow-acero-asof-join-benchmark PUBLIC arrow_acero_static)
+ if(!gig)
Review Comment:
Hmm, what is 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] pitrou commented on a diff in pull request #35471: GH-35176: [C++] Add support for disabling threading for emscripten
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #35471:
URL: https://github.com/apache/arrow/pull/35471#discussion_r1188841937
##########
.github/workflows/cpp.yml:
##########
@@ -57,11 +57,19 @@ jobs:
name: ${{ matrix.title }}
runs-on: ${{ matrix.runs-on }}
if: ${{ !contains(github.event.pull_request.title, 'WIP') }}
- timeout-minutes: 75
+ timeout-minutes: 120
strategy:
fail-fast: false
matrix:
include:
+ - arch: amd64
+ clang-tools: "14"
+ image: ubuntu-cpp
+ llvm: "14"
+ runs-on: ubuntu-latest
+ title: AMD64 ubuntu-cpp no threads
+ ubuntu: "22.04"
+ enable_threading: "OFF"
Review Comment:
I'm not convinced this should be part of the PR-based CI jobs. We can instead define a nightly build, which will stress our CI resources less. Perhaps @kou can help here?
--
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] pitrou commented on a diff in pull request #35471: GH-35176: [C++] Add support for disabling threading for emscripten
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #35471:
URL: https://github.com/apache/arrow/pull/35471#discussion_r1188842371
##########
cmdline-working.txt:
##########
@@ -0,0 +1,10 @@
+export CMAKE_TOOLCHAIN_FILE=/home/pszjm2/arrow/Emscripten.cmake
Review Comment:
This is probably not meant to be part of this PR, is it?
--
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] joemarshall commented on a diff in pull request #35471: GH-35176: [C++] Add support for disabling threading for emscripten
Posted by "joemarshall (via GitHub)" <gi...@apache.org>.
joemarshall commented on code in PR #35471:
URL: https://github.com/apache/arrow/pull/35471#discussion_r1189379104
##########
cpp/src/arrow/acero/asof_join_node_test.cc:
##########
@@ -1380,7 +1380,7 @@ T GetEnvValue(const std::string& var, T default_value) {
} // namespace
TEST(AsofJoinTest, BackpressureWithBatchesGen) {
- int num_batches = GetEnvValue("ARROW_BACKPRESSURE_DEMO_NUM_BATCHES", 10);
+ int num_batches = GetEnvValue("ARROW_BACKPRESSURE_DEMO_NUM_BATCHES", 2);
Review Comment:
oops, sorry left in from testing. Reverting.
--
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] assignUser commented on a diff in pull request #35471: GH-35176: [C++] Add support for disabling threading for emscripten
Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser commented on code in PR #35471:
URL: https://github.com/apache/arrow/pull/35471#discussion_r1189282604
##########
.github/workflows/cpp.yml:
##########
@@ -57,11 +57,19 @@ jobs:
name: ${{ matrix.title }}
runs-on: ${{ matrix.runs-on }}
if: ${{ !contains(github.event.pull_request.title, 'WIP') }}
- timeout-minutes: 75
+ timeout-minutes: 120
strategy:
fail-fast: false
matrix:
include:
+ - arch: amd64
+ clang-tools: "14"
+ image: ubuntu-cpp
+ llvm: "14"
+ runs-on: ubuntu-latest
+ title: AMD64 ubuntu-cpp no threads
+ ubuntu: "22.04"
+ enable_threading: "OFF"
Review Comment:
Agreed! A nightly would be much more appropriate.
--
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] joemarshall commented on a diff in pull request #35471: GH-35176: [C++] Add support for disabling threading for emscripten
Posted by "joemarshall (via GitHub)" <gi...@apache.org>.
joemarshall commented on code in PR #35471:
URL: https://github.com/apache/arrow/pull/35471#discussion_r1189391417
##########
.github/workflows/cpp.yml:
##########
@@ -57,11 +57,19 @@ jobs:
name: ${{ matrix.title }}
runs-on: ${{ matrix.runs-on }}
if: ${{ !contains(github.event.pull_request.title, 'WIP') }}
- timeout-minutes: 75
+ timeout-minutes: 120
strategy:
fail-fast: false
matrix:
include:
+ - arch: amd64
+ clang-tools: "14"
+ image: ubuntu-cpp
+ llvm: "14"
+ runs-on: ubuntu-latest
+ title: AMD64 ubuntu-cpp no threads
+ ubuntu: "22.04"
+ enable_threading: "OFF"
Review Comment:
okay, reverted and added nightly
--
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] joemarshall commented on a diff in pull request #35471: GH-35176: [C++] Add support for disabling threading for emscripten
Posted by "joemarshall (via GitHub)" <gi...@apache.org>.
joemarshall commented on code in PR #35471:
URL: https://github.com/apache/arrow/pull/35471#discussion_r1189391877
##########
cpp/src/arrow/acero/task_util.cc:
##########
@@ -328,10 +328,13 @@ Status TaskSchedulerImpl::ScheduleMore(size_t thread_id, int num_tasks_finished)
}
ARROW_DCHECK(register_finished_);
-
+#ifndef ARROW_ENABLE_THREADING
+ return ExecuteMore(thread_id, 1, true);
+#else
if (use_sync_execution_) {
Review Comment:
have done
--
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] kou commented on pull request #35471: GH-35176: [C++] Add support for disabling threading for emscripten
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #35471:
URL: https://github.com/apache/arrow/pull/35471#issuecomment-1545843867
> A feature branch in my repo?
Yes.
--
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] joemarshall commented on pull request #35471: GH-35176: [C++] Add support for disabling threading for emscripten
Posted by "joemarshall (via GitHub)" <gi...@apache.org>.
joemarshall commented on PR #35471:
URL: https://github.com/apache/arrow/pull/35471#issuecomment-1541598042
@github-actions crossbow submit test-ubuntu-22.04-cpp-no-threading
--
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] pitrou commented on pull request #35471: GH-35176: [C++] Add support for disabling threading for emscripten
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #35471:
URL: https://github.com/apache/arrow/pull/35471#issuecomment-1540503095
Hmm, I've started reviewing this PR but it adds a lot of code in various delicate internals (Future, ThreadPool, TaskGroup...) that makes me uneasy. Do we value emscripten/webassembly compat so much that it warrants the cost in maintainability? Would there be another way to support emscripten/webassembly?
--
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] pitrou commented on a diff in pull request #35471: GH-35176: [C++] Add support for disabling threading for emscripten
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #35471:
URL: https://github.com/apache/arrow/pull/35471#discussion_r1188854447
##########
cpp/src/arrow/util/config.h.cmake:
##########
@@ -57,5 +57,6 @@
#cmakedefine ARROW_WITH_MUSL
#cmakedefine ARROW_WITH_OPENTELEMETRY
#cmakedefine ARROW_WITH_UCX
+#cmakedefine ARROW_ENABLE_THREADING
Review Comment:
Let's try to keep this alphabetically-ordered?
--
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] pitrou commented on pull request #35471: GH-35176: [C++] Add support for disabling threading for emscripten
Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on PR #35471:
URL: https://github.com/apache/arrow/pull/35471#issuecomment-1541607355
@github-actions crossbow submit test-ubuntu-22.04-cpp-no-threading
--
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] kou commented on pull request #35471: GH-35176: [C++] Add support for disabling threading for emscripten
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on PR #35471:
URL: https://github.com/apache/arrow/pull/35471#issuecomment-1537536360
In general, I think that `ARROW_ENABLE_*` is better than `ARROW_DISABLE_*` for readability.
--
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] joemarshall commented on pull request #35471: GH-35176: [C++] Add support for disabling threading for emscripten
Posted by "joemarshall (via GitHub)" <gi...@apache.org>.
joemarshall commented on PR #35471:
URL: https://github.com/apache/arrow/pull/35471#issuecomment-1560049525
This is now in #35672 which is the same but from a differently named branch because that means that the CI stuff (crossbow submit commands) actually work, whereas they don't work if the name of the source branch is main.
--
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] joemarshall closed pull request #35471: GH-35176: [C++] Add support for disabling threading for emscripten
Posted by "joemarshall (via GitHub)" <gi...@apache.org>.
joemarshall closed pull request #35471: GH-35176: [C++] Add support for disabling threading for emscripten
URL: https://github.com/apache/arrow/pull/35471
--
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