Repository: bazelbuild/bazel
Issue: add an option to disable the idle GC
Labels: type: feature request

If a Bazel server is idle for 10 seconds after finishing a command, it will invoke `System.gc()`. On HotSpot, `System.gc()` triggers an aggressive full GC. In practice, I see `System.gc()` in a Bazel server with a 2GB heap fully consume 4 cores for several seconds. This behavior is problematic for workflows that involve building a binary and then immediately running it; if the binary itself is resource intensive and long-running, Bazel will steal CPU from it. One can effectively disable the idle GC today with `--host_jvm_args=-XX:+DisableExplicitGC`, but this is hacky and makes instrumentation commands like `bazel info used-heap-size-after-gc` silently useless. So, I propose introducing an official startup flag to disable idle GC.

A new option is my conservative proposal. Going further, I'd be interested in learning the reasons for Bazel's idle GC behavior. To me, it's polite to only use lots of CPU when Bazel is synchronously processing a command. I suppose in theory an idle GC saves GC work during synchronous work, but can the value of that be measured? If a user is iterating by running `build` or `test` on the same target, most server state should be essentially persistent and won't benefit from being repeatedly walked by a full GC. If the idle GC is valuable, could Bazel use a whole-system idleness check in lieu of the current 10 second heuristic?

Issue comments:
@meisterT for triage since this is perf related. I do agree that "every 10 seconds full gc" is not awesome.
I think we should delete this code. I have yet to see a benchmark showing that the application logic choosing to do a GC results in better performance than the JVM triggering the GC itself.

Pull request review comments:
I think it's better not to create the IdleServerTasks if we don't have to, so perhaps rename the option and check for nullness below. WDYT?
Contrary to my earlier statement, I think it's safer to default to the old behavior. Then, we can release it with the toggle and flip the default value in a later release.
You mean rename the command line option or something else?

The name `IdleServerTasks` implies a generic framework for executing tasks at idle not just GC. Of course, it's not currently used for anything else. So, if I make the refactor I think you're suggesting, maybe I should just rename it `IdleGCTaskRunner`.
That makes sense.
My suggestion is to keep the name IdleServerTasks and the generic framework.
But instead of disabling the GC part of it with --idle_gc=no, I would disable the whole IdleServerTasks with --idle_server_tasks=no.  

Patch that resolved the issue:
diff --git a/src/main/cpp/blaze.cc b/src/main/cpp/blaze.cc
index 9e7d9798552d30..801a9314c00593 100644
--- a/src/main/cpp/blaze.cc
+++ b/src/main/cpp/blaze.cc
@@ -541,6 +541,11 @@ static vector<string> GetArgumentArray(
     // being "null" to set the programmatic default in the server.
     result.push_back("--digest_function=" + globals->options->digest_function);
   }
