date:20220416

[Lldb-commits] [PATCH] D123226: [lldb] use one shared ThreadPool and task groups

2022-04-16 Thread Luboš Luňák via Phabricator via lldb-commits

llunak marked an inline comment as done.
llunak added inline comments.



Comment at: lldb/source/Core/Debugger.cpp:1969
+llvm::ThreadPool &Debugger::GetThreadPool() {
+  static llvm::ThreadPool threadpool(llvm::optimal_concurrency());
+  return threadpool;

clayborg wrote:
> aganea wrote:
> > clayborg wrote:
> > > aganea wrote:
> > > > Ideally this should be explicitly created on the stack & passed along 
> > > > on the callstack or in a context, like MLIR does: 
> > > > https://github.com/llvm/llvm-project/blob/main/mlir/include/mlir/IR/MLIRContext.h#L48
> > > We need one instance of the thread pool so we can't create on the stack. 
> > > 
> > > Static is not great because of the C++ global destructor chain could try 
> > > and use if "main" exits and we still have threads running. I would do 
> > > something like the "g_debugger_list_ptr" where you create a static 
> > > variable in Debugger.cpp:105 which is a pointer, and we initialize it 
> > > inside of Debugger::Initialize() like we do for "g_debugger_list_ptr". 
> > > Then the thread pool will not cause shutdown crashes when "main" exits 
> > > before other threads.
> > > 
> > I meant having the `ThreadPool` in a context created by main() or the lib 
> > caller, before getting here in library/plugin code, and passed along. LLD 
> > does that too: 
> > https://github.com/llvm/llvm-project/blob/main/lld/ELF/Driver.cpp#L94 - the 
> > statics in `Debugger.cpp` seems like a workaround for that. In any case, 
> > it's better than having the `static` here.
> In LLDB, the lldb_private::Debugger has static functions that hand out things 
> that are needed for the debugger to do its work, so I like the static 
> function here. We don't allow any llvm or clang code to make it through our 
> public API (in lldb::SB* classes), so we do need code inside the debugger to 
> be able to access global resources and the debugger class is where this 
> should live. 
> 
> We can also just have code like:
> ```
> // NOTE: intentional leak to avoid issues with C++ destructor chain
> static llvm::ThreadPool *g_thread_pool = nullptr;
> static llvm::once_flag g_once_flag;
> llvm::call_once(g_once_flag, []() {
>   g_thread_pool = new llvm::ThreadPool(llvm::optimal_concurrency());
> });
> return *g_thread_pool;
> ```
> 
I've changed the code to initialize/cleanup from Debugger ctor/dtor.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123226/new/

https://reviews.llvm.org/D123226

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D123226: [lldb] use one shared ThreadPool and task groups

2022-04-16 Thread Luboš Luňák via Phabricator via lldb-commits

llunak updated this revision to Diff 423246.
llunak edited the summary of this revision.
llunak added a comment.

ThreadPool object is now created/destroyed by Debugger class ctor/dtor.
Adapted to API changes from D123225 .


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123226/new/

https://reviews.llvm.org/D123226

Files:
  lldb/include/lldb/Core/Debugger.h
  lldb/source/Core/Debugger.cpp
  lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp

Index: lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
@@ -13,6 +13,7 @@
 #include "Plugins/SymbolFile/DWARF/LogChannelDWARF.h"
 #include "Plugins/SymbolFile/DWARF/SymbolFileDWARFDwo.h"
 #include "lldb/Core/DataFileCache.h"
+#include "lldb/Core/Debugger.h"
 #include "lldb/Core/Module.h"
 #include "lldb/Core/Progress.h"
 #include "lldb/Symbol/ObjectFile.h"
@@ -94,7 +95,7 @@
 
   // Share one thread pool across operations to avoid the overhead of
   // recreating the threads.
-  llvm::ThreadPool pool(llvm::optimal_concurrency(units_to_index.size()));
+  llvm::ThreadPoolTaskGroup task_group(Debugger::GetThreadPool());
 
   // Create a task runner that extracts dies for each DWARF unit in a
   // separate thread.
@@ -105,14 +106,14 @@
   // to wait until all units have been indexed in case a DIE in one
   // unit refers to another and the indexes accesses those DIEs.
   for (size_t i = 0; i < units_to_index.size(); ++i)
-pool.async(extract_fn, i);
-  pool.wait();
+task_group.async(extract_fn, i);
+  task_group.wait();
 
   // Now create a task runner that can index each DWARF unit in a
   // separate thread so we can index quickly.
   for (size_t i = 0; i < units_to_index.size(); ++i)
-pool.async(parser_fn, i);
-  pool.wait();
+task_group.async(parser_fn, i);
+  task_group.wait();
 
   auto finalize_fn = [this, &sets, &progress](NameToDIE(IndexSet::*index)) {
 NameToDIE &result = m_set.*index;
@@ -122,15 +123,15 @@
 progress.Increment();
   };
 
-  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();
+  task_group.async(finalize_fn, &IndexSet::function_basenames);
+  task_group.async(finalize_fn, &IndexSet::function_fullnames);
+  task_group.async(finalize_fn, &IndexSet::function_methods);
+  task_group.async(finalize_fn, &IndexSet::function_selectors);
+  task_group.async(finalize_fn, &IndexSet::objc_class_selectors);
+  task_group.async(finalize_fn, &IndexSet::globals);
+  task_group.async(finalize_fn, &IndexSet::types);
+  task_group.async(finalize_fn, &IndexSet::namespaces);
+  task_group.wait();
 
   SaveToCache();
 }
