Skip to content
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.

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

[llvm] Make sure that BenchmarkFactory::close() is called #592

Merged
merged 5 commits into from
Mar 3, 2022
Merged
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions compiler_gym/envs/llvm/service/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ cc_binary(
name = "compiler_gym-llvm-service-prelinked",
srcs = ["RunService.cc"],
deps = [
":BenchmarkFactory",
":LlvmSession",
"//compiler_gym/service/runtime:cc_runtime",
],
Expand Down
3 changes: 3 additions & 0 deletions compiler_gym/envs/llvm/service/Benchmark.cc
Original file line number Diff line number Diff line change
Expand Up @@ -162,9 +162,12 @@ Benchmark::Benchmark(const std::string& name, std::unique_ptr<llvm::LLVMContext>
needsRecompile_(true) {}

void Benchmark::close() {
VLOG(3) << "Closing benchmark " << name() << " with scratch directory "
<< scratchDirectory().string();
sys::error_code ec;
fs::remove_all(scratchDirectory(), ec);
CHECK(!ec) << "Failed to delete scratch directory: " << scratchDirectory().string();
VLOG(3) << "Closed benchmark " << name();
}

std::unique_ptr<Benchmark> Benchmark::clone(const fs::path& workingDirectory) const {
Expand Down
1 change: 1 addition & 0 deletions compiler_gym/envs/llvm/service/BenchmarkFactory.cc
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ void BenchmarkFactory::close() {
for (auto& entry : benchmarks_) {
entry.second.close();
}
benchmarks_.clear();
}

Status BenchmarkFactory::getBenchmark(const BenchmarkProto& benchmarkMessage,
Expand Down
1 change: 1 addition & 0 deletions compiler_gym/envs/llvm/service/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ cg_cc_binary(
"RunService.cc"
DEPS
::LlvmSession
::BenchmarkFactory
compiler_gym::service::runtime::cc_runtime
)

Expand Down
15 changes: 14 additions & 1 deletion compiler_gym/envs/llvm/service/RunService.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
//
// This source code is licensed under the MIT license found in the
// LICENSE file in the root directory of this source tree.
#include "compiler_gym/envs/llvm/service/BenchmarkFactory.h"
#include "compiler_gym/envs/llvm/service/LlvmSession.h"
#include "compiler_gym/service/runtime/Runtime.h"
#include "llvm/InitializePasses.h"
Expand Down Expand Up @@ -59,5 +60,17 @@ void initLlvm() {

int main(int argc, char** argv) {
initLlvm();
createAndRunCompilerGymService<LlvmSession>(argc, argv, usage);
const auto ret = createAndRunCompilerGymService<LlvmSession>(argc, argv, usage);

// NOTE(github.com/facebookresearch/CompilerGym/issues/582): We need to make
// sure that BenchmarkFactory::close() is called on the global singleton
// instance, so that the temporary scratch directories are tidied up.
//
// TODO(github.com/facebookresearch/CompilerGym/issues/591): Once the runtime
// has been refactored to support intra-session mutable state, this singleton
// can be replaced by a member variable that is closed on
// CompilerGymServiceContext::shutdown().
BenchmarkFactory::getSingleton(FLAGS_working_dir).close();

return ret;
}
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ void setGrpcChannelOptions(grpc::ServerBuilder& builder);
// createAndRunCompilerGymServiceImpl(argc, argv, "usage string");
// }
template <typename CompilationSessionType>
[[noreturn]] void createAndRunCompilerGymServiceImpl(int argc, char** argv, const char* usage) {
[[nodiscard]] int createAndRunCompilerGymServiceImpl(int argc, char** argv, const char* usage) {
Copy link
Contributor

Choose a reason for hiding this comment

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

out of curiousity, what does [[nodiscard]] do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It triggers a compiler warning if the user doesn't do anything with the return value. With -Werror, it prevents the common typo:

int main(int argc, char** argv) {
  createAndRunCompilerGymServiceImpl(&argc, &argv, usage);
}

instead of:

int main(int argc, char** argv) {
  return createAndRunCompilerGymServiceImpl(&argc, &argv, usage);
}

// Register a signal handler for SIGTERM that will set the shutdown_signal
// future value.
std::signal(SIGTERM, shutdown_handler);
Expand All @@ -62,7 +62,7 @@ template <typename CompilationSessionType>
gflags::ParseCommandLineFlags(&argc, &argv, /*remove_flags=*/true);
if (argc > 1) {
std::cerr << "ERROR: unknown command line argument '" << argv[1] << '\'';
exit(1);
return 1;
}

// Set up the working and logging directories.
Expand Down Expand Up @@ -129,15 +129,16 @@ template <typename CompilationSessionType>
VLOG(2) << "Shutting down the RPC service";
server->Shutdown();
serverThread.join();
VLOG(2) << "Service closed";

if (service.sessionCount()) {
LOG(ERROR) << "ERROR: Killing a service with " << service.sessionCount()
<< (service.sessionCount() > 1 ? " active sessions!" : " active session!")
<< std::endl;
exit(6);
return 6;
}

exit(0);
return 0;
}

} // namespace compiler_gym::runtime
10 changes: 5 additions & 5 deletions compiler_gym/service/runtime/Runtime.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,20 +20,20 @@ namespace compiler_gym::runtime {
* #include "my_compiler_service/MyCompilationSession.h"
*
* int main(int argc, char** argv) {
* createAndRunCompilerGymService<MyCompilationSession>(
* return createAndRunCompilerGymService<MyCompilationSession>(
* argc, argc, "My compiler service"
* );
* }
* \endcode
*
* This function never returns.
*
* @tparam CompilationSessionType A sublass of CompilationSession that provides
* implementations of the abstract methods.
*
* @return An integer return code.
*/
template <typename CompilationSessionType>
[[noreturn]] void createAndRunCompilerGymService(int argc, char** argv, const char* usage) {
createAndRunCompilerGymServiceImpl<CompilationSessionType>(argc, argv, usage);
[[nodiscard]] int createAndRunCompilerGymService(int argc, char** argv, const char* usage) {
return createAndRunCompilerGymServiceImpl<CompilationSessionType>(argc, argv, usage);
}

} // namespace compiler_gym::runtime
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ def main(argv):
logging.info("Shutting down the RPC service")
server.stop(60).wait()
server_thread.join()
logging.info("Service closed")

if len(service.sessions):
print(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,5 +133,5 @@ class ExampleCompilationSession final : public CompilationSession {
} // namespace

int main(int argc, char** argv) {
runtime::createAndRunCompilerGymService<ExampleCompilationSession>(argc, argv, usage);
return runtime::createAndRunCompilerGymService<ExampleCompilationSession>(argc, argv, usage);
}