You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2020/09/22 13:29:57 UTC
[GitHub] [arrow] pitrou opened a new pull request #8240: ARROW-10038: [C++] Spawn thread pool threads lazily
pitrou opened a new pull request #8240:
URL: https://github.com/apache/arrow/pull/8240
Thread pool threads are not spawned until necessary to execute a pending task.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #8240: ARROW-10038: [C++] Spawn thread pool threads lazily
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #8240:
URL: https://github.com/apache/arrow/pull/8240#issuecomment-705127783
So, there is still a crash with RTools 3.5 in Release builds:
https://github.com/apache/arrow/pull/8240/checks?check_run_id=1221808315
but not in RelWithDebugInfo builds:
https://github.com/apache/arrow/runs/1221597248
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on pull request #8240: ARROW-10038: [C++] Spawn thread pool threads lazily
Posted by GitBox <gi...@apache.org>.
bkietz commented on pull request #8240:
URL: https://github.com/apache/arrow/pull/8240#issuecomment-697387129
@pitrou looks like an ordering-of-dict-fields problem in test_parquet_nested_storage https://github.com/apache/arrow/pull/8240/checks?check_run_id=1154644456#step:8:2885
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace edited a comment on pull request #8240: ARROW-10038: [C++] Spawn thread pool threads lazily
Posted by GitBox <gi...@apache.org>.
westonpace edited a comment on pull request #8240:
URL: https://github.com/apache/arrow/pull/8240#issuecomment-765025427
Sorry, I am having trouble understanding what you are referencing. There was an RTools 3.5 failure because of a gcc bug requiring you to std::move a unique_ptr but I don't think that would apply here.
The other compiler warning treated as error seemed to be unique to gcc 9.3 and tsan and I didn't know there was any failing job related to 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #8240: ARROW-10038: [C++] Spawn thread pool threads lazily
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #8240:
URL: https://github.com/apache/arrow/pull/8240#issuecomment-765553385
@westonpace Sorry, I wasn't clear. I was just cc'ing you for the PR itself, not for the CI failure.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8240: ARROW-10038: [C++] Spawn thread pool threads lazily
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8240:
URL: https://github.com/apache/arrow/pull/8240#issuecomment-696730535
https://issues.apache.org/jira/browse/ARROW-10038
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on pull request #8240: ARROW-10038: [C++] Spawn thread pool threads lazily
Posted by GitBox <gi...@apache.org>.
bkietz commented on pull request #8240:
URL: https://github.com/apache/arrow/pull/8240#issuecomment-697424423
https://github.com/apache/arrow/pull/8248
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #8240: ARROW-10038: [C++] Spawn thread pool threads lazily
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #8240:
URL: https://github.com/apache/arrow/pull/8240#issuecomment-765025427
Sorry, I having trouble understanding what you are referencing. There was an RTools 3.5 failure because of a gcc bug requiring you to std::move a unique_ptr but I don't think that would apply here.
The other compiler warning treated as error seemed to be unique to gcc 9.3 and tsan and I didn't know there was any failing job related to 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #8240: ARROW-10038: [C++] Spawn thread pool threads lazily
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #8240:
URL: https://github.com/apache/arrow/pull/8240#discussion_r492829401
##########
File path: cpp/src/arrow/util/thread_pool.cc
##########
@@ -168,9 +174,11 @@ Status ThreadPool::SetCapacity(int threads) {
CollectFinishedWorkersUnlocked();
state_->desired_capacity_ = threads;
- int diff = static_cast<int>(threads - state_->workers_.size());
- if (diff > 0) {
- LaunchWorkersUnlocked(diff);
+ const int diff = threads - static_cast<int>(state_->workers_.size());
Review comment:
Maybe easier to read?
```suggestion
const int required = static_cast<int>(std::min(
threads - state_->workers_.size(),
state_->pending_tasks_.size()));
```
##########
File path: cpp/src/arrow/util/thread_pool.h
##########
@@ -182,8 +182,12 @@ class ARROW_EXPORT ThreadPool : public Executor {
int GetCapacity() override;
// Dynamically change the number of worker threads.
- // This function returns quickly, but it may take more time before the
- // thread count is fully adjusted.
+ //
+ // This function always returns immediately.
+ // If less threads are running than this number, new threads are spawned
Review comment:
```suggestion
// If fewer threads are running than this number, new threads are spawned
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #8240: ARROW-10038: [C++] Spawn thread pool threads lazily
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #8240:
URL: https://github.com/apache/arrow/pull/8240#issuecomment-764950406
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #8240: ARROW-10038: [C++] Spawn thread pool threads lazily
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #8240:
URL: https://github.com/apache/arrow/pull/8240#issuecomment-767742154
Okay, I'm gonna merge this since the RTools 3.5 CI failures haven't reappeared. Other CI failures are unrelated.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou closed pull request #8240: ARROW-10038: [C++] Spawn thread pool threads lazily
Posted by GitBox <gi...@apache.org>.
pitrou closed pull request #8240:
URL: https://github.com/apache/arrow/pull/8240
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] bkietz commented on a change in pull request #8240: ARROW-10038: [C++] Spawn thread pool threads lazily
Posted by GitBox <gi...@apache.org>.
bkietz commented on a change in pull request #8240:
URL: https://github.com/apache/arrow/pull/8240#discussion_r492829401
##########
File path: cpp/src/arrow/util/thread_pool.cc
##########
@@ -168,9 +174,11 @@ Status ThreadPool::SetCapacity(int threads) {
CollectFinishedWorkersUnlocked();
state_->desired_capacity_ = threads;
- int diff = static_cast<int>(threads - state_->workers_.size());
- if (diff > 0) {
- LaunchWorkersUnlocked(diff);
+ const int diff = threads - static_cast<int>(state_->workers_.size());
Review comment:
Maybe easier to read?
```suggestion
const int required = static_cast<int>(std::min(
threads - state_->workers_.size(),
state_->pending_tasks_.size()));
```
##########
File path: cpp/src/arrow/util/thread_pool.h
##########
@@ -182,8 +182,12 @@ class ARROW_EXPORT ThreadPool : public Executor {
int GetCapacity() override;
// Dynamically change the number of worker threads.
- // This function returns quickly, but it may take more time before the
- // thread count is fully adjusted.
+ //
+ // This function always returns immediately.
+ // If less threads are running than this number, new threads are spawned
Review comment:
```suggestion
// If fewer threads are running than this number, new threads are spawned
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #8240: ARROW-10038: [C++] Spawn thread pool threads lazily
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #8240:
URL: https://github.com/apache/arrow/pull/8240#issuecomment-765025427
Sorry, I having trouble understanding what you are referencing. There was an RTools 3.5 failure because of a gcc bug requiring you to std::move a unique_ptr but I don't think that would apply here.
The other compiler warning treated as error seemed to be unique to gcc 9.3 and tsan and I didn't know there was any failing job related to 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #8240: ARROW-10038: [C++] Spawn thread pool threads lazily
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #8240:
URL: https://github.com/apache/arrow/pull/8240#issuecomment-764950406
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #8240: ARROW-10038: [C++] Spawn thread pool threads lazily
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8240:
URL: https://github.com/apache/arrow/pull/8240#issuecomment-696730535
https://issues.apache.org/jira/browse/ARROW-10038
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace edited a comment on pull request #8240: ARROW-10038: [C++] Spawn thread pool threads lazily
Posted by GitBox <gi...@apache.org>.
westonpace edited a comment on pull request #8240:
URL: https://github.com/apache/arrow/pull/8240#issuecomment-765025427
Sorry, I am having trouble understanding what you are referencing. There was an RTools 3.5 failure because of a gcc bug requiring you to std::move a unique_ptr but I don't think that would apply here.
The other compiler warning treated as error seemed to be unique to gcc 9.3 and tsan and I didn't know there was any failing job related to 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] westonpace commented on pull request #8240: ARROW-10038: [C++] Spawn thread pool threads lazily
Posted by GitBox <gi...@apache.org>.
westonpace commented on pull request #8240:
URL: https://github.com/apache/arrow/pull/8240#issuecomment-765628471
Ah, no problem. Looks good. We will have to keep this need in mind if we ever end up adopting the multi-queue approach.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #8240: ARROW-10038: [C++] Spawn thread pool threads lazily
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #8240:
URL: https://github.com/apache/arrow/pull/8240#issuecomment-697401323
@bkietz Yes, this probably needs a quick fixup 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org