Index: lldb/source/Core/Debugger.cpp
===
--- lldb/source/Core/Debugger.cpp
+++ lldb/source/Core/Debugger.cpp
@@ -66,6 +66,7 @@
 #include "llvm/Support/DynamicLibrary.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Process.h"
+#include "llvm/Support/ThreadPool.h"
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/raw_ostream.h"
 
@@ -96,6 +97,7 @@
 
 static lldb::user_id_t g_unique_id = 1;
 static size_t g_debugger_event_thread_stack_bytes = 8 * 1024 * 1024;
+static std::unique_ptr g_threadpool;
 
 #pragma mark Static Functions
 
@@ -756,6 +758,7 @@
 GetStaticBroadcasterClass().AsCString()),
   m_forward_listener_sp(), m_clear_once() {
   m_instance_name.SetString(llvm::formatv("debugger_{0}", GetID()).str());
+  g_threadpool = std::make_unique(llvm::optimal_concurrency());
   if (log_callback)
 m_log_callback_stream_sp =
 std::make_shared(log_callback, baton);
@@ -846,6 +849,7 @@
 GetInputFile().Close();
 
 m_command_interpreter_up->Clear();
+g_threadpool.reset();
   });
 }
 
@@ -1970,3 +1974,8 @@
 
   return err;
 }
+
+llvm::ThreadPool &Debugger::GetThreadPool() {
+  assert(g_threadpool);
+  return *g_threadpool;
+}
Index: lldb/include/lldb/Core/Debugger.h
===
--- lldb/include/lldb/Core/Debugger.h
+++ lldb/include/lldb/Core/Debugger.h
@@ -49,6 +49,7 @@
 
 namespace llvm {
 class raw_ostream;
+class ThreadPool;
 }
 
 namespace lldb_private {
@@ -379,6 +380,9 @@
 return m_broadcaster_manager_sp;
   }
 
+  /// Shared thread poll. Use only with ThreadPoolTaskGroup.
+  static llvm::ThreadPool &GetThreadPool();
+
   /// Report warning events.
   ///
   /// Progress events will be delivered to

[Lldb-commits] [PATCH] D122975: parallelize calling of Module::PreloadSymbols()

2022-04-16 Thread Luboš Luňák via Phabricator via lldb-commits

llunak updated this revision to Diff 423247.
llunak added a comment.

Adapted to API changes from D123225 .


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122975/new/

https://reviews.llvm.org/D122975

Files:
  lldb/source/Target/Target.cpp


Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -62,6 +62,7 @@
 
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SetVector.h"
+#include "llvm/Support/ThreadPool.h"
 
 #include 
 #include 
