Page MenuHomePhabricator

[lldb] use one shared ThreadPool and task groups
ClosedPublic

Authored by llunak on Apr 6 2022, 8:32 AM.

Details

Summary

As a preparation for parallelizing loading of symbols (D122975), it is necessary to use just one thread pool to avoid using a thread pool from inside a task of another thread pool.

The ThreadPool groups change is D123225 .

Diff Detail

Event Timeline

llunak created this revision.Apr 6 2022, 8:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2022, 8:32 AM
llunak requested review of this revision.Apr 6 2022, 8:32 AM
tschuett added a subscriber: tschuett.EditedApr 6 2022, 8:32 AM

You cannot access/find the global thread pool? Sorry, wrong diff.

aganea added a subscriber: aganea.Apr 6 2022, 1:25 PM
aganea added inline comments.
lldb/source/Core/Debugger.cpp
1976

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

lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
126–134

Like suggested in D123225, wouldn't it be better if we had group.async(finalize_fn, &IndexSet::function_basenames)? Same for the waits: group.wait();.

clayborg requested changes to this revision.Apr 6 2022, 2:08 PM
clayborg added inline comments.
lldb/source/Core/Debugger.cpp
1976

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.

This revision now requires changes to proceed.Apr 6 2022, 2:08 PM
aganea added inline comments.Apr 6 2022, 2:20 PM
lldb/source/Core/Debugger.cpp
1976

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.

clayborg added inline comments.Apr 6 2022, 2:41 PM
lldb/source/Core/Debugger.cpp
1976

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;
llunak marked an inline comment as done.Apr 16 2022, 10:50 AM
llunak added inline comments.
lldb/source/Core/Debugger.cpp
1976

I've changed the code to initialize/cleanup from Debugger ctor/dtor.

llunak updated this revision to Diff 423246.Apr 16 2022, 10:55 AM
llunak edited the summary of this revision. (Show Details)

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

clayborg requested changes to this revision.Apr 18 2022, 6:01 PM
clayborg added inline comments.
lldb/source/Core/Debugger.cpp
760
850

remove

1976

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;

This revision now requires changes to proceed.Apr 18 2022, 6:01 PM
clayborg added inline comments.Apr 18 2022, 6:04 PM
lldb/source/Core/Debugger.cpp
760

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.

1976

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

llunak marked 2 inline comments as done.Apr 30 2022, 11:38 PM
llunak added inline comments.
lldb/source/Core/Debugger.cpp
1976

I see, I thought there could be just one Debugger instance. I'll use the call_once code from above then.

llunak updated this revision to Diff 426269.Apr 30 2022, 11:39 PM

Changed to use std::call_once().

clayborg accepted this revision.May 2 2022, 4:00 PM
This revision is now accepted and ready to land.May 2 2022, 4:00 PM
This revision was automatically updated to reflect the committed changes.