westonpace commented on a change in pull request #8680:
URL: https://github.com/apache/arrow/pull/8680#discussion_r545718970
##########
File path: cpp/src/arrow/util/thread_pool_benchmark.cc
##########
@@ -136,21 +168,24 @@ static void ThreadedTaskGroup(benchmark::State& state) {
for (auto _ : state) {
auto task_group = TaskGroup::MakeThreaded(pool.get());
- for (int32_t i = 0; i < nspawns; ++i) {
- // Pass the task by reference to avoid copying it around
- task_group->Append(std::ref(task));
- }
+ task_group->Append([&task, nspawns, task_group] {
Review comment:
I spoke with @bkietz a bit more on this and he pointed out it is not a
race condition as written without the futures. The threaded task group cannot
actually finish unless `Finish` is called. Once futures are added in then it
becomes an issue. As soon as the task counter hits 0 it will complete the
future.
I spoke about whether we should pull this nesting back out and it was
recommended to just leave it all alone since futures will replace task group
someday.
----------------------------------------------------------------
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:
[email protected]
