[lldb/Host] Remove TaskPool and replace its uses with llvm::ThreadPool

[JDevlieghere]

Summary

No need to roll our own implementation for something that's covered by llvm.

[labath]

While getting rid of this would be great, this (the manual dwarf index) is also very performance sensitive code which has been optimized and re-optimized a number of times. So, I would want to see a benchmark to ensure this does not affect performance adversely. A good benchmark would be to take a release (without asserts) build of lldb and debug build of clang (without accelerator tables) and then run something like time opt/lldb dbg/clang -o "br set -n nonexisting_function" -b a couple of times. The test should be run on an elf binary because the mac debug info model is sufficiently different to skew the results, even if you do manage to disable the accelerator tables.

lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp

24–30

This *is* simpler than the previous implementation, but it may have different performance characteristics. The previous implementation was optimized for small workloads where the overhead of enqueuing a task was not negligible compared to the size of the job itself.

86–112

All of this should use the same ThreadPool instance to avoid re-creating the pool threads a couple of times.

[kwk]

All tests pass. I first thought that the lldb/test/Shell/Reproducer/TestGDBRemoteRepro.test test didn't work but it seems to be flaky.

lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp

86–88

@JDevlieghere I assume you've removed the first of the three parameters to TaskMapOverInt because it was always 0 anyways? If not, shouldn't this be easily changeable?

static void TaskMapOverInt(size_t idx, size_t end,
                           const llvm::function_ref<void(size_t)> &func) {
  llvm::ThreadPool pool;
  for (; idx < end; idx++)
    pool.async(func, idx);
  pool.wait();
}

[kwk]

I resign because I think @labath made some good points that I cannot argue about.

[labath]

lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp

24–30

(I'm not saying this should be changed back. llvm's thread pool creates threads differently, and that is likely to have been the main cause of overhead, so this optimization may not matter anymore. However, it would be good to verify that.)

[JDevlieghere]

Below are the results for the situation you suggested: release (no-asserts) lldb, debug clang (no debug names). Results were collected in a Ubuntu 18.04 docker image running on CentOS.

$ for i in {1..10}; do  time ./build-ra/bin/lldb ./build-debug/bin/clang -o "br set -n nonexisting_function" -b > /dev/null ;done

-----------------
before
-----------------
real    0m11.613s
real    0m11.527s
real    0m11.547s
real    0m11.677s
real    0m11.614s
real    0m11.627s
real    0m11.882s
real    0m11.468s
real    0m11.682s
real    0m11.562s
-----------------
avg     0m11.620s

-----------------
after
-----------------
real    0m11.887s
real    0m11.614s
real    0m11.812s
real    0m11.723s
real    0m11.605s
real    0m12.080s
real    0m11.673s
real    0m11.751s
real    0m11.660s
real    0m11.647s
-----------------
avg     0m11.745s

[labath]

Thanks. The observed difference appears to be (borderline) statistically significant, but the difference (~1%) is not very big, so I think that's worth reusing the llvm class (and any future performance improvements). I think the difference will go down even more if you avoid recreating the thread pool three times.

[JDevlieghere]

Share the thread pool instead of recreating it.

[labath]

Cool. I'm not sure how useful is the TaskMapOverInt function in this new world (if I was writing this from scratch, I don't think I would create a function for that), but it doesn't hurt that much either.

lldb/include/lldb/Host/TaskPool.h