@@ -1623,6 +1624,17 @@
 void Target::ModulesDidLoad(ModuleList &module_list) {
   const size_t num_images = module_list.GetSize();
   if (m_valid && num_images) {
+if (GetPreloadSymbols()) {
+  // Try to preload symbols in parallel.
+  llvm::ThreadPoolTaskGroup task_group(Debugger::GetThreadPool());
+  auto preload_symbols_fn = [&](size_t idx) {
+ModuleSP module_sp(module_list.GetModuleAtIndex(idx));
+module_sp->PreloadSymbols();
+  };
+  for (size_t idx = 0; idx < num_images; ++idx)
+task_group.async(preload_symbols_fn, idx);
+  task_group.wait();
+}
 for (size_t idx = 0; idx < num_images; ++idx) {
   ModuleSP module_sp(module_list.GetModuleAtIndex(idx));
   LoadScriptingResourceForModule(module_sp, this);
@@ -2170,11 +2182,6 @@
   });
 }
 
-// Preload symbols outside of any lock, so hopefully we can do this for
-// each library in parallel.
-if (GetPreloadSymbols())
-  module_sp->PreloadSymbols();
-
 llvm::SmallVector replaced_modules;
 for (ModuleSP &old_module_sp : old_modules) {
   if (m_images.GetIndexForModule(old_module_sp.get()) !=


Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -62,6 +62,7 @@
 
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SetVector.h"
+#include "llvm/Support/ThreadPool.h"
 
 #include 
 #include 
@@ -1623,6 +1624,17 @@
 void Target::ModulesDidLoad(ModuleList &module_list) {
   const size_t num_images = module_list.GetSize();
   if (m_valid && num_images) {
+if (GetPreloadSymbols()) {
+  // Try to preload symbols in parallel.
+  llvm::ThreadPoolTaskGroup task_group(Debugger::GetThreadPool());
+  auto preload_symbols_fn = [&](size_t idx) {
+ModuleSP module_sp(module_list.GetModuleAtIndex(idx));
+module_sp->PreloadSymbols();
+  };
+  for (size_t idx = 0; idx < num_images; ++idx)
+task_group.async(preload_symbols_fn, idx);
+  task_group.wait();
+}
 for (size_t idx = 0; idx < num_images; ++idx) {
   ModuleSP module_sp(module_list.GetModuleAtIndex(idx));
   LoadScriptingResourceForModule(module_sp, this);
@@ -2170,11 +2182,6 @@
   });
 }
 
-// Preload symbols outside of any lock, so hopefully we can do this for
-// each library in parallel.
-if (GetPreloadSymbols())
-  module_sp->PreloadSymbols();
-
 llvm::SmallVector replaced_modules;
 for (ModuleSP &old_module_sp : old_modules) {
   if (m_images.GetIndexForModule(old_module_sp.get()) !=
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [lldb] 2696d82 - Windows: correct iteration of additional search paths

2022-04-16 Thread Saleem Abdulrasool via lldb-commits


Author: Saleem Abdulrasool
Date: 2022-04-16T18:01:02-07:00
New Revision: 2696d82fa0c323d92d8794f0a34ea9619888fae9

URL: 
https://github.com/llvm/llvm-project/commit/2696d82fa0c323d92d8794f0a34ea9619888fae9
DIFF: 
https://github.com/llvm/llvm-project/commit/2696d82fa0c323d92d8794f0a34ea9619888fae9.diff

LOG: Windows: correct iteration of additional search paths

This adjusts the path iteration - `paths` is a null-terminated sequence
of C strings, creating an array from a single contiguous buffer.  We
would previously continue to iterate indefinitely as we did not check if
we had encountered the terminator.

Found by inspection.

Added: 


Modified: 
lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp

Removed: 




diff  --git a/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp 
b/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
index b0501af7df30b..38f387dfdb29d 100644
--- a/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
+++ b/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
@@ -651,7 +651,7 @@ _Static_assert(sizeof(struct __lldb_LoadLibraryResult) <= 3 
* sizeof(void *),
 
 void * __lldb_LoadLibraryHelper(const wchar_t *name, const wchar_t *paths,
 __lldb_LoadLibraryResult *result) {
-  for (const wchar_t *path = paths; path; ) {
+  for (const wchar_t *path = paths; path && *path; ) {
 (void)AddDllDirectory(path);
 path += wcslen(path) + 1;
   }



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D123226: [lldb] use one shared ThreadPool and task groups

[Lldb-commits] [PATCH] D123226: [lldb] use one shared ThreadPool and task groups

[Lldb-commits] [PATCH] D122975: parallelize calling of Module::PreloadSymbols()

[Lldb-commits] [lldb] 2696d82 - Windows: correct iteration of additional search paths

4 matches

Site Navigation

Mail list logo

Footer information