Skip to content

Commit

Permalink
Move deleteTree and deleteTreesBelow into FileSystem and Path.
Browse files Browse the repository at this point in the history 
    The current implementation of these functions is very inefficient and
    degrades overall performance significantly, especially when sandboxing is
    enabled. However, that's almost the best we can do with a generic
    algorithm.

    To make room for optimizations that rely on specific file system features,
    move these functions into the FileSystem class. I will supply a custom
    implementation for UnixFileSystem later.

    Note that this is intended to be a pure code move. I haven't applied any
    improvements to the code nor tests yet (with the exception of cleaning up
    docstrings).

    Addresses bazelbuild/bazel#7527.

    RELNOTES: None.
    PiperOrigin-RevId: 239412965
  • Loading branch information
Luca Di Grazia committed Sep 4, 2022
1 parent 5d6c4ec commit 074bdf0
Show file tree
Hide file tree
Showing 35 changed files with 870 additions and 1,337 deletions.
13 changes: 7 additions & 6 deletions .../bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import com.google.devtools.build.lib.syntax.SkylarkList;
import com.google.devtools.build.lib.syntax.SkylarkNestedSet;
import com.google.devtools.build.lib.util.Fingerprint;
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.Symlinks;
Expand Down Expand Up @@ -379,17 +380,17 @@ public void repr(SkylarkPrinter printer) {
*
* @param execRoot the exec root in which this action is executed
*/
protected void deleteOutputs(Path execRoot) throws IOException {
protected void deleteOutputs(FileSystem fileSystem, Path execRoot) throws IOException {
for (Artifact output : getOutputs()) {
deleteOutput(output);
deleteOutput(fileSystem, output);
}
}

/**
* Helper method to remove an Artifact. If the Artifact refers to a directory recursively removes
* the contents of the directory.
*/
protected void deleteOutput(Artifact output) throws IOException {
protected void deleteOutput(FileSystem fileSystem, Artifact output) throws IOException {
Path path = output.getPath();
try {
// Optimize for the common case: output artifacts are files.
Expand All @@ -409,7 +410,7 @@ protected void deleteOutput(Artifact output) throws IOException {
if (!parentDir.isWritable() && outputRoot.contains(parentDir)) {
// Retry deleting after making the parent writable.
parentDir.setWritable(true);
deleteOutput(output);
deleteOutput(fileSystem, output);
} else if (path.isDirectory(Symlinks.NOFOLLOW)) {
path.deleteTree();
} else {
Expand Down Expand Up @@ -468,8 +469,8 @@ protected void checkOutputsForDirectories(ActionExecutionContext actionExecution
}

@Override
public void prepare(Path execRoot) throws IOException {
deleteOutputs(execRoot);
public void prepare(FileSystem fileSystem, Path execRoot) throws IOException {
deleteOutputs(fileSystem, execRoot);
}

@Override
Expand Down
81 changes: 68 additions & 13 deletions ...ild.bazel/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -754,13 +754,6 @@ public ActionContinuationOrResult beginExecution(ActionExecutionContext actionEx
throws InterruptedException, ActionExecutionException {
TestActionContext testActionContext =
actionExecutionContext.getContext(TestActionContext.class);
return beginExecution(actionExecutionContext, testActionContext);
}

@VisibleForTesting
public ActionContinuationOrResult beginExecution(
ActionExecutionContext actionExecutionContext, TestActionContext testActionContext)
throws InterruptedException, ActionExecutionException {
try {
TestRunnerSpawn testRunnerSpawn =
testActionContext.createTestRunnerSpawn(this, actionExecutionContext);
Expand Down Expand Up @@ -799,17 +792,79 @@ public ActionResult execute(
ActionExecutionContext actionExecutionContext, TestActionContext testActionContext)
throws ActionExecutionException, InterruptedException {
try {
ActionContinuationOrResult continuation =
beginExecution(actionExecutionContext, testActionContext);
while (!continuation.isDone()) {
continuation = continuation.execute();
}
return continuation.get();
TestRunnerSpawn testRunnerSpawn =
testActionContext.createTestRunnerSpawn(this, actionExecutionContext);
return ActionResult.create(
runAttempts(
testRunnerSpawn, actionExecutionContext, testActionContext.isTestKeepGoing()));
} catch (ExecException e) {
throw e.toActionExecutionException(this);
} finally {
unconditionalExecution = null;
}
}

private List<SpawnResult> runAttempts(
TestRunnerSpawn testRunnerSpawn,
ActionExecutionContext actionExecutionContext,
boolean keepGoing)
throws ExecException, InterruptedException {
List<SpawnResult> spawnResults = new ArrayList<>();
runAttempts(
testRunnerSpawn, actionExecutionContext, keepGoing, spawnResults, new ArrayList<>());
return spawnResults;
}

private void runAttempts(
TestRunnerSpawn testRunnerSpawn,
ActionExecutionContext actionExecutionContext,
boolean keepGoing,
List<SpawnResult> spawnResults,
List<FailedAttemptResult> failedAttempts)
throws ExecException, InterruptedException {
try {
TestAttemptResult testProcessResult = testRunnerSpawn.execute();
spawnResults.addAll(testProcessResult.spawnResults());
int maxAttempts = testRunnerSpawn.getMaxAttempts(testProcessResult);
// Failed test retry loop.
for (int attempt = 1; attempt < maxAttempts && !testProcessResult.hasPassed(); attempt++) {
failedAttempts.add(
testRunnerSpawn.finalizeFailedTestAttempt(
testProcessResult, failedAttempts.size() + 1));

testProcessResult = testRunnerSpawn.execute();
spawnResults.addAll(testProcessResult.spawnResults());
}

TestRunnerSpawn fallbackRunner;
if (!testProcessResult.hasPassed()
&& (fallbackRunner = testRunnerSpawn.getFallbackRunner()) != null) {
// All attempts failed and fallback is enabled.
failedAttempts.add(
testRunnerSpawn.finalizeFailedTestAttempt(
testProcessResult, failedAttempts.size() + 1));
runAttempts(
fallbackRunner, actionExecutionContext, keepGoing, spawnResults, failedAttempts);
} else {
testRunnerSpawn.finalizeTest(testProcessResult, failedAttempts);

if (!keepGoing && !testProcessResult.hasPassed()) {
throw new TestExecException("Test failed: aborting");
}
}
} catch (IOException e) {
// Print the stack trace, otherwise the unexpected I/O error is hard to diagnose.
// A stack trace could help with bugs like https://github.com/bazelbuild/bazel/issues/4924
StringBuilder sb = new StringBuilder();
sb.append("Caught I/O exception: ").append(e.getMessage());
for (Object s : e.getStackTrace()) {
sb.append("\n\t").append(s);
}
actionExecutionContext.getEventHandler().handle(Event.error(sb.toString()));
throw new EnvironmentalExecException("unexpected I/O exception", e);
}
}

@Override
public String getMnemonic() {
return MNEMONIC;
Expand Down
75 changes: 19 additions & 56 deletions ...ava/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryFunction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,8 @@

package com.google.devtools.build.lib.bazel.repository.skylark;

import com.google.common.base.Preconditions;
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
import com.google.devtools.build.lib.analysis.RuleDefinition;
import com.google.devtools.build.lib.analysis.skylark.BazelStarlarkContext;
Expand All @@ -28,19 +25,16 @@
import com.google.devtools.build.lib.cmdline.LabelConstants;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.pkgcache.PathPackageLocator;
import com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction;
import com.google.devtools.build.lib.rules.repository.RepositoryDirectoryValue;
import com.google.devtools.build.lib.rules.repository.RepositoryFunction;
import com.google.devtools.build.lib.rules.repository.ResolvedHashesValue;
import com.google.devtools.build.lib.skyframe.BlacklistedPackagePrefixesValue;
import com.google.devtools.build.lib.skyframe.PrecomputedValue;
import com.google.devtools.build.lib.syntax.BaseFunction;
import com.google.devtools.build.lib.syntax.EvalException;
import com.google.devtools.build.lib.syntax.Mutability;
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.skyframe.SkyFunction.Environment;
import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
import com.google.devtools.build.skyframe.SkyKey;
Expand Down Expand Up @@ -75,46 +69,26 @@ public RepositoryDirectoryValue.Builder fetch(
Map<String, String> markerData,
SkyKey key)
throws RepositoryFunctionException, InterruptedException {
if (rule.getDefinitionInformation() != null) {
env.getListener()
.post(
new SkylarkRepositoryDefinitionLocationEvent(
rule.getName(), rule.getDefinitionInformation()));
}
BaseFunction function = rule.getRuleClassObject().getConfiguredTargetFunction();
if (declareEnvironmentDependencies(markerData, env, getEnviron(rule)) == null) {
return null;
}
StarlarkSemantics starlarkSemantics = PrecomputedValue.STARLARK_SEMANTICS.get(env);
if (env.valuesMissing()) {
if (starlarkSemantics == null) {
return null;
}

Set<String> verificationRules =
RepositoryDelegatorFunction.OUTPUT_VERIFICATION_REPOSITORY_RULES.get(env);
if (env.valuesMissing()) {
if (verificationRules == null) {
return null;
}
ResolvedHashesValue resolvedHashesValue =
(ResolvedHashesValue) env.getValue(ResolvedHashesValue.key());
if (env.valuesMissing()) {
return null;
}
Map<String, String> resolvedHashes =
Preconditions.checkNotNull(resolvedHashesValue).getHashes();

PathPackageLocator packageLocator = PrecomputedValue.PATH_PACKAGE_LOCATOR.get(env);
if (env.valuesMissing()) {
if (resolvedHashesValue == null) {
return null;
}

BlacklistedPackagePrefixesValue blacklistedPackagesValue =
(BlacklistedPackagePrefixesValue) env.getValue(BlacklistedPackagePrefixesValue.key());
if (env.valuesMissing()) {
return null;
}
ImmutableSet<PathFragment> blacklistedPatterns =
Preconditions.checkNotNull(blacklistedPackagesValue).getPatterns();
Map<String, String> resolvedHashes = resolvedHashesValue.getHashes();

try (Mutability mutability = Mutability.create("Starlark repository")) {
com.google.devtools.build.lib.syntax.Environment buildEnv =
Expand All @@ -133,9 +107,7 @@ public RepositoryDirectoryValue.Builder fetch(
SkylarkRepositoryContext skylarkRepositoryContext =
new SkylarkRepositoryContext(
rule,
packageLocator,
outputDirectory,
blacklistedPatterns,
env,
clientEnvironment,
httpDownloader,
Expand All @@ -146,11 +118,12 @@ public RepositoryDirectoryValue.Builder fetch(
// all label-arguments can be resolved to paths.
try {
skylarkRepositoryContext.enforceLabelAttributes();
} catch (RepositoryMissingDependencyException e) {
// Missing values are expected; just restart before we actually start the rule
return null;
} catch (EvalException e) {
// EvalExceptions indicate labels not referring to existing files. This is fine,
if (e instanceof RepositoryMissingDependencyException) {
// Missing values are expected; just restart before we actually start the rule
return null;
}
// Other EvalExceptions indicate labels not referring to existing files. This is fine,
// as long as they are never resolved to files in the execution of the rule; we allow
// non-strict rules. So now we have to start evaluating the actual rule, even if that
// means the rule might get restarted for legitimate reasons.
Expand All @@ -174,7 +147,6 @@ public RepositoryDirectoryValue.Builder fetch(
rule, skylarkRepositoryContext.getAttr(), outputDirectory, retValue);
if (resolved.isNewInformationReturned()) {
env.getListener().handle(Event.debug(resolved.getMessage()));
env.getListener().handle(Event.debug(rule.getDefinitionInformation()));
}

String ruleClass =
Expand All @@ -192,26 +164,17 @@ public RepositoryDirectoryValue.Builder fetch(
}
}
env.getListener().post(resolved);
} catch (RepositoryMissingDependencyException e) {
// A dependency is missing, cleanup and returns null
try {
if (outputDirectory.exists()) {
outputDirectory.deleteTree();
}
} catch (IOException e1) {
throw new RepositoryFunctionException(e1, Transience.TRANSIENT);
}
return null;
} catch (EvalException e) {
env.getListener()
.handle(
Event.error(
"An error occurred during the fetch of repository '"
+ rule.getName()
+ "':\n "
+ e.getMessage()));
if (!Strings.isNullOrEmpty(rule.getDefinitionInformation())) {
env.getListener().handle(Event.info(rule.getDefinitionInformation()));
if (e.getCause() instanceof RepositoryMissingDependencyException) {
// A dependency is missing, cleanup and returns null
try {
if (outputDirectory.exists()) {
outputDirectory.deleteTree();
}
} catch (IOException e1) {
throw new RepositoryFunctionException(e1, Transience.TRANSIENT);
}
return null;
}
throw new RepositoryFunctionException(e, Transience.TRANSIENT);
}
Expand Down
8 changes: 1 addition & 7 deletions ...bazelbuild.bazel/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -171,12 +171,6 @@ public class ExecutionTool {
actionContextProviders,
options.testStrategy,
options.incompatibleListBasedExecutionStrategySelection);

if (options.availableResources != null && options.removeLocalResources) {
throw new ExecutorInitException(
"--local_resources is deprecated. Please use "
+ "--local_ram_resources and/or --local_cpu_resources");
}
}

Executor getExecutor() throws ExecutorInitException {
Expand Down Expand Up @@ -647,7 +641,7 @@ public static void configureResourceManager(BuildRequest request) {
ResourceManager resourceMgr = ResourceManager.instance();
ExecutionOptions options = request.getOptions(ExecutionOptions.class);
ResourceSet resources;
if (options.availableResources != null && !options.removeLocalResources) {
if (options.availableResources != null) {
logger.warning(
"--local_resources will be deprecated. Please use --local_ram_resources "
+ "and/or --local_cpu_resources.");
Expand Down
Loading

0 comments on commit 074bdf0

Please sign in to comment.