-//===--------------------- TaskPool.h ---------------------------*- C++ -*-===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-#ifndef LLDB_HOST_TASKPOOL_H
-#define LLDB_HOST_TASKPOOL_H
-
-#include "llvm/ADT/STLExtras.h"
-#include <functional>
-#include <future>
-#include <list>
-#include <memory>
-#include <mutex>
-#include <type_traits>
-
-namespace lldb_private {
-
-// Global TaskPool class for running tasks in parallel on a set of worker
-// thread created the first time the task pool is used. The TaskPool provide no
-// guarantee about the order the task will be run and about what tasks will run
-// in parallel. None of the task added to the task pool should block on
-// something (mutex, future, condition variable) what will be set only by the
-// completion of an other task on the task pool as they may run on the same
-// thread sequentally.
-class TaskPool {
-public:
-  // Add a new task to the task pool and return a std::future belonging to the
-  // newly created task. The caller of this function has to wait on the future
-  // for this task to complete.
-  template <typename F, typename... Args>
-  static std::future<typename std::result_of<F(Args...)>::type>
-  AddTask(F &&f, Args &&... args);
-
-  // Run all of the specified tasks on the task pool and wait until all of them
-  // are finished before returning. This method is intended to be used for
-  // small number tasks where listing them as function arguments is acceptable.
-  // For running large number of tasks you should use AddTask for each task and
-  // then call wait() on each returned future.
-  template <typename... T> static void RunTasks(T &&... tasks);
-
-private:
-  TaskPool() = delete;
-
-  template <typename... T> struct RunTaskImpl;
-
-  static void AddTaskImpl(std::function<void()> &&task_fn);
-};
-
-template <typename F, typename... Args>
-std::future<typename std::result_of<F(Args...)>::type>
-TaskPool::AddTask(F &&f, Args &&... args) {
-  auto task_sp = std::make_shared<
-      std::packaged_task<typename std::result_of<F(Args...)>::type()>>(
-      std::bind(std::forward<F>(f), std::forward<Args>(args)...));
-
-  AddTaskImpl([task_sp]() { (*task_sp)(); });
-
-  return task_sp->get_future();
-}
-
-template <typename... T> void TaskPool::RunTasks(T &&... tasks) {
-  RunTaskImpl<T...>::Run(std::forward<T>(tasks)...);
-}
-
-template <typename Head, typename... Tail>
-struct TaskPool::RunTaskImpl<Head, Tail...> {
-  static void Run(Head &&h, Tail &&... t) {
-    auto f = AddTask(std::forward<Head>(h));
-    RunTaskImpl<Tail...>::Run(std::forward<Tail>(t)...);
-    f.wait();
-  }
-};
-
-template <> struct TaskPool::RunTaskImpl<> {
-  static void Run() {}
-};
-
-// Run 'func' on every value from begin .. end-1.  Each worker will grab
-// 'batch_size' numbers at a time to work on, so for very fast functions, batch
-// should be large enough to avoid too much cache line contention.
-void TaskMapOverInt(size_t begin, size_t end,
-                    const llvm::function_ref<void(size_t)> &func);
-
-unsigned GetHardwareConcurrencyHint();
-
-} // namespace lldb_private
-
-#endif // LLDB_HOST_TASKPOOL_H

lldb/include/lldb/module.modulemap

module lldb_Host {
   module ProcessLauncher { header "Host/ProcessLauncher.h" export * }
   module ProcessLaunchInfo { header "Host/ProcessLaunchInfo.h" export * }
   module ProcessRunLock { header "Host/ProcessRunLock.h" export * }
   module PseudoTerminal { header "Host/PseudoTerminal.h" export * }
   module SafeMachO { header "Host/SafeMachO.h" export * }
   module SocketAddress { header "Host/SocketAddress.h" export * }
   module Socket { header "Host/Socket.h" export * }
   module StringConvert { textual header "Host/StringConvert.h" export * }
-  module TaskPool { header "Host/TaskPool.h" export * }
   module Terminal { header "Host/Terminal.h" export * }
   module ThreadLauncher { header "Host/ThreadLauncher.h" export * }
   module Time { header "Host/Time.h" export * }
   module XML { header "Host/XML.h" export * }
 
   module common {
     umbrella "Host/common"
     module * { export * }

lldb/source/Host/CMakeLists.txt

add_host_subdirectory(common
   common/OptionParser.cpp
   common/PipeBase.cpp
   common/ProcessLaunchInfo.cpp
   common/ProcessRunLock.cpp
   common/PseudoTerminal.cpp
   common/SocketAddress.cpp
   common/Socket.cpp
   common/StringConvert.cpp
-  common/TaskPool.cpp
   common/TCPSocket.cpp
   common/Terminal.cpp
   common/ThreadLauncher.cpp
   common/UDPSocket.cpp
   common/XML.cpp
   )
 
