Skip to content

Commit

Permalink
Centralize PathMapper construction
Browse files Browse the repository at this point in the history
The new `PathMappers` helper class is introduced to be the canonical
place for actions to create `PathMapper` instances from. As a result,
the particular kind of `PathMapper` can now be switched out without
having to change individual actions.

This requires a larger refactoring of how `PathMapper` instances are
created and passed around, which results in a number of additional
improvements:

* `PathMapper` instances are now only created when an `Action` is
  converted into a `Spawn` for execution and are no longer retained.
  This should prevent memory regressions even if `PathMapper` instances
  need to add more fields.
* By exposing the active `PathMapper` via `Spawn#getPathMapper`,
  participating executors automatically use the same instance that was
  used to map paths in the action's command lines, which ensures
  consistency.
* Actions now have a simple way to add a fingerprint for the given
  `PathMapper` instance, thus ensuring incremental correctness when the
  value of the `--experimental_output_path_mode` flag or the
  implementation of a particular `PathMapper` changes.
* `PathMapper`s are no longer passed into builders for structured
  command lines, but are supplied when expanding command lines. In
  addition to not retaining them, this also reduces the number of places
  in which participating actions have to pass in `PathMapper`s.

Apart from completely pure refactorings, two more interesting changes
were necessary to prevent `PathMapper` instances from being constructed
during analysis:

* Native actions are now also opted in by their mnemonic, rather than
  declaring their support in code. This makes it easier to experiment
  with path mapping and opens up the possibility to maintain the list of
  supported mnemonics in a flag, which can be done in a follow-up PR.
* Instead of hard-coding stripping for execution-time arguments that use
  location expansion for exec paths (in this case, `--javacopts`), a new
  function on `PathMapper` now allows mapping arbitray string-valued
  vector args. This is only relevant for native rules as Starlark rules
  can perform location expansion manually and emit a structured command
  line. A helper function for this may be provided in the future.
  • Loading branch information
fmeum committed Apr 14, 2023
1 parent c8d036d commit 64387c0
Show file tree
Hide file tree
Showing 34 changed files with 682 additions and 768 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
import com.google.devtools.build.lib.actions.PathStripper.PathMapper;
import com.google.devtools.build.lib.collect.CollectionUtils;
import com.google.devtools.build.lib.collect.IterablesChain;
import com.google.devtools.build.lib.util.Fingerprint;
Expand Down Expand Up @@ -46,14 +45,9 @@ public abstract Iterable<String> arguments()
* artifact expansion. Subclasses should override this method if they contain TreeArtifacts and
* need to expand them for proper argument evaluation.
*/
public Iterable<String> arguments(ArtifactExpander artifactExpander)
throws CommandLineExpansionException, InterruptedException {
return arguments();
}

public Iterable<String> arguments(ArtifactExpander artifactExpander, PathMapper pathMapper)
throws CommandLineExpansionException, InterruptedException {
return arguments(artifactExpander);
return arguments();
}

