Make The Build Faster: Use AutoCloseable when dealing with resources.

--
MOS_MIGRATED_REVID=114204404

4 files changed, 53 insertions, 34 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/ResourceManager.java b/src/main/java/com/google/devtools/build/lib/actions/ResourceManager.java
index e87e87e697..c263b2d05f 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ResourceManager.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ResourceManager.java
@@ -60,6 +60,31 @@ import java.util.concurrent.CountDownLatch;
 @ThreadSafe
 public class ResourceManager {
 
+  /**
+   * A handle returned by {@link #acquireResources(ActionMetadata, ResourceSet)} that must be closed
+   * in order to free the resources again.
+   */
+  public static class ResourceHandle implements AutoCloseable {
+    final ResourceManager rm;
+    final ActionMetadata actionMetadata;
+    final ResourceSet resourceSet;
+
+    public ResourceHandle(
+        ResourceManager rm, ActionMetadata actionMetadata, ResourceSet resources) {
+      this.rm = rm;
+      this.actionMetadata = actionMetadata;
+      this.resourceSet = resources;
+    }
+
+    /**
+     * Closing the ResourceHandle releases the resources associated with it.
+     */
+    @Override
+    public void close() {
+      rm.releaseResources(actionMetadata, resourceSet);
+    }
+  }
+
   private EventBus eventBus;
 
   private final ThreadLocal<Boolean> threadLocked = new ThreadLocal<Boolean>() {
@@ -135,8 +160,7 @@ public class ResourceManager {
 
   /**
    * Resets resource manager state and releases all thread locks.
-   * Note - it does not reset available resources. Use
-   * separate call to setAvailableResoures().
+   * Note - it does not reset available resources. Use separate call to setAvailableResources().
    */
   public synchronized void resetResourceUsage() {
     usedCpu = 0;
@@ -167,7 +191,6 @@ public class ResourceManager {
 
   /**
    * Specify how much of the available RAM we should allow to be used.
-   * This has no effect if autosensing is enabled.
    */
   public synchronized void setRamUtilizationPercentage(int percentage) {
     ramUtilizationPercentage = percentage;
@@ -177,7 +200,7 @@ public class ResourceManager {
    * Acquires requested resource set. Will block if resource is not available.
    * NB! This method must be thread-safe!
    */
-  public void acquireResources(ActionMetadata owner, ResourceSet resources)
+  public ResourceHandle acquireResources(ActionMetadata owner, ResourceSet resources)
       throws InterruptedException {
     Preconditions.checkNotNull(resources);
     AutoProfiler p = profiled(owner, ProfilerTask.ACTION_LOCK);
@@ -198,6 +221,7 @@ public class ResourceManager {
         p.complete();
       }
     }
+    return new ResourceHandle(this, owner, resources);
   }
 
   /**
diff --git a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java
index 9534f5c591..ed87aea887 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/SymlinkTreeHelper.java
@@ -21,6 +21,7 @@ import com.google.devtools.build.lib.actions.ActionExecutionContext;
 import com.google.devtools.build.lib.actions.BaseSpawn;
 import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.ResourceManager;
+import com.google.devtools.build.lib.actions.ResourceManager.ResourceHandle;
 import com.google.devtools.build.lib.actions.ResourceSet;
 import com.google.devtools.build.lib.analysis.config.BinTools;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
@@ -110,13 +111,11 @@ public final class SymlinkTreeHelper {
       BinTools binTools) throws ExecException, InterruptedException {
     List<String> args = getSpawnArgumentList(
         actionExecutionContext.getExecutor().getExecRoot(), binTools);
-    try {
-      ResourceManager.instance().acquireResources(action, RESOURCE_SET);
+    try (ResourceHandle handle =
+            ResourceManager.instance().acquireResources(action, RESOURCE_SET)) {
       actionExecutionContext.getExecutor().getSpawnActionContext(action.getMnemonic()).exec(
           new BaseSpawn.Local(args, ImmutableMap.<String, String>of(), action),
           actionExecutionContext);
-    } finally {
-      ResourceManager.instance().releaseResources(action, RESOURCE_SET);
     }
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/test/StandaloneTestStrategy.java b/src/main/java/com/google/devtools/build/lib/rules/test/StandaloneTestStrategy.java
index 1cddbf6791..11b77f564c 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/test/StandaloneTestStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/test/StandaloneTestStrategy.java
@@ -23,6 +23,7 @@ import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.ExecutionStrategy;
 import com.google.devtools.build.lib.actions.Executor;
 import com.google.devtools.build.lib.actions.ResourceManager;
+import com.google.devtools.build.lib.actions.ResourceManager.ResourceHandle;
 import com.google.devtools.build.lib.actions.ResourceSet;
 import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.TestExecException;
@@ -116,34 +117,27 @@ public class StandaloneTestStrategy extends TestStrategy {
       throw new EnvironmentalExecException("Could not create TEST_TMPDIR " + testTmpDir, e);
     }
 
-    ResourceSet resources = null;
-    FileOutErr fileOutErr = null;
     try {
       FileSystemUtils.createDirectoryAndParents(workingDirectory);
-      fileOutErr = new FileOutErr(action.getTestLog().getPath(),
-          action.resolve(actionExecutionContext.getExecutor().getExecRoot()).getTestStderr());
-
-      resources = action.getTestProperties()
-          .getLocalResourceUsage(executionOptions.usingLocalTestJobs());
-      ResourceManager.instance().acquireResources(action, resources);
-      TestResultData data = execute(
-          actionExecutionContext.withFileOutErr(fileOutErr), spawn, action);
-      appendStderr(fileOutErr.getOutputFile(), fileOutErr.getErrorFile());
-      finalizeTest(actionExecutionContext, action, data);
+
+      ResourceSet resources =
+          action.getTestProperties().getLocalResourceUsage(executionOptions.usingLocalTestJobs());
+
+      try (FileOutErr fileOutErr =
+              new FileOutErr(
+                  action.getTestLog().getPath(),
+                  action
+                      .resolve(actionExecutionContext.getExecutor().getExecRoot())
+                      .getTestStderr());
+          ResourceHandle handle = ResourceManager.instance().acquireResources(action, resources)) {
+        TestResultData data =
+            execute(actionExecutionContext.withFileOutErr(fileOutErr), spawn, action);
+        appendStderr(fileOutErr.getOutputFile(), fileOutErr.getErrorFile());
+        finalizeTest(actionExecutionContext, action, data);
+      }
     } catch (IOException e) {
       executor.getEventHandler().handle(Event.error("Caught I/O exception: " + e));
       throw new EnvironmentalExecException("unexpected I/O exception", e);
-    } finally {
-      if (resources != null) {
-        ResourceManager.instance().releaseResources(action, resources);
-      }
-      try {
-        if (fileOutErr != null) {
-          fileOutErr.close();
-        }
-      } catch (IOException e) {
-        // If the close fails, there is little we can do.
-      }
     }
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
index 27310f0a4a..39adae3b33 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeActionExecutor.java
@@ -51,6 +51,7 @@ import com.google.devtools.build.lib.actions.NotifyOnActionCacheHit;
 import com.google.devtools.build.lib.actions.PackageRootResolutionException;
 import com.google.devtools.build.lib.actions.PackageRootResolver;
 import com.google.devtools.build.lib.actions.ResourceManager;
+import com.google.devtools.build.lib.actions.ResourceManager.ResourceHandle;
 import com.google.devtools.build.lib.actions.ResourceSet;
 import com.google.devtools.build.lib.actions.TargetOutOfDateException;
 import com.google.devtools.build.lib.actions.cache.MetadataHandler;
@@ -706,20 +707,21 @@ public final class SkyframeActionExecutor implements ActionExecutionContextFacto
     postEvent(new ActionStartedEvent(action, actionStartTime));
     ResourceSet estimate = action.estimateResourceConsumption(executorEngine);
     ActionExecutionStatusReporter statusReporter = statusReporterRef.get();
+    ResourceHandle handle = null;
     try {
       if (estimate == null || estimate == ResourceSet.ZERO) {
         statusReporter.setRunningFromBuildData(action);
       } else {
         // If estimated resource consumption is null, action will manually call
         // resource manager when it knows what resources are needed.
-        resourceManager.acquireResources(action, estimate);
+        handle = resourceManager.acquireResources(action, estimate);
       }
       boolean outputDumped = executeActionTask(action, context);
       completeAction(action, context.getMetadataHandler(),
           context.getFileOutErr(), outputDumped);
     } finally {
-      if (estimate != null) {
-        resourceManager.releaseResources(action, estimate);
+      if (handle != null) {
+        handle.close();
       }
       statusReporter.remove(action);
       postEvent(new ActionCompletionEvent(actionStartTime, action));