 if (LLDB_ENABLE_LIBEDIT)

lldb/source/Host/common/TaskPool.cpp

-//===-- TaskPool.cpp ------------------------------------------------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-
-#include "lldb/Host/TaskPool.h"
-#include "lldb/Host/ThreadLauncher.h"
-#include "lldb/Utility/Log.h"
-
-#include <cstdint>
-#include <queue>
-#include <thread>
-
-namespace lldb_private {
-
-namespace {
-class TaskPoolImpl {
-public:
-  static TaskPoolImpl &GetInstance();
-
-  void AddTask(std::function<void()> &&task_fn);
-
-private:
-  TaskPoolImpl();
-
-  static lldb::thread_result_t WorkerPtr(void *pool);
-
-  static void Worker(TaskPoolImpl *pool);
-
-  std::queue<std::function<void()>> m_tasks;
-  std::mutex m_tasks_mutex;
-  uint32_t m_thread_count;
-};
-
-} // end of anonymous namespace
-
-TaskPoolImpl &TaskPoolImpl::GetInstance() {
-  static TaskPoolImpl g_task_pool_impl;
-  return g_task_pool_impl;
-}
-
-void TaskPool::AddTaskImpl(std::function<void()> &&task_fn) {
-  TaskPoolImpl::GetInstance().AddTask(std::move(task_fn));
-}
-
-TaskPoolImpl::TaskPoolImpl() : m_thread_count(0) {}
-
-unsigned GetHardwareConcurrencyHint() {
-  // std::thread::hardware_concurrency may return 0 if the value is not well
-  // defined or not computable.
-  static const unsigned g_hardware_concurrency = 
-    std::max(1u, std::thread::hardware_concurrency());
-  return g_hardware_concurrency;
-}
-
-void TaskPoolImpl::AddTask(std::function<void()> &&task_fn) {
-  const size_t min_stack_size = 8 * 1024 * 1024;
-
-  std::unique_lock<std::mutex> lock(m_tasks_mutex);
-  m_tasks.emplace(std::move(task_fn));
-  if (m_thread_count < GetHardwareConcurrencyHint()) {
-    m_thread_count++;
-    // Note that this detach call needs to happen with the m_tasks_mutex held.
-    // This prevents the thread from exiting prematurely and triggering a linux
-    // libc bug (https://sourceware.org/bugzilla/show_bug.cgi?id=19951).
-    llvm::Expected<HostThread> host_thread =
-        lldb_private::ThreadLauncher::LaunchThread(
-            "task-pool.worker", WorkerPtr, this, min_stack_size);
-    if (host_thread) {
-      host_thread->Release();
-    } else {
-      LLDB_LOG(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_HOST),
-               "failed to launch host thread: {}",
-               llvm::toString(host_thread.takeError()));
-    }
-  }
-}
-
-lldb::thread_result_t TaskPoolImpl::WorkerPtr(void *pool) {
-  Worker((TaskPoolImpl *)pool);
-  return {};
-}
-
-void TaskPoolImpl::Worker(TaskPoolImpl *pool) {
-  while (true) {
-    std::unique_lock<std::mutex> lock(pool->m_tasks_mutex);
-    if (pool->m_tasks.empty()) {
-      pool->m_thread_count--;
-      break;
-    }
-
-    std::function<void()> f = std::move(pool->m_tasks.front());
-    pool->m_tasks.pop();
-    lock.unlock();
-
-    f();
-  }
-}
-
-void TaskMapOverInt(size_t begin, size_t end,
-                    const llvm::function_ref<void(size_t)> &func) {
-  const size_t num_workers = std::min<size_t>(end, GetHardwareConcurrencyHint());
-  std::atomic<size_t> idx{begin};
-  
-  auto wrapper = [&idx, end, &func]() {
-    while (true) {
-      size_t i = idx.fetch_add(1);
-      if (i >= end)
-        break;
-      func(i);
-    }
-  };
-
-  std::vector<std::future<void>> futures;
-  futures.reserve(num_workers);
-  for (size_t i = 0; i < num_workers; i++)
-    futures.push_back(TaskPool::AddTask(wrapper));
-  for (size_t i = 0; i < num_workers; i++)
-    futures[i].wait();
-}
-
-} // namespace lldb_private
-

lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp

 //===-- ManualDWARFIndex.cpp ----------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Plugins/SymbolFile/DWARF/ManualDWARFIndex.h"
 #include "Plugins/Language/ObjC/ObjCLanguage.h"
 #include "Plugins/SymbolFile/DWARF/DWARFDebugInfo.h"
 #include "Plugins/SymbolFile/DWARF/DWARFDeclContext.h"
 #include "Plugins/SymbolFile/DWARF/LogChannelDWARF.h"
 #include "Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h"
 #include "lldb/Core/Module.h"
-#include "lldb/Host/TaskPool.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/Utility/Timer.h"
+#include "llvm/Support/ThreadPool.h"
 
 using namespace lldb_private;
 using namespace lldb;
 
 void ManualDWARFIndex::Index() {
   if (!m_dwarf)
     return;
 
   SymbolFileDWARF &main_dwarf = *m_dwarf;
   m_dwarf = nullptr;
 
   static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
   Timer scoped_timer(func_cat, "%p", static_cast<void *>(&main_dwarf));
 
   DWARFDebugInfo &main_info = main_dwarf.DebugInfo();
   SymbolFileDWARFDwo *dwp_dwarf = main_dwarf.GetDwpSymbolFile().get();
   DWARFDebugInfo *dwp_info = dwp_dwarf ? &dwp_dwarf->DebugInfo() : nullptr;
 
   std::vector<DWARFUnit *> units_to_index;
void ManualDWARFIndex::Index() {
   auto parser_fn = [&](size_t cu_idx) {
     IndexUnit(*units_to_index[cu_idx], dwp_dwarf, sets[cu_idx]);
   };
 
   auto extract_fn = [&units_to_index, &clear_cu_dies](size_t cu_idx) {
     clear_cu_dies[cu_idx] = units_to_index[cu_idx]->ExtractDIEsScoped();
   };
 
+  // Share one thread pool across operations to avoid the overhead of
+  // recreating the threads.
+  llvm::ThreadPool pool;
+
   // Create a task runner that extracts dies for each DWARF unit in a
-  // separate thread
+  // separate thread.
   // First figure out which units didn't have their DIEs already
   // parsed and remember this.  If no DIEs were parsed prior to this index
   // function call, we are going to want to clear the CU dies after we are
   // done indexing to make sure we don't pull in all DWARF dies, but we need
   // to wait until all units have been indexed in case a DIE in one
   // unit refers to another and the indexes accesses those DIEs.
-  TaskMapOverInt(0, units_to_index.size(), extract_fn);
+  for (size_t i = 0; i < units_to_index.size(); ++i)
+    pool.async(extract_fn, i);
+  pool.wait();
 
   // Now create a task runner that can index each DWARF unit in a
   // separate thread so we can index quickly.
-
-  TaskMapOverInt(0, units_to_index.size(), parser_fn);
+  for (size_t i = 0; i < units_to_index.size(); ++i)
+    pool.async(parser_fn, i);
+  pool.wait();
 
   auto finalize_fn = [this, &sets](NameToDIE(IndexSet::*index)) {
     NameToDIE &result = m_set.*index;
     for (auto &set : sets)
       result.Append(set.*index);
     result.Finalize();
   };
 
-  TaskPool::RunTasks([&]() { finalize_fn(&IndexSet::function_basenames); },
-                     [&]() { finalize_fn(&IndexSet::function_fullnames); },
-                     [&]() { finalize_fn(&IndexSet::function_methods); },
-                     [&]() { finalize_fn(&IndexSet::function_selectors); },
-                     [&]() { finalize_fn(&IndexSet::objc_class_selectors); },
-                     [&]() { finalize_fn(&IndexSet::globals); },
-                     [&]() { finalize_fn(&IndexSet::types); },
-                     [&]() { finalize_fn(&IndexSet::namespaces); });
+  pool.async(finalize_fn, &IndexSet::function_basenames);
+  pool.async(finalize_fn, &IndexSet::function_fullnames);
+  pool.async(finalize_fn, &IndexSet::function_methods);
+  pool.async(finalize_fn, &IndexSet::function_selectors);
+  pool.async(finalize_fn, &IndexSet::objc_class_selectors);
+  pool.async(finalize_fn, &IndexSet::globals);
+  pool.async(finalize_fn, &IndexSet::types);
+  pool.async(finalize_fn, &IndexSet::namespaces);
+  pool.wait();
 }
 
 void ManualDWARFIndex::IndexUnit(DWARFUnit &unit, SymbolFileDWARFDwo *dwp,
                                  IndexSet &set) {
   Log *log = LogChannelDWARF::GetLogIfAll(DWARF_LOG_LOOKUPS);
 
   if (log) {
     m_module.LogMessage(
         log, "ManualDWARFIndex::IndexUnit for unit at .debug_info[0x%8.8x]",

lldb/unittests/Host/CMakeLists.txt

 set (FILES
   ConnectionFileDescriptorTest.cpp
   FileActionTest.cpp
   FileSystemTest.cpp
   FileTest.cpp
   HostInfoTest.cpp
   HostTest.cpp
   MainLoopTest.cpp
   NativeProcessProtocolTest.cpp
   ProcessLaunchInfoTest.cpp
   SocketAddressTest.cpp
   SocketTest.cpp
   SocketTestUtilities.cpp
-  TaskPoolTest.cpp
 )
 
 if (CMAKE_SYSTEM_NAME MATCHES "Linux|Android")
   list(APPEND FILES
     linux/HostTest.cpp
     linux/SupportTest.cpp
   )
 endif()
 
 add_lldb_unittest(HostTests
   ${FILES}
   LINK_LIBS
     lldbHost
     lldbUtilityHelpers
     LLVMTestingSupport
   )

lldb/unittests/Host/TaskPoolTest.cpp

-#include "gtest/gtest.h"
-
-#include "lldb/Host/TaskPool.h"
-
-using namespace lldb_private;
-
-TEST(TaskPoolTest, AddTask) {
-  auto fn = [](int x) { return x * x + 1; };
-
-  auto f1 = TaskPool::AddTask(fn, 1);
-  auto f2 = TaskPool::AddTask(fn, 2);
-  auto f3 = TaskPool::AddTask(fn, 3);
-  auto f4 = TaskPool::AddTask(fn, 4);
-
-  ASSERT_EQ(10, f3.get());
-  ASSERT_EQ(2, f1.get());
-  ASSERT_EQ(17, f4.get());
-  ASSERT_EQ(5, f2.get());
-}
-
-TEST(TaskPoolTest, RunTasks) {
-  std::vector<int> r(4);
-
-  auto fn = [](int x, int &y) { y = x * x + 1; };
-
-  TaskPool::RunTasks([fn, &r]() { fn(1, r[0]); }, [fn, &r]() { fn(2, r[1]); },
-                     [fn, &r]() { fn(3, r[2]); }, [fn, &r]() { fn(4, r[3]); });
-
-  ASSERT_EQ(2, r[0]);
-  ASSERT_EQ(5, r[1]);
-  ASSERT_EQ(10, r[2]);
-  ASSERT_EQ(17, r[3]);
-}
-
-TEST(TaskPoolTest, TaskMap) {
-  int data[4];
-  auto fn = [&data](int x) { data[x] = x * x; };
-
-  TaskMapOverInt(0, 4, fn);
-
-  ASSERT_EQ(data[0], 0);
-  ASSERT_EQ(data[1], 1);
-  ASSERT_EQ(data[2], 4);
-  ASSERT_EQ(data[3], 9);
-}