/**
Expand Down Expand Up @@ -108,9 +102,10 @@ public Iterable<String> arguments() throws CommandLineExpansionException, Interr
}

@Override
public Iterable<String> arguments(ArtifactExpander artifactExpander)
public Iterable<String> arguments(ArtifactExpander artifactExpander, PathMapper pathMapper)
throws CommandLineExpansionException, InterruptedException {
return IterablesChain.concat(executableArgs, commandLine.arguments(artifactExpander));
return IterablesChain.concat(executableArgs,
commandLine.arguments(artifactExpander, pathMapper));
}
}

Expand All @@ -129,9 +124,10 @@ public Iterable<String> arguments() throws CommandLineExpansionException, Interr
}

@Override
public Iterable<String> arguments(ArtifactExpander artifactExpander)
public Iterable<String> arguments(ArtifactExpander artifactExpander, PathMapper pathMapper)
throws CommandLineExpansionException, InterruptedException {
return IterablesChain.concat(commandLine.arguments(artifactExpander), executableArgs);
return IterablesChain.concat(commandLine.arguments(artifactExpander, pathMapper),
executableArgs);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
import com.google.devtools.build.lib.actions.ParameterFile.ParameterFileType;
import com.google.devtools.build.lib.actions.PathStripper.PathMapper;
import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
import com.google.devtools.build.lib.collect.IterablesChain;
import com.google.devtools.build.lib.util.Fingerprint;
Expand Down Expand Up @@ -110,8 +109,8 @@ private CommandLines(Object commandLines) {
*
* @param artifactExpander The artifact expander to use.
* @param paramFileBasePath Used to derive param file names. Often the first output of an action
* @param pathMapper function to strip configuration prefixes from output paths, in accordance
* with the logic in {@link PathStripper}
* @param pathMapper function to map configuration prefixes in output paths to more cache-friendly
* identifiers
* @param limits The command line limits the host OS can support.
* @return The expanded command line and its param files (if any).
*/
Expand All @@ -136,7 +135,7 @@ ExpandedCommandLines expand(
// Optimize for simple case of single command line
if (commandLines instanceof CommandLine) {
CommandLine commandLine = (CommandLine) commandLines;
Iterable<String> arguments = commandLine.arguments(artifactExpander);
Iterable<String> arguments = commandLine.arguments(artifactExpander, pathMapper);
return new ExpandedCommandLines(arguments, ImmutableList.of());
}
List<CommandLineAndParamFileInfo> commandLines = getCommandLines();
Expand Down Expand Up @@ -212,15 +211,9 @@ ExpandedCommandLines expand(
*/
public ImmutableList<String> allArguments()
throws CommandLineExpansionException, InterruptedException {
return allArguments(PathMapper.NOOP);
}

/** Variation of {@link #allArguments()} that supports output path stripping. */
public ImmutableList<String> allArguments(PathMapper stripPaths)
throws CommandLineExpansionException, InterruptedException {
ImmutableList.Builder<String> arguments = ImmutableList.builder();
for (CommandLineAndParamFileInfo pair : getCommandLines()) {
arguments.addAll(pair.commandLine.arguments(/*artifactExpander=*/ null, stripPaths));
arguments.addAll(pair.commandLine.arguments(/*artifactExpander=*/ null, PathMapper.NOOP));
}
return arguments.build();
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
// Copyright 2023 The Bazel Authors. All rights reserved.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package com.google.devtools.build.lib.actions;

import com.google.devtools.build.lib.actions.CommandLineItem.MapFn;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.List;

/**
* Support for mapping config parts of exec paths in an action's command line as well as when
* staging its inputs and outputs for execution, with the aim of making the resulting {@link Spawn}
* more cacheable.
*
* <p>Action that want to support path mapping should use {@link
* com.google.devtools.build.lib.analysis.actions.PathMappers}.
*
* <p>An example of an implementing class is {@link
* com.google.devtools.build.lib.analysis.actions.StrippingPathMapper}, which removes the config
* part (e.g. "k8-fastbuild") from exec paths to allow for cross-configuration cache hits.
*/
public interface PathMapper {
/**
* Returns the exec path with the path mapping applied.
*/
PathFragment map(PathFragment execPath);

/**
* Returns the exec path of the input with the path mapping applied.
*/
default String getMappedExecPathString(ActionInput artifact) {
return map(artifact.getExecPath()).getPathString();
}

/**
* We don't yet have a Starlark API for mapping paths in command lines. Simple Starlark calls like
* {@code args.add(arg_name, file_path} are automatically handled. But calls that involve custom
* Starlark code require deeper API support that remains a TODO.
*
* <p>This method allows implementations to hard-code support for specific command line entries
* for specific Starlark actions.
*/
default List<String> mapCustomStarlarkArgs(List<String> args) {
return args;
}

/**
* Returns the {@link MapFn} to apply to a vector argument with the given previous String argument
* in a {@link com.google.devtools.build.lib.analysis.actions.CustomCommandLine}.
*
* <p>For example, if the previous argument is {@code "--foo"}, this method should return a
* {@link MapFn} that maps the next arguments to the correct path, potentially mapping them if
* "--foo" requires it.
*
* <p>This is used to map paths obtained via location expansion in native rules, which returns
* a list of strings rather than a structured command line.
*
* <p>By default, this method returns {@link MapFn#DEFAULT}.
*/
default MapFn<Object> getMapFn(String previousArg) {
return MapFn.DEFAULT;
}

/**
* @return {@code true} if the mapper is known to map all paths identically.
*
* <p>Can be used by actions to skip additional work that isn't needed if path mapping is not
* enabled.
*/
default boolean isNoop() {
return this == NOOP;
}

/**
* A {@link PathMapper} that doesn't change paths.
*/
PathMapper NOOP = execPath -> execPath;
}
Loading

0 comments on commit 64387c0

Please sign in to comment.