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

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.


================
Comment at: lldb/source/Core/Debugger.cpp:761
   m_instance_name.SetString(llvm::formatv("debugger_{0}", GetID()).str());
+  g_threadpool = std::make_unique<llvm::ThreadPool>(llvm::optimal_concurrency());
   if (log_callback)
----------------



================
Comment at: lldb/source/Core/Debugger.cpp:852
     m_command_interpreter_up->Clear();
+    g_threadpool.reset();
   });
----------------
remove


================
Comment at: lldb/source/Core/Debugger.cpp:1969
+llvm::ThreadPool &Debugger::GetThreadPool() {
+  static llvm::ThreadPool threadpool(llvm::optimal_concurrency());
+  return threadpool;
----------------
llunak wrote:
> 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.
> 
You can't do this as there can be more than one debugger. This needs to be a true global and use a once_flag. 

Xcode creates one debugger per target window, that way each one has its own command interpreter etc;


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

https://reviews.llvm.org/D123226