Skip to content

Commit

Permalink
Add diff_against_dynamic_baseline option to experimental_output_direc…
Browse files Browse the repository at this point in the history
…tory_naming_scheme.

To support, a new SkyFunction is introduced, BaselineOptionsFunction, with corresponding SkyValue and SkyKey. Also, a new function ExecutionTransitionFactory.createTransition was added to immediately create an ExecutionTransition.

When --experimental_output_directory_naming_scheme=diff_against_dynamic_baseline, the behavior is similar to diff_against_baseline, except when "is Exec" is true (that is, an ExecutionTransition has previously been applied), the baseline is chosen to be result of the ExecutionTransition being applied to the normal baseline.

This is meant to resolve an issue with performance when using remote execution engines that collate requests from multiple users. They were seeing varying output paths for actions in an execution configuration depending on how much 'resetting' the exec transition was doing because of varying user commandlines.

This relates to #14023 and #18002

PiperOrigin-RevId: 537097551
Change-Id: I72966406b7b8af0174a81b638bf0d1fb62466653
  • Loading branch information
Googler authored and fmeum committed Sep 13, 2023
1 parent 2fe15f5 commit 76a4ad7
Show file tree
Hide file tree
Showing 11 changed files with 338 additions and 26 deletions.
15 changes: 15 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -1291,6 +1291,21 @@ java_library(
],
)

java_library(
name = "config/transitions/baseline_options_value",
srcs = [
"config/transitions/BaselineOptionsValue.java",
],
deps = [
":config/build_options",
"//src/main/java/com/google/devtools/build/lib/concurrent:thread_safety",
"//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions",
"//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
"//third_party:auto_value",
"//third_party:error_prone_annotations",
],
)

# TODO(b/144899336): This should be analysis/actions/BUILD
java_library(
name = "actions/abstract_file_write_action",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -575,7 +575,9 @@ public enum OutputDirectoryNamingScheme {
/** Use `affected by starlark transition` to track configuration changes */
LEGACY,
/** Produce name based on diff from some baseline BuildOptions (usually top-level) */
DIFF_AGAINST_BASELINE
DIFF_AGAINST_BASELINE,
/** Like DIFF_AGAINST_BASELINE, but compare against post-exec baseline if isExec is set. */
DIFF_AGAINST_DYNAMIC_BASELINE
}

/** Converter for the {@code --experimental_output_directory_naming_scheme} options. */
Expand All @@ -601,6 +603,17 @@ public OutputDirectoryNamingSchemeConverter() {
+ " for all configuration, by diffing against the top-level configuration.")
public OutputDirectoryNamingScheme outputDirectoryNamingScheme;

public boolean useBaselineForOutputDirectoryNamingScheme() {
switch (outputDirectoryNamingScheme) {
case DIFF_AGAINST_BASELINE:
case DIFF_AGAINST_DYNAMIC_BASELINE:
return true;
case LEGACY:
return false;
}
throw new IllegalStateException("unreachable");
}

@Option(
name = "is host configuration",
defaultValue = "false",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,11 @@
* {@link TransitionFactory} implementation which creates a {@link PatchTransition} which will
* transition to a configuration suitable for building dependencies for the execution platform of
* the depending target.
*
* <p>Note that execGroup is not directly consumed by the involved transition but instead stored
* here. Instead, the rule definition stores it in this factory. Then, toolchain resolution extracts
* and consumes it to store an execution platform in attrs. Finally, the execution platform is read
* by the factory to create the transition.
*/
public class ExecutionTransitionFactory
implements TransitionFactory<AttributeTransitionData>, ExecTransitionFactoryApi {
Expand All @@ -48,6 +53,11 @@ public static ExecutionTransitionFactory create() {
return new ExecutionTransitionFactory(DEFAULT_EXEC_GROUP_NAME);
}

/** Returns a new {@link ExecutionTransition} immediately. */
public static PatchTransition createTransition(@Nullable Label executionPlatform) {
return new ExecutionTransition(executionPlatform);
}

/**
* Returns a new {@link ExecutionTransitionFactory} for the given {@link
* com.google.devtools.build.lib.packages.ExecGroup}.
Expand Down Expand Up @@ -104,6 +114,10 @@ public String getName() {

@Override
public ImmutableSet<Class<? extends FragmentOptions>> requiresOptionFragments() {
// This is technically a lie since the call to underlying().createExecOptions is transitively
// reading and potentially modifying all fragments. There is currently no way for the
// transition to actually list all fragments like this and thus only lists the ones that are
// directly being read here. Note that this transition is exceptional in its implementation.
return FRAGMENTS;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
// 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.analysis.config.transitions;

import com.google.auto.value.AutoValue;
import com.google.devtools.build.lib.analysis.config.BuildOptions;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import com.google.devtools.build.lib.skyframe.SkyFunctions;
import com.google.devtools.build.skyframe.SkyFunctionName;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import com.google.errorprone.annotations.CheckReturnValue;

/**
* This contains the baseline options to compare against when constructing output paths.
*
* <p>When constructing the output mnemonic as part of making a {@link BuildConfigurationValue} and
* the selected naming scheme is to diff against a baseline, this function returns the baseline to
* use for that comparison. Differences in options between the given option and this baseline will
* then be used to append a deconflicting ST-hash to the output mnemonic.
*
* <p>The afterExecTransition option in the key will apply the exec transition to the usual
* baseline. It is expected that this is set whenever the given options have isExec set (and thus an
* exec transition has already been applied to those options). The expectation here is that, as the
* exec transition particularly sets many options, comparing against a post-exec baseline will yield
* fewer diffenences. Note that some indicator must be added to the mnemonic (e.g. -exec-) in order
* to deconflict for similar options where isExec is not set.
*/
@CheckReturnValue
@Immutable
@ThreadSafe
@AutoValue
public abstract class BaselineOptionsValue implements SkyValue {
public abstract BuildOptions toOptions();

public static BaselineOptionsValue create(BuildOptions toOptions) {
return new AutoValue_BaselineOptionsValue(toOptions);
}

public static Key key(boolean afterExecTransition) {
return Key.create(afterExecTransition);
}

/** {@link SkyKey} implementation used for {@link BaselineOptionsValue}. */
@CheckReturnValue
@Immutable
@ThreadSafe
@AutoValue
public abstract static class Key implements SkyKey {
public abstract boolean afterExecTransition();

@Override
public SkyFunctionName functionName() {
return SkyFunctions.BASELINE_OPTIONS;
}

static Key create(boolean afterExecTransition) {
return new AutoValue_BaselineOptionsValue_Key(afterExecTransition);
}
}
}
3 changes: 3 additions & 0 deletions src/main/java/com/google/devtools/build/lib/skyframe/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ java_library(
"ActionOutputDirectoryHelper.java",
"AspectCompletor.java",
"AspectFunction.java",
"BaselineOptionsFunction.java",
"BazelSkyframeExecutorConstants.java",
"BuildConfigurationFunction.java",
"BuildInfoCollectionFunction.java",
Expand Down Expand Up @@ -242,6 +243,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/analysis:config/config_conditions",
"//src/main/java/com/google/devtools/build/lib/analysis:config/config_matching_provider",
"//src/main/java/com/google/devtools/build/lib/analysis:config/core_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/execution_transition_factory",
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_factory",
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/host_transition",
Expand All @@ -250,6 +252,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/analysis:config/starlark_defined_config_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:config/starlark_transition_cache",
"//src/main/java/com/google/devtools/build/lib/analysis:config/toolchain_type_requirement",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/baseline_options_value",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/configuration_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/no_transition",
"//src/main/java/com/google/devtools/build/lib/analysis:config/transitions/null_transition",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
// 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.skyframe;

import com.google.devtools.build.lib.analysis.PlatformOptions;
import com.google.devtools.build.lib.analysis.config.BuildOptions;
import com.google.devtools.build.lib.analysis.config.ExecutionTransitionFactory;
import com.google.devtools.build.lib.analysis.config.transitions.BaselineOptionsValue;
import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
import com.google.devtools.build.lib.analysis.config.transitions.TransitionUtil;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyFunctionException;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
import com.google.devtools.common.options.OptionsParsingException;
import javax.annotation.Nullable;

/** A builder for {@link BaselineOptionsValue} instances. */
public final class BaselineOptionsFunction implements SkyFunction {
@Override
@Nullable
public SkyValue compute(SkyKey skyKey, Environment env)
throws InterruptedException, BaselineOptionsFunctionException {
BaselineOptionsValue.Key key = (BaselineOptionsValue.Key) skyKey.argument();

BuildOptions rawBaselineOptions = PrecomputedValue.BASELINE_CONFIGURATION.get(env);

// Some test infrastructure only creates mock or partial top-level BuildOptions such that
// PlatformOptions or even CoreOptions might not be included.
// In that case, is not worth doing any special processing of the baseline.
if (rawBaselineOptions.hasNoConfig()) {
return BaselineOptionsValue.create(rawBaselineOptions);
}

// Herein lies a hack to apply platform mappings to the baseline options.
// TODO(blaze-configurability-team): this should become unnecessary once --platforms is marked
// as EXPLICIT_IN_OUTPUT_PATH
PlatformMappingValue platformMappingValue =
(PlatformMappingValue)
env.getValue(
PlatformMappingValue.Key.create(
rawBaselineOptions.get(PlatformOptions.class).platformMappings));
if (platformMappingValue == null) {
return null;
}
try {
BuildOptions mappedBaselineOptions =
BuildConfigurationKey.withPlatformMapping(platformMappingValue, rawBaselineOptions)
.getOptions();

if (key.afterExecTransition()) {
// A null executionPlatform actually skips transition application so need some value here.
// It is safe to supply some fake value here (as long as it is constant) since the baseline
// should never be used to actually construct an action or do toolchain resolution
// TODO(twigg): This can eventually be replaced by the actual exec platform once
// platforms is explicitly in the output path (with the garbage value as a fallback).
PatchTransition execTransition =
ExecutionTransitionFactory.createTransition(
Label.parseCanonicalUnchecked(
"//this_is_a_faked_exec_platform_for_blaze_internals"));
BuildOptions toOptions =
execTransition.patch(
TransitionUtil.restrict(execTransition, mappedBaselineOptions), env.getListener());
return BaselineOptionsValue.create(toOptions);
} else {
return BaselineOptionsValue.create(mappedBaselineOptions);
}
} catch (OptionsParsingException e) {
throw new BaselineOptionsFunctionException(e);
}
}

private static final class BaselineOptionsFunctionException extends SkyFunctionException {
BaselineOptionsFunctionException(Exception e) {
super(e, Transience.PERSISTENT);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,13 @@
import com.google.common.collect.ImmutableSet;
import com.google.devtools.build.lib.analysis.BlazeDirectories;
import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
import com.google.devtools.build.lib.analysis.PlatformOptions;
import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
import com.google.devtools.build.lib.analysis.config.BuildOptions;
import com.google.devtools.build.lib.analysis.config.CoreOptions;
import com.google.devtools.build.lib.analysis.config.FragmentFactory;
import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
import com.google.devtools.build.lib.analysis.config.OptionInfo;
import com.google.devtools.build.lib.analysis.config.transitions.BaselineOptionsValue;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.cmdline.RepositoryName;
import com.google.devtools.build.lib.packages.RuleClassProvider;
Expand All @@ -41,7 +41,6 @@
import com.google.devtools.build.skyframe.SkyValue;
import com.google.devtools.common.options.OptionDefinition;
import com.google.devtools.common.options.OptionMetadataTag;
import com.google.devtools.common.options.OptionsParsingException;
import java.util.Map;
import java.util.TreeMap;
import javax.annotation.Nullable;
Expand Down Expand Up @@ -81,33 +80,23 @@ public SkyValue compute(SkyKey skyKey, Environment env)
BuildConfigurationKey key = (BuildConfigurationKey) skyKey.argument();

BuildOptions targetOptions = key.getOptions();
CoreOptions coreOptions = targetOptions.get(CoreOptions.class);

String transitionDirectoryNameFragment;
if (targetOptions
.get(CoreOptions.class)
.outputDirectoryNamingScheme
.equals(CoreOptions.OutputDirectoryNamingScheme.DIFF_AGAINST_BASELINE)) {
// Herein lies a hack to apply platform mappings to the baseline options.
// TODO(blaze-configurability-team): this should become unnecessary once --platforms is marked
// as EXPLICIT_IN_OUTPUT_PATH
PlatformMappingValue platformMappingValue =
(PlatformMappingValue)
env.getValue(
PlatformMappingValue.Key.create(
targetOptions.get(PlatformOptions.class).platformMappings));
if (platformMappingValue == null) {
if (coreOptions.useBaselineForOutputDirectoryNamingScheme()) {
boolean applyExecTransitionToBaseline =
coreOptions.outputDirectoryNamingScheme.equals(
CoreOptions.OutputDirectoryNamingScheme.DIFF_AGAINST_DYNAMIC_BASELINE)
&& coreOptions.isExec;
var baselineOptionsValue =
(BaselineOptionsValue)
env.getValue(BaselineOptionsValue.key(applyExecTransitionToBaseline));
if (baselineOptionsValue == null) {
return null;
}
BuildOptions baselineOptions = PrecomputedValue.BASELINE_CONFIGURATION.get(env);
try {
BuildOptions mappedBaselineOptions =
BuildConfigurationKey.withPlatformMapping(platformMappingValue, baselineOptions)
.getOptions();
transitionDirectoryNameFragment =
computeNameFragmentWithDiff(targetOptions, mappedBaselineOptions);
} catch (OptionsParsingException e) {
throw new BuildConfigurationFunctionException(e);
}

transitionDirectoryNameFragment =
computeNameFragmentWithDiff(targetOptions, baselineOptionsValue.toOptions());
} else {
transitionDirectoryNameFragment =
computeNameFragmentWithAffectedByStarlarkTransition(targetOptions);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ public final class SkyFunctions {
static final SkyFunctionName TEST_COMPLETION = SkyFunctionName.createHermetic("TEST_COMPLETION");
public static final SkyFunctionName BUILD_CONFIGURATION =
SkyFunctionName.createHermetic("BUILD_CONFIGURATION");
public static final SkyFunctionName BASELINE_OPTIONS =
SkyFunctionName.createHermetic("BASELINE_OPTIONS");
public static final SkyFunctionName STARLARK_BUILD_SETTINGS_DETAILS =
SkyFunctionName.createHermetic("STARLARK_BUILD_SETTINGS_DETAILS");
// Action execution can be nondeterministic, so semi-hermetic.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -592,6 +592,7 @@ private ImmutableMap<SkyFunctionName, SkyFunction> skyFunctions() {
map.put(
SkyFunctions.BUILD_CONFIGURATION,
new BuildConfigurationFunction(directories, ruleClassProvider));
map.put(SkyFunctions.BASELINE_OPTIONS, new BaselineOptionsFunction());
map.put(
SkyFunctions.STARLARK_BUILD_SETTINGS_DETAILS, new StarlarkBuildSettingsDetailsFunction());
map.put(SkyFunctions.WORKSPACE_NAME, new WorkspaceNameFunction());
Expand Down
Loading

0 comments on commit 76a4ad7

Please sign in to comment.