Skip to content

[lldb] Create dependent modules in parallel #114507

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Nov 2, 2024
Merged

[lldb] Create dependent modules in parallel #114507

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9a269fa
[lldb] Create dependent modules in parallel
JDevlieghere Nov 1, 2024
0bba5f7
Fix formatting
JDevlieghere Nov 1, 2024
535c27d
Don't lock for the duration fo GetDependentModules
JDevlieghere Nov 1, 2024
37243c1
Merge branch 'main' into parallel-dependents
JDevlieghere Nov 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 28 additions & 5 deletions lldb/source/Target/Target.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@

#include "llvm/ADT/ScopeExit.h"
#include "llvm/ADT/SetVector.h"
#include "llvm/Support/ThreadPool.h"

#include <memory>
#include <mutex>
Expand Down Expand Up @@ -1575,7 +1576,6 @@ void Target::SetExecutableModule(ModuleSP &executable_sp,
m_arch.GetSpec().GetTriple().getTriple());
}

FileSpecList dependent_files;
ObjectFile *executable_objfile = executable_sp->GetObjectFile();
bool load_dependents = true;
switch (load_dependent_files) {
Expand All @@ -1591,10 +1591,14 @@ void Target::SetExecutableModule(ModuleSP &executable_sp,
}

if (executable_objfile && load_dependents) {
// FileSpecList is not thread safe and needs to be synchronized.
FileSpecList dependent_files;
std::mutex dependent_files_mutex;

// ModuleList is thread safe.
ModuleList added_modules;
executable_objfile->GetDependentModules(dependent_files);
for (uint32_t i = 0; i < dependent_files.GetSize(); i++) {
FileSpec dependent_file_spec(dependent_files.GetFileSpecAtIndex(i));

auto GetDependentModules = [&](FileSpec dependent_file_spec) {
FileSpec platform_dependent_file_spec;
if (m_platform_sp)
m_platform_sp->GetFileWithUUID(dependent_file_spec, nullptr,
Expand All @@ -1608,9 +1612,28 @@ void Target::SetExecutableModule(ModuleSP &executable_sp,
if (image_module_sp) {
added_modules.AppendIfNeeded(image_module_sp, false);
ObjectFile *objfile = image_module_sp->GetObjectFile();
if (objfile)
if (objfile) {
std::lock_guard<std::mutex> guard(dependent_files_mutex);
objfile->GetDependentModules(dependent_files);
Copy link
Contributor

@DmT021 DmT021 Nov 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this operation is heavy in any of the ObjectFile implementations. If it is we may want to lock the mutex only when an actual append to the dependent_files happens.

Copy link
Collaborator

@jasonmolenda jasonmolenda Nov 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good question. for mach-o this is a simple iteration over the load commands, but we might want to to pass a temporary file list object to this method, and then acquire the lock and append its entries to dependency_files, if we didn't want to assume that.

Copy link
Member Author

@JDevlieghere JDevlieghere Nov 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah fair enough. Even the Mach-O one is doing some filesystem operations that really shouldn't be happening while holding the lock. I don't think we need to make every FileSpecList thread safe, and passing an optional mutex* was messy, so I went with the local copy as Jason suggested.

Copy link
Contributor

@DmT021 DmT021 Nov 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just in case, did you check for a regression in performance after the latest patch?

Copy link
Member Author

@JDevlieghere JDevlieghere Nov 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I did. I was hoping for a slight speedup, but the difference was in the noise.

}
}
};

executable_objfile->GetDependentModules(dependent_files);

llvm::ThreadPoolTaskGroup task_group(Debugger::GetThreadPool());
for (uint32_t i = 0; i < dependent_files.GetSize(); i++) {
// Process all currently known dependencies in parallel in the innermost
// loop. This may create newly discovered dependencies to be appended to
// dependent_files. We'll deal with these files during the next
// iteration of the outermost loop.
{
std::lock_guard<std::mutex> guard(dependent_files_mutex);
for (; i < dependent_files.GetSize(); i++)
task_group.async(GetDependentModules,
Copy link
Collaborator

@adrian-prantl adrian-prantl Nov 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

async inside a lock_guard looks dangerous. I assume you're sure this won't recurse into this function? :-)

Copy link
Member Author

@JDevlieghere JDevlieghere Nov 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't, but if that becomes a problem we can do the same trick that Jason suggested and iterate over a copy of the list here. For now that seems unnecessary but hopefully someone will see this comment if that ever changes :-)

dependent_files.GetFileSpecAtIndex(i));
}
task_group.wait();
}
ModulesDidLoad(added_modules);
}
Expand Down
Loading