+  if (globals->options->idle_server_tasks) {
+    result.push_back("--idle_server_tasks");
+  } else {
+    result.push_back("--noidle_server_tasks");
+  }
   if (globals->options->oom_more_eagerly) {
     result.push_back("--experimental_oom_more_eagerly");
   } else {
diff --git a/src/main/cpp/startup_options.cc b/src/main/cpp/startup_options.cc
index a2d26b74d3f048..c54728f92e0a2a 100644
--- a/src/main/cpp/startup_options.cc
+++ b/src/main/cpp/startup_options.cc
@@ -93,6 +93,7 @@ StartupOptions::StartupOptions(const string &product_name,
           "com.google.devtools.build.lib.util.SingleLineFormatter"),
       expand_configs_in_place(true),
       digest_function(),
+      idle_server_tasks(true),
       original_startup_options_(std::vector<RcStartupFlag>()) {
   bool testing = !blaze::GetEnv("TEST_TMPDIR").empty();
   if (testing) {
@@ -133,6 +134,7 @@ StartupOptions::StartupOptions(const string &product_name,
   RegisterNullaryStartupFlag("experimental_oom_more_eagerly");
   RegisterNullaryStartupFlag("fatal_event_bus_exceptions");
   RegisterNullaryStartupFlag("host_jvm_debug");
+  RegisterNullaryStartupFlag("idle_server_tasks");
   RegisterNullaryStartupFlag("ignore_all_rc_files");
   RegisterNullaryStartupFlag("watchfs");
   RegisterNullaryStartupFlag("write_command_log");
@@ -334,6 +336,12 @@ blaze_exit_code::ExitCode StartupOptions::ProcessArg(
   } else if (GetNullaryOption(arg, "--noexpand_configs_in_place")) {
     expand_configs_in_place = false;
     option_sources["expand_configs_in_place"] = rcfile;
+  } else if (GetNullaryOption(arg, "--idle_server_tasks")) {
+    idle_server_tasks = true;
+    option_sources["idle_server_tasks"] = rcfile;
+  } else if (GetNullaryOption(arg, "--noidle_server_tasks")) {
+    idle_server_tasks = false;
+    option_sources["idle_server_tasks"] = rcfile;
   } else if ((value = GetUnaryOption(arg, next_arg,
                                      "--connect_timeout_secs")) != NULL) {
     if (!blaze_util::safe_strto32(value, &connect_timeout_secs) ||
diff --git a/src/main/cpp/startup_options.h b/src/main/cpp/startup_options.h
index cca38369c33b0a..9f4cd0c622bdcd 100644
--- a/src/main/cpp/startup_options.h
+++ b/src/main/cpp/startup_options.h
@@ -301,6 +301,8 @@ class StartupOptions {
   // The hash function to use when computing file digests.
   std::string digest_function;
 
+  bool idle_server_tasks;
+
   // The startup options as received from the user and rc files, tagged with
   // their origin. This is populated by ProcessArgs.
   std::vector<RcStartupFlag> original_startup_options_;
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java
index a8b1d0ae75d1cf..f3d86fcac1f65d 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeRuntime.java
@@ -925,11 +925,14 @@ private static int serverMain(Iterable<BlazeModule> modules, OutErr outErr, Stri
         Class<?> factoryClass = Class.forName(
             "com.google.devtools.build.lib.server.GrpcServerImpl$Factory");
         RPCServer.Factory factory = (RPCServer.Factory) factoryClass.getConstructor().newInstance();
-        rpcServer[0] = factory.create(dispatcher, runtime.getClock(),
+        rpcServer[0] = factory.create(
+            dispatcher,
+            runtime.getClock(),
             startupOptions.commandPort,
             runtime.getWorkspace().getWorkspace(),
             runtime.getServerDirectory(),
-            startupOptions.maxIdleSeconds);
+            startupOptions.maxIdleSeconds,
+            startupOptions.idleServerTasks);
       } catch (ReflectiveOperationException | IllegalArgumentException e) {
         throw new AbruptExitException("gRPC server not compiled in", ExitCode.BLAZE_INTERNAL_ERROR);
       }
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeServerStartupOptions.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeServerStartupOptions.java
index 6d434f8d568044..6b35fcde85905f 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeServerStartupOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeServerStartupOptions.java
@@ -430,4 +430,16 @@ public String getTypeDescription() {
           "Changed the expansion of --config flags to be done in-place, as opposed to in a fixed "
               + "point expansion between normal rc options and command-line specified options.")
   public boolean expandConfigsInPlace;
+
+  @Option(
+    name = "idle_server_tasks",
+    defaultValue = "true", // NOTE: only for documentation, value is set and used by the client.
+    documentationCategory = OptionDocumentationCategory.BAZEL_CLIENT_OPTIONS,
+    effectTags = {
+      OptionEffectTag.LOSES_INCREMENTAL_STATE,
+      OptionEffectTag.HOST_MACHINE_RESOURCE_OPTIMIZATIONS,
+    },
+    help = "Run System.gc() when the server is idle"
+  )
+  public boolean idleServerTasks;
 }
diff --git a/src/main/java/com/google/devtools/build/lib/server/GrpcServerImpl.java b/src/main/java/com/google/devtools/build/lib/server/GrpcServerImpl.java
index 2d02631e409b81..43f9ab1713156d 100644
--- a/src/main/java/com/google/devtools/build/lib/server/GrpcServerImpl.java
+++ b/src/main/java/com/google/devtools/build/lib/server/GrpcServerImpl.java
@@ -152,10 +152,16 @@ public void close() {
    */
   public static class Factory implements RPCServer.Factory {
     @Override
-    public RPCServer create(BlazeCommandDispatcher dispatcher, Clock clock, int port,
-      Path workspace, Path serverDirectory, int maxIdleSeconds) throws IOException {
+    public RPCServer create(
+        BlazeCommandDispatcher dispatcher,
+        Clock clock,
+        int port,
+        Path workspace,
+        Path serverDirectory,
+        int maxIdleSeconds,
+        boolean idleServerTasks) throws IOException {
       return new GrpcServerImpl(
-          dispatcher, clock, port, workspace, serverDirectory, maxIdleSeconds);
+          dispatcher, clock, port, workspace, serverDirectory, maxIdleSeconds, idleServerTasks);
     }
   }
 
@@ -512,13 +518,20 @@ public void run() {
   private final String pidInFile;
   private final List<Path> filesToDeleteAtExit = new ArrayList<>();
   private final int port;
+  private final boolean doIdleServerTasks;
 
   private Server server;
   private IdleServerTasks idleServerTasks;
   boolean serving;
 
-  public GrpcServerImpl(BlazeCommandDispatcher dispatcher, Clock clock, int port,
-      Path workspace, Path serverDirectory, int maxIdleSeconds) throws IOException {
+  public GrpcServerImpl(
+      BlazeCommandDispatcher dispatcher,
+      Clock clock,
+      int port,
+      Path workspace,
+      Path serverDirectory,
+      int maxIdleSeconds,
+      boolean doIdleServerTasks) throws IOException {
     Runtime.getRuntime().addShutdownHook(new Thread() {
       @Override
       public void run() {
@@ -555,20 +568,24 @@ public void run() {
 
     pidFileWatcherThread = new PidFileWatcherThread();
     pidFileWatcherThread.start();
-    idleServerTasks = new IdleServerTasks();
-    idleServerTasks.idle();
+    this.doIdleServerTasks = doIdleServerTasks;
+    idle();
   }
 
   private void idle() {
     Preconditions.checkState(idleServerTasks == null);
-    idleServerTasks = new IdleServerTasks();
-    idleServerTasks.idle();
+    if (doIdleServerTasks) {
+      idleServerTasks = new IdleServerTasks();
+      idleServerTasks.idle();
+    }
   }
 
   private void busy() {
-    Preconditions.checkState(idleServerTasks != null);
-    idleServerTasks.busy();
-    idleServerTasks = null;
+    if (doIdleServerTasks) {
+      Preconditions.checkState(idleServerTasks != null);
+      idleServerTasks.busy();
+      idleServerTasks = null;
+    }
   }
 
   private static String generateCookie(SecureRandom random, int byteCount) {
diff --git a/src/main/java/com/google/devtools/build/lib/server/IdleServerTasks.java b/src/main/java/com/google/devtools/build/lib/server/IdleServerTasks.java
index 01e596a00c94f9..d8cb41a2b3ff92 100644
--- a/src/main/java/com/google/devtools/build/lib/server/IdleServerTasks.java
+++ b/src/main/java/com/google/devtools/build/lib/server/IdleServerTasks.java
@@ -16,6 +16,10 @@
 
 import com.google.common.base.Preconditions;
 import com.google.devtools.build.lib.profiler.AutoProfiler;
+import com.google.devtools.build.lib.util.StringUtilities;
+import java.lang.management.ManagementFactory;
+import java.lang.management.MemoryMXBean;
+import java.lang.management.MemoryUsage;
 import java.util.concurrent.Future;
 import java.util.concurrent.ScheduledThreadPoolExecutor;
 import java.util.concurrent.TimeUnit;
@@ -29,9 +33,7 @@ class IdleServerTasks {
   private final ScheduledThreadPoolExecutor executor;
   private static final Logger logger = Logger.getLogger(IdleServerTasks.class.getName());
 
-  /**
-   * Must be called from the main thread.
-   */
+  /** Must be called from the main thread. */
   public IdleServerTasks() {
     this.executor = new ScheduledThreadPoolExecutor(1);
   }
@@ -43,14 +45,23 @@ public IdleServerTasks() {
   public void idle() {
     Preconditions.checkState(!executor.isShutdown());
 
-    // Do a GC cycle while the server is idle.
     @SuppressWarnings("unused")
     Future<?> possiblyIgnoredError =
         executor.schedule(
             () -> {
+              MemoryMXBean memBean = ManagementFactory.getMemoryMXBean();
+              MemoryUsage before = memBean.getHeapMemoryUsage();
               try (AutoProfiler p = AutoProfiler.logged("Idle GC", logger)) {
                 System.gc();
               }
+              MemoryUsage after = memBean.getHeapMemoryUsage();
+              logger.info(
+                  String.format(
+                      "[Idle GC] used: %s -> %s, committed: %s -> %s",
+                      StringUtilities.prettyPrintBytes(before.getUsed()),
+                      StringUtilities.prettyPrintBytes(after.getUsed()),
+                      StringUtilities.prettyPrintBytes(before.getCommitted()),
+                      StringUtilities.prettyPrintBytes(after.getCommitted())));
             },
             10,
             TimeUnit.SECONDS);
diff --git a/src/main/java/com/google/devtools/build/lib/server/RPCServer.java b/src/main/java/com/google/devtools/build/lib/server/RPCServer.java
index 014b17b5ae423a..45833b2e3574f1 100644
--- a/src/main/java/com/google/devtools/build/lib/server/RPCServer.java
+++ b/src/main/java/com/google/devtools/build/lib/server/RPCServer.java
@@ -28,8 +28,14 @@ public interface RPCServer {
    * Present so that we don't need to invoke a constructor with multiple arguments by reflection.
    */
   interface Factory {
-    RPCServer create(BlazeCommandDispatcher dispatcher, Clock clock, int port,
-        Path workspace, Path serverDirectory, int maxIdleSeconds) throws IOException;
+    RPCServer create(
+        BlazeCommandDispatcher dispatcher,
+        Clock clock,
+        int port,
+        Path workspace,
+        Path serverDirectory,
+        int maxIdleSeconds,
+        boolean idleServerTasks) throws IOException;
   }
 
   /**


Test patch: