Skip to content

Commit

Permalink
Refactor marker file logic
Browse files Browse the repository at this point in the history
Marker files today store all predeclared inputs as one hash (on the first line of the file), and then each recorded input as a following line in the `TYPE:KEY VALUE` format. This commit refactors the parsing/stringification logic of recorded inputs so that they're not all clumped in big methods in `RepositoryFunction`, to pave the way for more recorded input types (watching directories, etc) and more places to write recorded input data (for the true repo cache).

- The StarlarkSemantics object is no longer treated as a recorded input (only recorded for Starlark repo rules, ignored for native repo rules), but as a predeclared input instead (i.e. hashed on the first line).
  - This slightly simplifies logic, and since the existing native repo rules are either local (local_repository, new_local_repository, local_config_platform) or being Starlarkified (the two Android repo rules), it will have minimal visible impact.
- Each type of recorded inputs is a subclass of `RepoRecordedInput`, which knows how to stringify itself, verify its own up-to-date-ness, and how to parse itself from a string.
- We also try to collect as many SkyKeys needed to verify up-to-date-ness as possible in one go and do a mass Skyframe evaluation. This avoids a fair amount of Skyframe restarts (unlikely to have super big impact on performance, but is a nice thing to do).

Work towards #20952 and #12227.

Closes #21182.

PiperOrigin-RevId: 604522692
Change-Id: Idc18ab202adb601cda47914c48642a6c9039da40
  • Loading branch information
Wyverald authored and copybara-github committed Feb 6, 2024
1 parent 05b1f06 commit 5ab21e0
Show file tree
Hide file tree
Showing 13 changed files with 432 additions and 279 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.google.devtools.build.lib.analysis.RuleDefinition;
import com.google.devtools.build.lib.bazel.ResolvedEvent;
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.rules.repository.RepoRecordedInput;
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.ResolvedFileValue;
Expand Down Expand Up @@ -56,7 +57,7 @@ public RepositoryDirectoryValue.Builder fetch(
Path outputDirectory,
BlazeDirectories directories,
Environment env,
Map<String, String> markerData,
Map<RepoRecordedInput, String> recordedInputValues,
SkyKey key)
throws RepositoryFunctionException {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@

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

import com.google.devtools.build.lib.rules.repository.RepoRecordedInput;
import com.google.devtools.build.lib.rules.repository.RepositoryDirectoryValue;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyFunction.Environment.SkyKeyComputeState;
Expand Down Expand Up @@ -71,12 +72,12 @@ enum Signal {
@Nullable volatile Future<RepositoryDirectoryValue.Builder> workerFuture = null;

/**
* This is where the {@code markerData} for the whole invocation is collected.
* This is where the recorded inputs & values for the whole invocation is collected.
*
* <p>{@link com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction} creates a
* new map on each restart, so we can't simply plumb that in.
*/
final Map<String, String> markerData = new TreeMap<>();
final Map<RepoRecordedInput, String> recordedInputValues = new TreeMap<>();

SkyFunction.Environment signalForFreshEnv() throws InterruptedException {
signalQueue.put(Signal.RESTART);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import com.google.devtools.build.lib.profiler.ProfilerTask;
import com.google.devtools.build.lib.profiler.SilentCloseable;
import com.google.devtools.build.lib.rules.repository.NeedsSkyframeRestartException;
import com.google.devtools.build.lib.rules.repository.RepoRecordedInput;
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.WorkspaceFileHelper;
Expand All @@ -55,8 +56,6 @@
import com.google.devtools.build.skyframe.SkyKey;
import java.io.IOException;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.ExecutorService;
import javax.annotation.Nullable;
Expand All @@ -69,8 +68,6 @@

/** A repository function to delegate work done by Starlark remote repositories. */
public final class StarlarkRepositoryFunction extends RepositoryFunction {
static final String SEMANTICS = "STARLARK_SEMANTICS";

private final DownloadManager downloadManager;
private double timeoutScaling = 1.0;
@Nullable private ExecutorService workerExecutorService = null;
Expand Down Expand Up @@ -98,26 +95,6 @@ public void setWorkerExecutorService(@Nullable ExecutorService workerExecutorSer
this.workerExecutorService = workerExecutorService;
}

static String describeSemantics(StarlarkSemantics semantics) {
// Here we use the hash code provided by AutoValue. This is unique, as long
// as the number of bits in the StarlarkSemantics is small enough. We will have to
// move to a longer description once the number of flags grows too large.
return "" + semantics.hashCode();
}

@Override
protected boolean verifySemanticsMarkerData(Map<String, String> markerData, Environment env)
throws InterruptedException {
StarlarkSemantics starlarkSemantics = PrecomputedValue.STARLARK_SEMANTICS.get(env);
if (starlarkSemantics == null) {
// As it is a precomputed value, it should already be available. If not, returning
// false is the safe thing to do.
return false;
}

return describeSemantics(starlarkSemantics).equals(markerData.get(SEMANTICS));
}

@Override
protected void setupRepoRootBeforeFetching(Path repoRoot) throws RepositoryFunctionException {
// DON'T delete the repo root here if we're using a worker thread, since when this SkyFunction
Expand All @@ -143,11 +120,11 @@ public RepositoryDirectoryValue.Builder fetch(
Path outputDirectory,
BlazeDirectories directories,
Environment env,
Map<String, String> markerData,
Map<RepoRecordedInput, String> recordedInputValues,
SkyKey key)
throws RepositoryFunctionException, InterruptedException {
if (workerExecutorService == null) {
return fetchInternal(rule, outputDirectory, directories, env, markerData, key);
return fetchInternal(rule, outputDirectory, directories, env, recordedInputValues, key);
}
var state = env.getState(RepoFetchingSkyKeyComputeState::new);
var workerFuture = state.workerFuture;
Expand All @@ -161,7 +138,12 @@ public RepositoryDirectoryValue.Builder fetch(
() -> {
try {
return fetchInternal(
rule, outputDirectory, directories, workerEnv, state.markerData, key);
rule,
outputDirectory,
directories,
workerEnv,
state.recordedInputValues,
key);
} finally {
state.signalQueue.put(Signal.DONE);
}
Expand All @@ -178,7 +160,7 @@ public RepositoryDirectoryValue.Builder fetch(
case DONE:
try {
RepositoryDirectoryValue.Builder result = workerFuture.get();
markerData.putAll(state.markerData);
recordedInputValues.putAll(state.recordedInputValues);
return result;
} catch (ExecutionException e) {
Throwables.throwIfInstanceOf(e.getCause(), RepositoryFunctionException.class);
Expand Down Expand Up @@ -209,22 +191,21 @@ private RepositoryDirectoryValue.Builder fetchInternal(
Path outputDirectory,
BlazeDirectories directories,
Environment env,
Map<String, String> markerData,
Map<RepoRecordedInput, String> recordedInputValues,
SkyKey key)
throws RepositoryFunctionException, InterruptedException {

String defInfo = RepositoryResolvedEvent.getRuleDefinitionInformation(rule);
env.getListener().post(new StarlarkRepositoryDefinitionLocationEvent(rule.getName(), defInfo));

StarlarkCallable function = rule.getRuleClassObject().getConfiguredTargetFunction();
if (declareEnvironmentDependencies(markerData, env, getEnviron(rule)) == null) {
if (declareEnvironmentDependencies(recordedInputValues, env, getEnviron(rule)) == null) {
return null;
}
StarlarkSemantics starlarkSemantics = PrecomputedValue.STARLARK_SEMANTICS.get(env);
if (env.valuesMissing()) {
return null;
}
markerData.put(SEMANTICS, describeSemantics(starlarkSemantics));

PathPackageLocator packageLocator = PrecomputedValue.PATH_PACKAGE_LOCATOR.get(env);
if (env.valuesMissing()) {
Expand Down Expand Up @@ -324,22 +305,20 @@ private RepositoryDirectoryValue.Builder fetchInternal(
// Modify marker data to include the files used by the rule's implementation function.
for (Map.Entry<Label, String> entry :
starlarkRepositoryContext.getAccumulatedFileDigests().entrySet()) {
// A label does not contain spaces so it's safe to use as a key.
markerData.put("FILE:" + entry.getKey(), entry.getValue());
recordedInputValues.put(new RepoRecordedInput.LabelFile(entry.getKey()), entry.getValue());
}

// Ditto for environment variables accessed via `getenv`.
for (String envKey : starlarkRepositoryContext.getAccumulatedEnvKeys()) {
markerData.put("ENV:" + envKey, clientEnvironment.get(envKey));
recordedInputValues.put(
new RepoRecordedInput.EnvVar(envKey), clientEnvironment.get(envKey));
}

for (Table.Cell<RepositoryName, String, RepositoryName> repoMappings :
repoMappingRecorder.recordedEntries().cellSet()) {
markerData.put(
"REPO_MAPPING:"
+ repoMappings.getRowKey().getName()
+ ","
+ repoMappings.getColumnKey(),
recordedInputValues.put(
new RepoRecordedInput.RecordedRepoMapping(
repoMappings.getRowKey(), repoMappings.getColumnKey()),
repoMappings.getValue().getName());
}

Expand Down Expand Up @@ -393,47 +372,6 @@ private static ImmutableSet<String> getEnviron(Rule rule) {
return ImmutableSet.copyOf((Iterable<String>) rule.getAttr("$environ"));
}

/**
* Verify marker data previously saved by {@link #declareEnvironmentDependencies(Map, Environment,
* Set)} and/or {@link #fetchInternal(Rule, Path, BlazeDirectories, Environment, Map, SkyKey)} (on
* behalf of {@link StarlarkBaseExternalContext#getEnvironmentValue(String, Object)}).
*/
@Override
protected boolean verifyEnvironMarkerData(
Map<String, String> markerData, Environment env, Set<String> keys)
throws InterruptedException {
/*
* We can ignore `keys` and instead only verify what's recorded in the marker file, because
* any change to `keys` between builds would be caused by a change to a .bzl file, and that's
* covered by RepositoryDelegatorFunction.DigestWriter#areRepositoryAndMarkerFileConsistent.
*/

ImmutableSet<String> markerKeys =
markerData.keySet().stream()
.filter(s -> s.startsWith("ENV:"))
.collect(ImmutableSet.toImmutableSet());

ImmutableMap<String, String> environ =
getEnvVarValues(
env,
markerKeys.stream()
.map(s -> s.substring(4)) // ENV:FOO -> FOO
.collect(ImmutableSet.toImmutableSet()));
if (environ == null) {
return false;
}

for (String key : markerKeys) {
String markerValue = markerData.get(key);
String envKey = key.substring(4); // ENV:FOO -> FOO
if (!Objects.equals(markerValue, environ.get(envKey))) {
return false;
}
}

return true;
}

@Override
protected boolean isLocal(Rule rule) {
return (Boolean) rule.getAttr("$local");
Expand All @@ -454,6 +392,7 @@ public static boolean isConfigureRule(Rule rule) {
return rule.getRuleClassObject().isStarlark() && ((Boolean) rule.getAttr("$configure"));
}

@Nullable
@Override
public Class<? extends RuleDefinition> getRuleDefinition() {
return null; // unused so safe to return null
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.Type;
import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
import com.google.devtools.build.lib.rules.repository.RepoRecordedInput;
import com.google.devtools.build.lib.rules.repository.RepositoryDirectoryValue;
import com.google.devtools.build.lib.rules.repository.WorkspaceAttributeMapper;
import com.google.devtools.build.lib.skyframe.DirectoryListingValue;
Expand All @@ -60,6 +61,7 @@
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import javax.annotation.Nullable;
import net.starlark.java.eval.EvalException;
Expand All @@ -75,7 +77,7 @@ public class AndroidNdkRepositoryFunction extends AndroidRepositoryFunction {

private static final ImmutableSet<String> PATH_ENV_VAR_AS_SET = ImmutableSet.of(PATH_ENV_VAR);

private static String getDefaultCrosstool(Integer majorRevision) {
private static String getDefaultCrosstool(int majorRevision) {
// From NDK 17, libc++ replaces gnu-libstdc++ as the default STL.
return majorRevision <= 16 ? GnuLibStdCppStlImpl.NAME : LibCppStlImpl.NAME;
}
Expand Down Expand Up @@ -170,7 +172,7 @@ private static String createCcToolchainRule(
// but the gcc tool will actually be clang.
ToolPath gcc = null;
for (ToolPath toolPath : toolchain.getToolPathList()) {
if ("gcc".equals(toolPath.getName())) {
if (toolPath.getName().equals("gcc")) {
gcc = toolPath;
}
}
Expand Down Expand Up @@ -247,13 +249,14 @@ public boolean isLocal(Rule rule) {
}

@Override
public boolean verifyMarkerData(Rule rule, Map<String, String> markerData, Environment env)
public boolean verifyRecordedInputs(
Rule rule, Map<RepoRecordedInput, String> recordedInputValues, Environment env)
throws InterruptedException {
WorkspaceAttributeMapper attributes = WorkspaceAttributeMapper.of(rule);
if (attributes.isAttributeValueExplicitlySpecified("path")) {
return true;
}
return super.verifyEnvironMarkerData(markerData, env, PATH_ENV_VAR_AS_SET);
return super.verifyRecordedInputs(rule, recordedInputValues, env);
}

@Override
Expand All @@ -263,11 +266,11 @@ public RepositoryDirectoryValue.Builder fetch(
Path outputDirectory,
BlazeDirectories directories,
Environment env,
Map<String, String> markerData,
Map<RepoRecordedInput, String> recordedInputValues,
SkyKey key)
throws InterruptedException, RepositoryFunctionException {
Map<String, String> environ =
declareEnvironmentDependencies(markerData, env, PATH_ENV_VAR_AS_SET);
declareEnvironmentDependencies(recordedInputValues, env, PATH_ENV_VAR_AS_SET);
if (environ == null) {
return null;
}
Expand Down Expand Up @@ -510,7 +513,7 @@ private ImmutableList<String> generateBzlConfigFor(String version, CToolchain to
tp ->
String.format(
" %s_path = '%s'",
tp.getName().toLowerCase().replaceAll("-", "_"), tp.getPath()))
tp.getName().toLowerCase(Locale.ROOT).replaceAll("-", "_"), tp.getPath()))
.collect(ImmutableList.toImmutableList()));
return bigConditional.add("").build();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import com.google.devtools.build.lib.io.InconsistentFilesystemException;
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.Type;
import com.google.devtools.build.lib.rules.repository.RepoRecordedInput;
import com.google.devtools.build.lib.rules.repository.RepositoryDirectoryValue;
import com.google.devtools.build.lib.rules.repository.WorkspaceAttributeMapper;
import com.google.devtools.build.lib.skyframe.DirectoryListingValue;
Expand Down Expand Up @@ -177,13 +178,14 @@ public boolean isLocal(Rule rule) {
}

@Override
public boolean verifyMarkerData(Rule rule, Map<String, String> markerData, Environment env)
public boolean verifyRecordedInputs(
Rule rule, Map<RepoRecordedInput, String> recordedInputValues, Environment env)
throws InterruptedException {
WorkspaceAttributeMapper attributes = WorkspaceAttributeMapper.of(rule);
if (attributes.isAttributeValueExplicitlySpecified("path")) {
return true;
}
return super.verifyEnvironMarkerData(markerData, env, PATH_ENV_VAR_AS_SET);
return super.verifyRecordedInputs(rule, recordedInputValues, env);
}

@Override
Expand All @@ -193,11 +195,11 @@ public RepositoryDirectoryValue.Builder fetch(
final Path outputDirectory,
BlazeDirectories directories,
Environment env,
Map<String, String> markerData,
Map<RepoRecordedInput, String> recordedInputValues,
SkyKey key)
throws RepositoryFunctionException, InterruptedException {
Map<String, String> environ =
declareEnvironmentDependencies(markerData, env, PATH_ENV_VAR_AS_SET);
declareEnvironmentDependencies(recordedInputValues, env, PATH_ENV_VAR_AS_SET);
if (environ == null) {
return null;
}
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/com/google/devtools/build/lib/rules/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,7 @@ java_library(
srcs = [
"repository/LocalRepositoryFunction.java",
"repository/NeedsSkyframeRestartException.java",
"repository/RepoRecordedInput.java",
"repository/RepositoryDelegatorFunction.java",
"repository/RepositoryDirectoryDirtinessChecker.java",
"repository/RepositoryFunction.java",
Expand Down Expand Up @@ -412,6 +413,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/repository:repository_events",
"//src/main/java/com/google/devtools/build/lib/skyframe:action_environment_function",
"//src/main/java/com/google/devtools/build/lib/skyframe:already_reported_exception",
"//src/main/java/com/google/devtools/build/lib/skyframe:client_environment_value",
"//src/main/java/com/google/devtools/build/lib/skyframe:package_lookup_function",
"//src/main/java/com/google/devtools/build/lib/skyframe:package_lookup_value",
"//src/main/java/com/google/devtools/build/lib/skyframe:precomputed_value",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public RepositoryDirectoryValue.Builder fetch(
Path outputDirectory,
BlazeDirectories directories,
Environment env,
Map<String, String> markerData,
Map<RepoRecordedInput, String> recordedInputValues,
SkyKey key)
throws InterruptedException, RepositoryFunctionException {
String userDefinedPath = RepositoryFunction.getPathAttr(rule);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public RepositoryDirectoryValue.Builder fetch(
Path outputDirectory,
BlazeDirectories directories,
Environment env,
Map<String, String> markerData,
Map<RepoRecordedInput, String> recordedInputValues,
SkyKey key)
throws InterruptedException, RepositoryFunctionException {

Expand Down Expand Up @@ -161,7 +161,7 @@ public RepositoryDirectoryValue.Builder fetch(
return null;
}

fileHandler.finishFile(rule, outputDirectory, markerData);
fileHandler.finishFile(rule, outputDirectory, recordedInputValues);
env.getListener().post(resolve(rule));

return RepositoryDirectoryValue.builder()
Expand Down
Loading

0 comments on commit 5ab21e0

Please sign in to comment.