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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added inline comments.


================
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)
----------------
clayborg wrote:
> 
actually probably best to do it the way I originally described all inside of "llvm::ThreadPool &Debugger::GetThreadPool()" since multiple debuggers can exist within the same process.


================
Comment at: lldb/source/Core/Debugger.cpp:1969
+llvm::ThreadPool &Debugger::GetThreadPool() {
+  static llvm::ThreadPool threadpool(llvm::optimal_concurrency());
+  return threadpool;
----------------
clayborg wrote:
> 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;
Probably best to do it as described above where everything is contained in this function due to multiple debuggers being possible, so we can't tie anything to the lifetime of any debugger and we need to not crash when the main thread exits first


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

https://reviews.llvm.org/D123226