diff options
author | 2015-11-04 17:52:32 +0000 | |
---|---|---|
committer | 2015-11-05 16:49:28 +0000 | |
commit | af33c67de5015fdfbb92d3ca6d5c99508540cb01 (patch) | |
tree | ad7bba7397ff5030b8e8ef365b55a7c77ba7f283 /src/main/java/com/google/devtools/build | |
parent | 53fb4d0ef10dea9d58f4b17f08c59c8e81ebc09a (diff) |
workers: Restart worker processes when their binary has changed since they were launched.
--
MOS_MIGRATED_REVID=107050157
Diffstat (limited to 'src/main/java/com/google/devtools/build')
15 files changed, 349 insertions, 104 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java index 49c7c15d3d..3a2bddcbff 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java +++ b/src/main/java/com/google/devtools/build/lib/actions/AbstractAction.java @@ -54,11 +54,32 @@ public abstract class AbstractAction implements Action, SkylarkValue { public static final ResourceSet DEFAULT_RESOURCE_SET = ResourceSet.createWithRamCpuIo(250, 0.5, 0); - // owner/inputs/outputs attributes below should never be directly accessed even - // within AbstractAction itself. The appropriate getter methods should be used - // instead. This has to be done due to the fact that the getter methods can be - // overridden in subclasses. + /** + * The owner/inputs/outputs attributes below should never be directly accessed even within + * AbstractAction itself. The appropriate getter methods should be used instead. This has to be + * done due to the fact that the getter methods can be overridden in subclasses. + */ private final ActionOwner owner; + + /** + * Tools are a subset of inputs and used by the WorkerSpawnStrategy to determine whether a + * compiler has changed since the last time it was used. This should include all artifacts that + * the tool does not dynamically reload / check on each unit of work - e.g. its own binary, the + * JDK for Java binaries, shared libraries, ... but not a configuration file, if it reloads that + * when it has changed. + * + * <p>If the "tools" set does not contain exactly the right set of artifacts, the following can + * happen: If an artifact that should be included is missing, the tool might not be restarted when + * it should, and builds can become incorrect (example: The compiler binary is not part of this + * set, then the compiler gets upgraded, but the worker strategy still reuses the old version). + * If an artifact that should *not* be included is accidentally part of this set, the worker + * process will be restarted more often that is necessary - e.g. if a file that is unique to each + * unit of work, e.g. the source code that a compiler should compile for a compile action, is + * part of this set, then the worker will never be reused and will be restarted for each unit of + * work. + */ + private final Iterable<Artifact> tools; + // The variable inputs is non-final only so that actions that discover their inputs can modify it. private Iterable<Artifact> inputs; private final RunfilesSupplier runfilesSupplier; @@ -72,16 +93,38 @@ public abstract class AbstractAction implements Action, SkylarkValue { protected AbstractAction(ActionOwner owner, Iterable<Artifact> inputs, Iterable<Artifact> outputs) { - this(owner, inputs, EmptyRunfilesSupplier.INSTANCE, outputs); + this(owner, ImmutableList.<Artifact>of(), inputs, EmptyRunfilesSupplier.INSTANCE, outputs); } - protected AbstractAction(ActionOwner owner, + /** + * Construct an abstract action with the specified tools, inputs and outputs; + */ + protected AbstractAction( + ActionOwner owner, + Iterable<Artifact> tools, + Iterable<Artifact> inputs, + Iterable<Artifact> outputs) { + this(owner, tools, inputs, EmptyRunfilesSupplier.INSTANCE, outputs); + } + + protected AbstractAction( + ActionOwner owner, + Iterable<Artifact> inputs, + RunfilesSupplier runfilesSupplier, + Iterable<Artifact> outputs) { + this(owner, ImmutableList.<Artifact>of(), inputs, runfilesSupplier, outputs); + } + + protected AbstractAction( + ActionOwner owner, + Iterable<Artifact> tools, Iterable<Artifact> inputs, RunfilesSupplier runfilesSupplier, Iterable<Artifact> outputs) { Preconditions.checkNotNull(owner); // TODO(bazel-team): Use RuleContext.actionOwner here instead this.owner = new ActionOwnerDescription(owner); + this.tools = CollectionUtils.makeImmutable(tools); this.inputs = CollectionUtils.makeImmutable(inputs); this.outputs = ImmutableSet.copyOf(outputs); this.runfilesSupplier = Preconditions.checkNotNull(runfilesSupplier, @@ -126,6 +169,11 @@ public abstract class AbstractAction implements Action, SkylarkValue { "Method must be overridden for actions that may have unknown inputs."); } + @Override + public Iterable<Artifact> getTools() { + return tools; + } + /** * Should only be overridden by actions that need to optionally insert inputs. Actions that * discover their inputs should use {@link #setInputs} to set the new iterable of inputs when they diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionMetadata.java b/src/main/java/com/google/devtools/build/lib/actions/ActionMetadata.java index 408b717125..c8c82c86ca 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/ActionMetadata.java +++ b/src/main/java/com/google/devtools/build/lib/actions/ActionMetadata.java @@ -95,6 +95,19 @@ public interface ActionMetadata { boolean discoversInputs(); /** + * Returns the tool Artifacts that this Action depends upon. May be empty. This is a subset of + * getInputs(). + * + * <p>This may be used by spawn strategies to determine whether an external tool has not changed + * since the last time it was used and could thus be reused, or whether it has to be restarted. + * + * <p>See {@link AbstractAction#getTools()} for an explanation of why it's important that this + * set contains exactly the right set of artifacts in order for the build to stay correct and the + * worker strategy to work. + */ + Iterable<Artifact> getTools(); + + /** * Returns the input Artifacts that this Action depends upon. May be empty. * * <p>During execution, the {@link Iterable} returned by {@code getInputs} <em>must not</em> be diff --git a/src/main/java/com/google/devtools/build/lib/actions/BaseSpawn.java b/src/main/java/com/google/devtools/build/lib/actions/BaseSpawn.java index 511295c522..0a0955538e 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/BaseSpawn.java +++ b/src/main/java/com/google/devtools/build/lib/actions/BaseSpawn.java @@ -195,6 +195,11 @@ public class BaseSpawn implements Spawn { } @Override + public Iterable<? extends ActionInput> getToolFiles() { + return action.getTools(); + } + + @Override public Iterable<? extends ActionInput> getInputFiles() { return action.getInputs(); } diff --git a/src/main/java/com/google/devtools/build/lib/actions/DelegateSpawn.java b/src/main/java/com/google/devtools/build/lib/actions/DelegateSpawn.java index 678b05c75c..2c0a0ad5ca 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/DelegateSpawn.java +++ b/src/main/java/com/google/devtools/build/lib/actions/DelegateSpawn.java @@ -80,6 +80,11 @@ public class DelegateSpawn implements Spawn { } @Override + public Iterable<? extends ActionInput> getToolFiles() { + return spawn.getToolFiles(); + } + + @Override public Iterable<? extends ActionInput> getInputFiles() { return spawn.getInputFiles(); } diff --git a/src/main/java/com/google/devtools/build/lib/actions/Spawn.java b/src/main/java/com/google/devtools/build/lib/actions/Spawn.java index e4c9399913..d027f70570 100644 --- a/src/main/java/com/google/devtools/build/lib/actions/Spawn.java +++ b/src/main/java/com/google/devtools/build/lib/actions/Spawn.java @@ -84,6 +84,21 @@ public interface Spawn { ImmutableMap<String, String> getEnvironment(); /** + * Returns the list of files that are required to execute this spawn (e.g. the compiler binary), + * in contrast to files necessary for the tool to do its work (e.g. source code to be compiled). + * + * <p>The returned set of files is a subset of what getInputFiles() returns. + * + * <p>This method explicitly does not expand middleman artifacts. Pass the result + * to an appropriate utility method on {@link com.google.devtools.build.lib.actions.Artifact} to + * expand the middlemen. + * + * <p>This is for use with persistent workers, so we can restart workers when their binaries + * have changed. + */ + Iterable<? extends ActionInput> getToolFiles(); + + /** * Returns the list of files that this command may read. * * <p>This method explicitly does not expand middleman artifacts. Pass the result diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java index 10c5a8a9e4..895c518c63 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnAction.java @@ -103,6 +103,7 @@ public class SpawnAction extends AbstractAction { * All collections provided must not be subsequently modified. * * @param owner the owner of the Action. + * @param tools the set of files comprising the tool that does the work (e.g. compiler). * @param inputs the set of all files potentially read by this action; must * not be subsequently modified. * @param outputs the set of all files written by this action; must not be @@ -116,17 +117,30 @@ public class SpawnAction extends AbstractAction { * @param progressMessage the message printed during the progression of the build * @param mnemonic the mnemonic that is reported in the master log. */ - public SpawnAction(ActionOwner owner, - Iterable<Artifact> inputs, Iterable<Artifact> outputs, + public SpawnAction( + ActionOwner owner, + Iterable<Artifact> tools, + Iterable<Artifact> inputs, + Iterable<Artifact> outputs, ResourceSet resourceSet, CommandLine argv, Map<String, String> environment, String progressMessage, String mnemonic) { - this(owner, inputs, outputs, - resourceSet, argv, ImmutableMap.copyOf(environment), - ImmutableMap.<String, String>of(), progressMessage, - ImmutableMap.<PathFragment, Artifact>of(), mnemonic, false, null); + this( + owner, + tools, + inputs, + outputs, + resourceSet, + argv, + ImmutableMap.copyOf(environment), + ImmutableMap.<String, String>of(), + progressMessage, + ImmutableMap.<PathFragment, Artifact>of(), + mnemonic, + false, + null); } /** @@ -135,25 +149,28 @@ public class SpawnAction extends AbstractAction { * <p>All collections provided must not be subsequently modified. * * @param owner the owner of the Action. - * @param inputs the set of all files potentially read by this action; must - * not be subsequently modified. - * @param outputs the set of all files written by this action; must not be - * subsequently modified. + * @param tools the set of files comprising the tool that does the work (e.g. compiler). This is + * a subset of "inputs" and is only used by the WorkerSpawnStrategy. + * @param inputs the set of all files potentially read by this action; must not be subsequently + * modified. + * @param outputs the set of all files written by this action; must not be subsequently modified. * @param resourceSet the resources consumed by executing this Action * @param environment the map of environment variables. * @param executionInfo out-of-band information for scheduling the spawn. - * @param argv the argv array (including argv[0]) of arguments to pass. This - * is merely a list of options to the executable, and is uninterpreted - * by the build tool for the purposes of dependency checking; typically - * it may include the names of input and output files, but this is not - * necessary. + * @param argv the argv array (including argv[0]) of arguments to pass. This is merely a list of + * options to the executable, and is uninterpreted by the build tool for the purposes of + * dependency checking; typically it may include the names of input and output files, but + * this is not necessary. * @param progressMessage the message printed during the progression of the build * @param inputManifests entries in inputs that are symlink manifest files. * These are passed to remote execution in the environment rather than as inputs. * @param mnemonic the mnemonic that is reported in the master log. */ - public SpawnAction(ActionOwner owner, - Iterable<Artifact> inputs, Iterable<Artifact> outputs, + public SpawnAction( + ActionOwner owner, + Iterable<Artifact> tools, + Iterable<Artifact> inputs, + Iterable<Artifact> outputs, ResourceSet resourceSet, CommandLine argv, ImmutableMap<String, String> environment, @@ -163,7 +180,7 @@ public class SpawnAction extends AbstractAction { String mnemonic, boolean executeUnconditionally, ExtraActionInfoSupplier<?> extraActionInfoSupplier) { - super(owner, inputs, outputs); + super(owner, tools, inputs, outputs); this.resourceSet = resourceSet; this.executionInfo = executionInfo; this.environment = environment; @@ -276,6 +293,9 @@ public class SpawnAction extends AbstractAction { f.addString(GUID); f.addStrings(argv.arguments()); f.addString(getMnemonic()); + // We don't need the toolManifests here, because they are a subset of the inputManifests by + // definition and the output of an action shouldn't change whether something is considered a + // tool or not. f.addInt(inputManifests.size()); for (Map.Entry<PathFragment, Artifact> input : inputManifests.entrySet()) { f.addString(input.getKey().getPathString() + "/"); @@ -413,7 +433,6 @@ public class SpawnAction extends AbstractAction { inputs.removeAll(filesets); inputs.removeAll(inputManifests.values()); return inputs; - // Also expand middleman artifacts. } } @@ -422,9 +441,11 @@ public class SpawnAction extends AbstractAction { */ public static class Builder { + private final NestedSetBuilder<Artifact> toolsBuilder = NestedSetBuilder.stableOrder(); private final NestedSetBuilder<Artifact> inputsBuilder = NestedSetBuilder.stableOrder(); private final List<Artifact> outputs = new ArrayList<>(); + private final Map<PathFragment, Artifact> toolManifests = new LinkedHashMap<>(); private final Map<PathFragment, Artifact> inputManifests = new LinkedHashMap<>(); private ResourceSet resourceSet = AbstractAction.DEFAULT_RESOURCE_SET; private ImmutableMap<String, String> environment = ImmutableMap.of(); @@ -452,8 +473,10 @@ public class SpawnAction extends AbstractAction { * Creates a builder that is a copy of another builder. */ public Builder(Builder other) { + this.toolsBuilder.addTransitive(other.toolsBuilder.build()); this.inputsBuilder.addTransitive(other.inputsBuilder.build()); this.outputs.addAll(other.outputs); + this.toolManifests.putAll(other.toolManifests); this.inputManifests.putAll(other.inputManifests); this.resourceSet = other.resourceSet; this.environment = other.environment; @@ -521,25 +544,53 @@ public class SpawnAction extends AbstractAction { if (paramsFile != null) { actualCommandLine = ParamFileHelper.createWithParamsFile(argv, arguments, commandLine, isShellCommand, owner, actions, paramFileInfo, paramsFile); + inputsBuilder.add(paramsFile); } else { actualCommandLine = ParamFileHelper.createWithoutParamsFile(argv, arguments, commandLine, isShellCommand); } - Iterable<Artifact> actualInputs = collectActualInputs(paramsFile); - - actions.add(0, new SpawnAction(owner, actualInputs, ImmutableList.copyOf(outputs), - resourceSet, actualCommandLine, environment, executionInfo, progressMessage, - ImmutableMap.copyOf(inputManifests), mnemonic, executeUnconditionally, - extraActionInfoSupplier)); + NestedSet<Artifact> tools = toolsBuilder.build(); + + // Tools are by definition a subset of the inputs, so make sure they're present there, too. + NestedSet<Artifact> inputsAndTools = + NestedSetBuilder.<Artifact>stableOrder() + .addTransitive(inputsBuilder.build()) + .addTransitive(tools) + .build(); + + LinkedHashMap<PathFragment, Artifact> inputAndToolManifests = + new LinkedHashMap<>(inputManifests); + inputAndToolManifests.putAll(toolManifests); + + actions.add( + 0, + new SpawnAction( + owner, + tools, + inputsAndTools, + ImmutableList.copyOf(outputs), + resourceSet, + actualCommandLine, + environment, + executionInfo, + progressMessage, + ImmutableMap.copyOf(inputAndToolManifests), + mnemonic, + executeUnconditionally, + extraActionInfoSupplier)); return actions.toArray(new Action[actions.size()]); } - private Iterable<Artifact> collectActualInputs(Artifact parameterFile) { - if (parameterFile != null) { - inputsBuilder.add(parameterFile); - } - return inputsBuilder.build(); + /** + * Adds an artifact that is necessary for executing the spawn itself (e.g. a compiler), in + * contrast to an artifact that is necessary for the spawn to do its work (e.g. source code). + * + * <p>The artifact is implicitly added to the inputs of the action as well. + */ + public Builder addTool(Artifact tool) { + toolsBuilder.add(tool); + return this; } /** @@ -551,6 +602,14 @@ public class SpawnAction extends AbstractAction { } /** + * Adds tools to this action. + */ + public Builder addTools(Iterable<Artifact> artifacts) { + toolsBuilder.addAll(artifacts); + return this; + } + + /** * Adds inputs to this action. */ public Builder addInputs(Iterable<Artifact> artifacts) { @@ -566,6 +625,11 @@ public class SpawnAction extends AbstractAction { return this; } + private Builder addToolManifest(Artifact artifact, PathFragment remote) { + toolManifests.put(remote, artifact); + return this; + } + public Builder addInputManifest(Artifact artifact, PathFragment remote) { inputManifests.put(remote, artifact); return this; @@ -651,13 +715,13 @@ public class SpawnAction extends AbstractAction { * {@link #setShellCommand(String)}. */ public Builder setExecutable(Artifact executable) { + addTool(executable); return setExecutable(executable.getExecPath()); } /** - * Sets the executable as a configured target. Automatically adds the files - * to run to the inputs and uses the executable of the target as the - * executable. + * Sets the executable as a configured target. Automatically adds the files to run to the tools + * and inputs and uses the executable of the target as the executable. * * <p>Calling this method overrides any previous values set via calls to * {@link #setExecutable(Artifact)}, {@link #setJavaExecutable}, or @@ -670,13 +734,11 @@ public class SpawnAction extends AbstractAction { } /** - * Sets the executable as a configured target. Automatically adds the files - * to run to the inputs and uses the executable of the target as the - * executable. + * Sets the executable as a configured target. Automatically adds the files to run to the tools + * and inputs and uses the executable of the target as the executable. * - * <p>Calling this method overrides any previous values set via calls to - * {@link #setExecutable}, {@link #setJavaExecutable}, or - * {@link #setShellCommand(String)}. + * <p>Calling this method overrides any previous values set via calls to {@link #setExecutable}, + * {@link #setJavaExecutable}, or {@link #setShellCommand(String)}. */ public Builder setExecutable(FilesToRunProvider executableProvider) { Preconditions.checkArgument(executableProvider.getExecutable() != null, @@ -692,7 +754,7 @@ public class SpawnAction extends AbstractAction { executableArgs.add("-Xverify:none"); executableArgs.addAll(jvmArgs); Collections.addAll(executableArgs, launchArgs); - inputsBuilder.add(deployJar); + toolsBuilder.add(deployJar); this.isShellCommand = false; return this; } @@ -759,12 +821,15 @@ public class SpawnAction extends AbstractAction { } /** - * Adds an executable and its runfiles, so it can be called from a shell command. + * Adds an executable and its runfiles, which is necessary for executing the spawn itself (e.g. + * a compiler), in contrast to artifacts that are necessary for the spawn to do its work (e.g. + * source code). */ public Builder addTool(FilesToRunProvider tool) { - addInputs(tool.getFilesToRun()); + addTools(tool.getFilesToRun()); if (tool.getRunfilesManifest() != null) { - addInputManifest(tool.getRunfilesManifest(), + addToolManifest( + tool.getRunfilesManifest(), BaseSpawn.runfilesForFragment(tool.getExecutable().getExecPath())); } return this; diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRule.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRule.java index 146fd2588c..c37f3b33a1 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRule.java @@ -130,10 +130,17 @@ public class GenRule implements RuleConfiguredTargetFactory { inputs.add(ruleContext.getAnalysisEnvironment().getVolatileWorkspaceStatusArtifact()); } - ruleContext.registerAction(new GenRuleAction( - ruleContext.getActionOwner(), inputs.build(), filesToBuild, argv, env, - ImmutableMap.copyOf(executionInfo), commandHelper.getRemoteRunfileManifestMap(), - message + ' ' + ruleContext.getLabel())); + ruleContext.registerAction( + new GenRuleAction( + ruleContext.getActionOwner(), + commandHelper.getResolvedTools(), + inputs.build(), + filesToBuild, + argv, + env, + ImmutableMap.copyOf(executionInfo), + commandHelper.getRemoteRunfileManifestMap(), + message + ' ' + ruleContext.getLabel())); RunfilesProvider runfilesProvider = withData( // No runfiles provided if not a data dependency. diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleAction.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleAction.java index c9990c4ff6..7481cdcadc 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleAction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/genrule/GenRuleAction.java @@ -37,7 +37,9 @@ public final class GenRuleAction extends SpawnAction { // Not chosen scientifically/carefully. 300MB memory, 100% CPU, no I/O. ResourceSet.createWithRamCpuIo(300, 1.0, 0.0); - public GenRuleAction(ActionOwner owner, + public GenRuleAction( + ActionOwner owner, + Iterable<Artifact> tools, Iterable<Artifact> inputs, Iterable<Artifact> outputs, List<String> argv, @@ -45,10 +47,20 @@ public final class GenRuleAction extends SpawnAction { ImmutableMap<String, String> executionInfo, ImmutableMap<PathFragment, Artifact> runfilesManifests, String progressMessage) { - super(owner, inputs, outputs, GENRULE_RESOURCES, - CommandLine.of(argv, false), environment, executionInfo, progressMessage, + super( + owner, + tools, + inputs, + outputs, + GENRULE_RESOURCES, + CommandLine.of(argv, false), + environment, + executionInfo, + progressMessage, runfilesManifests, - "Genrule", false, null); + "Genrule", + false, + null); } @Override diff --git a/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraAction.java b/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraAction.java index 13cb5b5c56..6bf19d3672 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/extra/ExtraAction.java @@ -17,6 +17,7 @@ package com.google.devtools.build.lib.rules.extra; import com.google.common.base.Preconditions; import com.google.common.base.Predicates; import com.google.common.collect.Collections2; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.actions.AbstractAction; @@ -74,6 +75,7 @@ public final class ExtraAction extends SpawnAction { String mnemonic) { super( shadowedAction.getOwner(), + ImmutableList.<Artifact>of(), createInputs(shadowedAction.getInputs(), extraActionInputs), outputs, AbstractAction.DEFAULT_RESOURCE_SET, diff --git a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java index 72ab4fb934..23bbcc9514 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java +++ b/src/main/java/com/google/devtools/build/lib/rules/java/JavaCompileAction.java @@ -167,30 +167,32 @@ public class JavaCompileAction extends AbstractAction { * parts (for example, ide_build_info) need access to the data * @param commandLine the actual invocation command line */ - private JavaCompileAction(ActionOwner owner, - Iterable<Artifact> baseInputs, - Collection<Artifact> outputs, - CommandLine javaCompileCommandLine, - CommandLine commandLine, - PathFragment classDirectory, - Artifact outputJar, - NestedSet<Artifact> classpathEntries, - Collection<Artifact> extdirInputs, - List<Artifact> processorPath, - Artifact langtoolsJar, - Artifact javaBuilderJar, - Iterable<Artifact> instrumentationJars, - List<String> processorNames, - Collection<Artifact> messages, - Map<PathFragment, Artifact> resources, - Collection<Artifact> classpathResources, - Collection<Artifact> sourceJars, - Collection<Artifact> sourceFiles, - List<String> javacOpts, - Collection<Artifact> directJars, - BuildConfiguration.StrictDepsMode strictJavaDeps, - Collection<Artifact> compileTimeDependencyArtifacts) { - super(owner, NestedSetBuilder.<Artifact>stableOrder() + private JavaCompileAction( + ActionOwner owner, + NestedSet<Artifact> tools, + Iterable<Artifact> baseInputs, + Collection<Artifact> outputs, + CommandLine javaCompileCommandLine, + CommandLine commandLine, + PathFragment classDirectory, + Artifact outputJar, + NestedSet<Artifact> classpathEntries, + Collection<Artifact> extdirInputs, + List<Artifact> processorPath, + List<String> processorNames, + Collection<Artifact> messages, + Map<PathFragment, Artifact> resources, + Collection<Artifact> classpathResources, + Collection<Artifact> sourceJars, + Collection<Artifact> sourceFiles, + List<String> javacOpts, + Collection<Artifact> directJars, + BuildConfiguration.StrictDepsMode strictJavaDeps, + Collection<Artifact> compileTimeDependencyArtifacts) { + super( + owner, + tools, + NestedSetBuilder.<Artifact>stableOrder() .addTransitive(classpathEntries) .addAll(processorPath) .addAll(messages) @@ -200,9 +202,7 @@ public class JavaCompileAction extends AbstractAction { .addAll(sourceFiles) .addAll(compileTimeDependencyArtifacts) .addAll(baseInputs) - .add(langtoolsJar) - .add(javaBuilderJar) - .addAll(instrumentationJars) + .addTransitive(tools) .build(), outputs); this.javaCompileCommandLine = javaCompileCommandLine; @@ -888,7 +888,16 @@ public class JavaCompileAction extends AbstractAction { semantics.getJavaBuilderMainClass(), configuration.getHostPathSeparator()); - return new JavaCompileAction(owner, + NestedSet<Artifact> tools = + NestedSetBuilder.<Artifact>stableOrder() + .add(langtoolsJar) + .add(javaBuilderJar) + .addAll(instrumentationJars) + .build(); + + return new JavaCompileAction( + owner, + tools, baseInputs, outputs, paramFileContents, @@ -898,9 +907,6 @@ public class JavaCompileAction extends AbstractAction { classpathEntries, extdirInputs, processorPath, - langtoolsJar, - javaBuilderJar, - instrumentationJars, processorNames, translations, resources, diff --git a/src/main/java/com/google/devtools/build/lib/worker/Worker.java b/src/main/java/com/google/devtools/build/lib/worker/Worker.java index f239bc4c0c..dcfd09a614 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/Worker.java +++ b/src/main/java/com/google/devtools/build/lib/worker/Worker.java @@ -14,6 +14,7 @@ package com.google.devtools.build.lib.worker; import com.google.common.base.Preconditions; +import com.google.common.hash.HashCode; import com.google.devtools.build.lib.events.Event; import com.google.devtools.build.lib.events.Reporter; import com.google.devtools.build.lib.vfs.Path; @@ -40,11 +41,13 @@ final class Worker { private final int workerId; private final Process process; private final Thread shutdownHook; + private final HashCode workerFilesHash; - private Worker(Process process, Thread shutdownHook, int pid) { + private Worker(Process process, Thread shutdownHook, int pid, HashCode workerFilesHash) { this.process = process; this.shutdownHook = shutdownHook; this.workerId = pid; + this.workerFilesHash = workerFilesHash; } static Worker create(WorkerKey key, Path logDir, Reporter reporter, boolean verbose) @@ -83,7 +86,7 @@ final class Worker { + logFile)); } - return new Worker(process, shutdownHook, workerId); + return new Worker(process, shutdownHook, workerId, key.getWorkerFilesHash()); } void destroy() { @@ -125,6 +128,10 @@ final class Worker { return this.workerId; } + HashCode getWorkerFilesHash() { + return workerFilesHash; + } + boolean isAlive() { // This is horrible, but Process.isAlive() is only available from Java 8 on and this is the // best we can do prior to that. diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java index b15ca6e736..5deafce1f4 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerFactory.java @@ -72,10 +72,12 @@ final class WorkerFactory extends BaseKeyedPooledObjectFactory<WorkerKey, Worker } /** - * The worker is considered to be valid when its process is still alive. + * The worker is considered to be valid when its files have not changed on disk and its process is + * still alive. */ @Override public boolean validateObject(WorkerKey key, PooledObject<Worker> p) { - return p.getObject().isAlive(); + Worker worker = p.getObject(); + return key.getWorkerFilesHash().equals(worker.getWorkerFilesHash()) && worker.isAlive(); } } diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java index 076b1d4638..86494ea586 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerKey.java @@ -16,6 +16,7 @@ package com.google.devtools.build.lib.worker; import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; +import com.google.common.hash.HashCode; import com.google.devtools.build.lib.actions.BaseSpawn; import com.google.devtools.build.lib.vfs.Path; @@ -32,11 +33,23 @@ final class WorkerKey { private final Path workDir; private final String mnemonic; - WorkerKey(List<String> args, Map<String, String> env, Path workDir, String mnemonic) { + /** + * This is used during validation whether a worker is still usable. It is not used to uniquely + * identify a kind of worker, thus it is not to be used by the .equals() / .hashCode() methods. + */ + private final HashCode workerFilesHash; + + WorkerKey( + List<String> args, + Map<String, String> env, + Path workDir, + String mnemonic, + HashCode workerFilesHash) { this.args = ImmutableList.copyOf(Preconditions.checkNotNull(args)); this.env = ImmutableMap.copyOf(Preconditions.checkNotNull(env)); this.workDir = Preconditions.checkNotNull(workDir); this.mnemonic = Preconditions.checkNotNull(mnemonic); + this.workerFilesHash = Preconditions.checkNotNull(workerFilesHash); } public ImmutableList<String> getArgs() { @@ -55,6 +68,10 @@ final class WorkerKey { return mnemonic; } + public HashCode getWorkerFilesHash() { + return workerFilesHash; + } + @Override public boolean equals(Object o) { if (this == o) { diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerPool.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerPool.java index 47086aee8b..03eb7d6a54 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerPool.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerPool.java @@ -19,9 +19,6 @@ import com.google.devtools.build.lib.vfs.Path; import org.apache.commons.pool2.impl.GenericKeyedObjectPool; import org.apache.commons.pool2.impl.GenericKeyedObjectPoolConfig; -import java.util.HashSet; -import java.util.Set; - import javax.annotation.concurrent.ThreadSafe; /** @@ -33,7 +30,6 @@ import javax.annotation.concurrent.ThreadSafe; @ThreadSafe final class WorkerPool extends GenericKeyedObjectPool<WorkerKey, Worker> { final WorkerFactory workerFactory; - final Set<Worker> workers = new HashSet<>(); public WorkerPool(WorkerFactory factory, GenericKeyedObjectPoolConfig config) { super(factory, config); diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategy.java index cf893ea8e2..f143ab6576 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategy.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnStrategy.java @@ -19,7 +19,12 @@ import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; import com.google.common.eventbus.EventBus; import com.google.common.eventbus.Subscribe; +import com.google.common.hash.HashCode; +import com.google.common.hash.Hasher; +import com.google.common.hash.Hashing; import com.google.devtools.build.lib.actions.ActionExecutionContext; +import com.google.devtools.build.lib.actions.ActionInput; +import com.google.devtools.build.lib.actions.ActionInputFileCache; import com.google.devtools.build.lib.actions.ChangedFilesMessage; import com.google.devtools.build.lib.actions.ExecException; import com.google.devtools.build.lib.actions.ExecutionStrategy; @@ -40,6 +45,8 @@ import com.google.devtools.build.lib.worker.WorkerProtocol.WorkRequest; import com.google.devtools.build.lib.worker.WorkerProtocol.WorkResponse; import com.google.devtools.common.options.OptionsClassProvider; +import java.io.IOException; +import java.nio.charset.Charset; import java.util.concurrent.atomic.AtomicBoolean; /** @@ -48,9 +55,17 @@ import java.util.concurrent.atomic.AtomicBoolean; */ @ExecutionStrategy(name = { "worker" }, contextType = SpawnActionContext.class) final class WorkerSpawnStrategy implements SpawnActionContext { + public static final String REASON_HEURISTIC = + "Not using worker strategy, because incrementalHeuristic says no"; + public static final String REASON_NO_FLAGFILE = + "Not using worker strategy, because last argument does not contain a @flagfile"; + public static final String REASON_NO_TOOLS = + "Not using worker strategy, because the action has no tools"; + private final WorkerPool workers; private final IncrementalHeuristic incrementalHeuristic; private final StandaloneSpawnStrategy standaloneStrategy; + private final WorkerOptions options; private final boolean verboseFailures; private final int maxRetries; @@ -62,7 +77,7 @@ final class WorkerSpawnStrategy implements SpawnActionContext { boolean verboseFailures, int maxRetries) { Preconditions.checkNotNull(optionsProvider); - WorkerOptions options = optionsProvider.getOptions(WorkerOptions.class); + this.options = optionsProvider.getOptions(WorkerOptions.class); this.incrementalHeuristic = new IncrementalHeuristic(options.workerMaxChangedFiles); eventBus.register(incrementalHeuristic); this.workers = Preconditions.checkNotNull(workers); @@ -75,6 +90,8 @@ final class WorkerSpawnStrategy implements SpawnActionContext { public void exec(Spawn spawn, ActionExecutionContext actionExecutionContext) throws ExecException, InterruptedException { Executor executor = actionExecutionContext.getExecutor(); + EventHandler eventHandler = executor.getEventHandler(); + if (executor.reportsSubcommands()) { executor.reportSubcommand( Label.print(spawn.getOwner().getLabel()) @@ -85,16 +102,29 @@ final class WorkerSpawnStrategy implements SpawnActionContext { } if (!incrementalHeuristic.shouldUseWorkers()) { + if (options.workerVerbose) { + eventHandler.handle(Event.info(REASON_HEURISTIC)); + } standaloneStrategy.exec(spawn, actionExecutionContext); return; } - // We assume that the spawn to be executed always gets a single argument, which is a flagfile - // prefixed with @ and that it will start in persistent mode when we don't pass it one. - // Thus, we can extract the last element from its args (which will be the flagfile) to start the - // persistent mode and then pass it the flagfile via a WorkRequest to make it actually do the - // work. + // We assume that the spawn to be executed always gets a @flagfile argument, which contains the + // flags related to the work itself (as opposed to start-up options for the executed tool). + // Thus, we can extract the last element from its args (which will be the @flagfile), expand it + // and put that into the WorkRequest instead. if (!Iterables.getLast(spawn.getArguments()).startsWith("@")) { + if (options.workerVerbose) { + eventHandler.handle(Event.info(REASON_NO_FLAGFILE)); + } + standaloneStrategy.exec(spawn, actionExecutionContext); + return; + } + + if (Iterables.isEmpty(spawn.getToolFiles())) { + if (options.workerVerbose) { + eventHandler.handle(Event.info(REASON_NO_TOOLS)); + } standaloneStrategy.exec(spawn, actionExecutionContext); return; } @@ -108,9 +138,13 @@ final class WorkerSpawnStrategy implements SpawnActionContext { .build(); ImmutableMap<String, String> env = spawn.getEnvironment(); Path workDir = actionExecutionContext.getExecutor().getExecRoot(); - WorkerKey key = new WorkerKey(args, env, workDir, spawn.getMnemonic()); try { + HashCode workerFilesHash = + combineActionInputHashes( + spawn.getToolFiles(), actionExecutionContext.getActionInputFileCache()); + WorkerKey key = new WorkerKey(args, env, workDir, spawn.getMnemonic(), workerFilesHash); + WorkResponse response = execInWorker(executor.getEventHandler(), paramFile, key, maxRetries); outErr.getErrorStream().write(response.getOutputBytes().toByteArray()); @@ -127,6 +161,17 @@ final class WorkerSpawnStrategy implements SpawnActionContext { } } + private HashCode combineActionInputHashes( + Iterable<? extends ActionInput> toolFiles, ActionInputFileCache actionInputFileCache) + throws IOException { + Hasher hasher = Hashing.sha256().newHasher(); + for (ActionInput tool : toolFiles) { + hasher.putString(tool.getExecPathString(), Charset.defaultCharset()); + hasher.putBytes(actionInputFileCache.getDigest(tool).toByteArray()); + } + return hasher.hash(); + } + private WorkResponse execInWorker( EventHandler eventHandler, String paramFile, WorkerKey key, int retriesLeft) throws Exception { |