Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
3 changes: 3 additions & 0 deletions toolchain/driver/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,11 @@ cc_library(
"//toolchain/install:install_paths",
"@llvm-project//clang:basic",
"@llvm-project//clang:clang-driver",
"@llvm-project//clang:codegen",
"@llvm-project//clang:driver",
"@llvm-project//clang:frontend",
"@llvm-project//clang:frontend_tool",
"@llvm-project//clang:serialization",
"@llvm-project//llvm:Core",
"@llvm-project//llvm:Object",
"@llvm-project//llvm:Support",
Expand Down
3 changes: 2 additions & 1 deletion toolchain/driver/build_runtimes_subcommand.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,8 @@ auto BuildRuntimesSubcommand::RunInternal(DriverEnv& driver_env)
: Runtimes::Make(explicit_output_path, driver_env.vlog_stream));
CARBON_ASSIGN_OR_RETURN(auto tmp_dir, Filesystem::MakeTmpDir());

return runner.BuildTargetResourceDir(features, runtimes, tmp_dir.abs_path());
return runner.BuildTargetResourceDir(features, runtimes, tmp_dir.abs_path(),
*driver_env.thread_pool);
}

} // namespace Carbon
275 changes: 219 additions & 56 deletions toolchain/driver/clang_runner.cpp

Large diffs are not rendered by default.

32 changes: 28 additions & 4 deletions toolchain/driver/clang_runner.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "common/ostream.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/ThreadPool.h"
#include "llvm/Support/VirtualFileSystem.h"
#include "llvm/TargetParser/Triple.h"
#include "toolchain/driver/runtimes_cache.h"
Expand All @@ -25,11 +26,15 @@ namespace Carbon {
// incorporating custom command line flags from user invocations that we don't
// parse, but will pass transparently along to Clang itself.
//
// This class is thread safe, allowing multiple threads to share a single runner
// and concurrently invoke Clang.
//
// This doesn't literally use a subprocess to invoke Clang; it instead tries to
// directly use the Clang command line driver library. We also work to simplify
// how that driver operates and invoke it in an opinionated way to get the best
// behavior for our expected use cases in the Carbon driver:
//
// - Ensure thread-safe invocation of Clang to enable concurrent usage.
// - Minimize canonicalization of file names to try to preserve the paths as
// users type them.
// - Minimize the use of subprocess invocations which are expensive on some
Expand All @@ -42,6 +47,13 @@ namespace Carbon {
// standard output and standard error, and otherwise can only read and write
// files based on their names described in the arguments. It doesn't provide any
// higher-level abstraction such as streams for inputs or outputs.
//
// TODO: Switch the diagnostic machinery to buffer and do locked output so that
// concurrent invocations of Clang don't intermingle their diagnostic output.
//
// TODO: If support for thread-local overrides of `llvm::errs` and `llvm::outs`
// becomes available upstream, also buffer and synchronize those streams to
// further improve the behavior of concurrent invocations.
class ClangRunner : ToolRunnerBase {
public:
// Build a Clang runner that uses the provided `exe_name` and `err_stream`.
Expand All @@ -52,7 +64,8 @@ class ClangRunner : ToolRunnerBase {
Runtimes::Cache* on_demand_runtimes_cache,
llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> fs,
llvm::raw_ostream* vlog_stream = nullptr,
bool build_runtimes_on_demand = false);
bool build_runtimes_on_demand = false,
llvm::ThreadPoolInterface* runtimes_build_thread_pool = nullptr);

// Run Clang with the provided arguments.
//
Expand Down Expand Up @@ -88,7 +101,8 @@ class ClangRunner : ToolRunnerBase {
// return the path.
auto BuildTargetResourceDir(const Runtimes::Cache::Features& features,
Runtimes& runtimes,
const std::filesystem::path& tmp_path)
const std::filesystem::path& tmp_path,
llvm::ThreadPoolInterface& threads)
-> ErrorOr<std::filesystem::path>;

// Enable leaking memory.
Expand All @@ -103,6 +117,14 @@ class ClangRunner : ToolRunnerBase {
auto EnableLeakingMemory() -> void { enable_leaking_ = true; }

private:
// Emulates `cc1_main` but in a way that doesn't assume it is running in the
// main thread and can more easily fit into library calls to do compiles.
//
// TODO: Much of the logic here should be factored out of the CC1
// implementation in Clang's driver and into a reusable part of its libraries.
// That should allow reducing the code here to a minimal amount.
auto RunCC1(llvm::SmallVectorImpl<const char*>& cc1_args) -> int;

// Handles building the Clang driver and passing the arguments down to it.
auto RunInternal(llvm::ArrayRef<llvm::StringRef> args, llvm::StringRef target,
std::optional<llvm::StringRef> target_resource_dir_path)
Expand All @@ -125,15 +147,17 @@ class ClangRunner : ToolRunnerBase {
auto BuildBuiltinsLib(llvm::StringRef target,
const llvm::Triple& target_triple,
const std::filesystem::path& tmp_path,
Filesystem::DirRef lib_dir) -> ErrorOr<Success>;
Filesystem::DirRef lib_dir,
llvm::ThreadPoolInterface& threads) -> ErrorOr<Success>;

Runtimes::Cache* runtimes_cache_;

llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> fs_;
llvm::IntrusiveRefCntPtr<clang::DiagnosticIDs> diagnostic_ids_;

std::optional<std::filesystem::path> prebuilt_runtimes_path_;

llvm::ThreadPoolInterface* runtimes_build_thread_pool_ = nullptr;

bool build_runtimes_on_demand_;
bool enable_leaking_ = false;
};
Expand Down
5 changes: 3 additions & 2 deletions toolchain/driver/clang_runner_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -191,8 +191,9 @@ TEST_F(ClangRunnerTest, BuildResourceDir) {
Runtimes::Cache::Features features = {.target = target};
auto runtimes = *runtimes_cache_.Lookup(features);
auto tmp_dir = *Filesystem::MakeTmpDir();
auto build_result =
runner.BuildTargetResourceDir(features, runtimes, tmp_dir.abs_path());
llvm::DefaultThreadPool threads(llvm::optimal_concurrency());
auto build_result = runner.BuildTargetResourceDir(
features, runtimes, tmp_dir.abs_path(), threads);
ASSERT_TRUE(build_result.ok()) << build_result.error();
std::filesystem::path resource_dir_path = std::move(*build_result);

Expand Down
3 changes: 2 additions & 1 deletion toolchain/driver/clang_subcommand.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,8 @@ auto ClangSubcommand::Run(DriverEnv& driver_env) -> DriverResult {
ClangRunner runner(
driver_env.installation, &driver_env.runtimes_cache, driver_env.fs,
driver_env.vlog_stream,
/*build_runtimes_on_demand=*/options_.build_runtimes_on_demand);
/*build_runtimes_on_demand=*/options_.build_runtimes_on_demand,
driver_env.thread_pool);

// Don't run Clang when fuzzing, it is known to not be reliable under fuzzing
// due to many unfixed issues.
Expand Down
28 changes: 28 additions & 0 deletions toolchain/driver/driver.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ struct Options {
bool verbose = false;
bool fuzzing = false;
bool include_diagnostic_kind = false;
bool threads = true;

llvm::StringRef runtimes_cache_path;
llvm::StringRef prebuilt_runtimes_path;
Expand Down Expand Up @@ -124,6 +125,25 @@ applies to each message that forms a diagnostic, not just the primary message.
},
[&](auto& arg_b) { arg_b.Set(&include_diagnostic_kind); });

b.AddFlag(
{
.name = "threads",
.help = R"""(
Controls whether threads are used to build runtimes.

When enabled (the default), Carbon will try to build runtime libraries using
threads to parallelize the operation. How many threads is controlled
automatically by the system.

Disabling threads ensures a single threaded build of the runtimes which can help
when there are errors or other output.
)""",
},
[&](auto& arg_b) {
arg_b.Default(true);
arg_b.Set(&threads);
});

runtimes.AddTo(b, &selected_subcommand);
clang.AddTo(b, &selected_subcommand);
compile.AddTo(b, &selected_subcommand);
Expand Down Expand Up @@ -208,6 +228,14 @@ auto Driver::RunCommand(llvm::ArrayRef<llvm::StringRef> args) -> DriverResult {
driver_env_.fuzzing = true;
}

llvm::SingleThreadExecutor single_thread({.ThreadsRequested = 1});
std::optional<llvm::DefaultThreadPool> threads;
driver_env_.thread_pool = &single_thread;
if (options.threads) {
threads.emplace(llvm::optimal_concurrency());
driver_env_.thread_pool = &*threads;
}
Comment on lines +231 to +237
Copy link
Contributor

Choose a reason for hiding this comment

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

We're are creating this SingleThreadExecutor at multiple levels it seems, both here and inside the ClangRunner. Could we consolidate to a single place - maybe making ClangRunner expect to receive a non-null executor always?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could... I was just a bit torn doing so as it adds quite a bit of complexity to building and using the runner which is only needed if you're actually building runtimes.

Another alternative would be to have ClangRunner either accept a pre-built path or a thread pool to use for on-demand building, and remove the boolean option.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you forsee wanting to use other thread pools than SingleThreadExecutor and DefaultThreadPool? Maybe we could tell it if we want threads or not and have ClangRunner make what it needs?

enum class BuildRuntimesOnDemand {
   UsePrebuiltOnly,
   BuildOnSingleThreaded,
   BuildOnWorkerThreads,
}

Taking either a path XOR a thread pool also sounds good, whichever you pref

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you forsee wanting to use other thread pools than SingleThreadExecutor and DefaultThreadPool?

Some possibility. One thing I wonder is if we'll want a (much) larger thread pool to fully absorb the latency, but would like to avoid it given the overhead.

Maybe we could tell it if we want threads or not and have ClangRunner make what it needs?

When using threads, my expectation is that it'll be very desirable to use the existing thread pool to avoid paying the cost of spinning one up and forking all the threads.

It also allows more global management of the load.

This is why I somewhat like the driver either accepting or building a thread pool, and then making it available for any commands or subcommands to use.

Taking either a path XOR a thread pool also sounds good, whichever you pref

I think what I'm liking is to take one of:

  • A thread pool, enabling on-demand building in that pool.
  • A pre-built path that will be used.
  • Nothing, disabling on-demand building, and its up to the caller to use the runner in a way compatible with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I thought more about this and I think I had the fundamental wrong structure of this API.

I've restructured everything so that we have a simple constructor that only accepts the necessary components. Then there are three variations on Run -- using on-demand runtimes with a cache and thread pool, using pre-built runtimes, and using no runtimes. I've also updated comments and callers accourdingly. I think this ends up more clear, but PTAL and let me know.


CARBON_CHECK(options.selected_subcommand != nullptr);
return options.selected_subcommand->Run(driver_env_);
}
Expand Down
5 changes: 5 additions & 0 deletions toolchain/driver/driver_env.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
#include <utility>

#include "common/ostream.h"
#include "llvm/Support/ThreadPool.h"
#include "llvm/Support/Threading.h"
#include "llvm/Support/VirtualFileSystem.h"
#include "toolchain/diagnostics/diagnostic_emitter.h"
#include "toolchain/driver/runtimes_cache.h"
Expand Down Expand Up @@ -62,6 +64,9 @@ struct DriverEnv {
// A diagnostic emitter that has no locations.
Diagnostics::NoLocEmitter emitter;

// Thread pool available for use when concurrency is needed.
llvm::ThreadPoolInterface* thread_pool;

// For CARBON_VLOG.
llvm::raw_pwrite_stream* vlog_stream = nullptr;

Expand Down
Loading