Skip to content

Commit afe070e

Browse files
buildbreaker2021copybara-github
authored andcommitted
Get rid of Skyframe interaction while calculating --fdo_optimize flag value.
This is required to avoid [Skyframe restart](https://github.com/bazelbuild/bazel/blob/0dd106e83ccbc149e31ef9ffeab76ab5f1bffdc6/src/main/java/com/google/devtools/build/lib/rules/cpp/CcToolchain.java#L109-L112) in `cc_toolchain` rule. After some research as it appears the only reason a custom `SkyFunction` was introduced for `--fdo_optimize` flag is to calculate full path in case its value was specified as a relative path to the workspace directory(interaction in [FdoHelper](https://github.com/bazelbuild/bazel/blob/0dd106e83ccbc149e31ef9ffeab76ab5f1bffdc6/src/main/java/com/google/devtools/build/lib/rules/cpp/FdoHelper.java#L84-L96) and full path [calculation](https://github.com/bazelbuild/bazel/blob/0dd106e83ccbc149e31ef9ffeab76ab5f1bffdc6/src/main/java/com/google/devtools/build/lib/rules/cpp/CcSkyframeFdoSupportFunction.java#L54)). This is a hard blocker for Starlarkification of cc_toolchain rule and also not ideal to have FDO specific `SkyFunction` baked into `Bazel`. Therefore the decision to *not* support relative paths for `--fdo_optimize` flag can be justified. This is a breaking change however relative paths should be easy to replace with `labels` to [`fdo_profile`](https://bazel.build/reference/be/c-cpp#fdo_profile) targets or fully qualified paths. PiperOrigin-RevId: 526912841 Change-Id: I392cb8dee5d18398d8db643d0899f554b3542d7a
1 parent 60c9cf3 commit afe070e

File tree

11 files changed

+25
-215
lines changed

11 files changed

+25
-215
lines changed

src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRulesModule.java

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -15,23 +15,18 @@
1515
package com.google.devtools.build.lib.bazel.rules;
1616

1717
import com.google.common.collect.ImmutableList;
18-
import com.google.devtools.build.lib.analysis.BlazeDirectories;
1918
import com.google.devtools.build.lib.analysis.ConfiguredRuleClassProvider;
2019
import com.google.devtools.build.lib.bazel.rules.cpp.BazelCppRuleClasses;
2120
import com.google.devtools.build.lib.bazel.rules.sh.BazelShRuleClasses;
2221
import com.google.devtools.build.lib.buildtool.BuildRequest;
2322
import com.google.devtools.build.lib.exec.ModuleActionContextRegistry;
2423
import com.google.devtools.build.lib.remote.options.RemoteOptions;
25-
import com.google.devtools.build.lib.rules.cpp.CcSkyframeFdoSupportFunction;
26-
import com.google.devtools.build.lib.rules.cpp.CcSkyframeFdoSupportValue;
2724
import com.google.devtools.build.lib.rules.cpp.CppOptions;
2825
import com.google.devtools.build.lib.rules.java.JavaCompileActionContext;
2926
import com.google.devtools.build.lib.rules.java.JavaOptions;
3027
import com.google.devtools.build.lib.runtime.BlazeModule;
31-
import com.google.devtools.build.lib.runtime.BlazeRuntime;
3228
import com.google.devtools.build.lib.runtime.Command;
3329
import com.google.devtools.build.lib.runtime.CommandEnvironment;
34-
import com.google.devtools.build.lib.runtime.WorkspaceBuilder;
3530
import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
3631
import com.google.devtools.build.lib.server.FailureDetails.RemoteExecution;
3732
import com.google.devtools.build.lib.server.FailureDetails.RemoteExecution.Code;
@@ -556,13 +551,6 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
556551
validateRemoteOutputsMode(env);
557552
}
558553

559-
@Override
560-
public void workspaceInit(
561-
BlazeRuntime runtime, BlazeDirectories directories, WorkspaceBuilder builder) {
562-
builder.addSkyFunction(
563-
CcSkyframeFdoSupportValue.SKYFUNCTION, new CcSkyframeFdoSupportFunction(directories));
564-
}
565-
566554
@Override
567555
public Iterable<Class<? extends OptionsBase>> getCommandOptions(Command command) {
568556
return "build".equals(command.name())

src/main/java/com/google/devtools/build/lib/rules/cpp/CcSkyframeFdoSupportFunction.java

Lines changed: 0 additions & 58 deletions
This file was deleted.

src/main/java/com/google/devtools/build/lib/rules/cpp/CcSkyframeFdoSupportValue.java

Lines changed: 0 additions & 107 deletions
This file was deleted.

src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -211,6 +211,12 @@ public CppConfiguration(BuildOptions options) throws InvalidConfigurationExcepti
211211
}
212212
} else {
213213
fdoPath = PathFragment.create(cppOptions.getFdoOptimize());
214+
if (!fdoPath.isAbsolute()) {
215+
throw new InvalidConfigurationException(
216+
"Path of '"
217+
+ fdoPath.getPathString()
218+
+ "' in --fdo_optimize has to be either an absolute path or a label.");
219+
}
214220
try {
215221
// We don't check for file existence, but at least the filename should be well-formed.
216222
FileSystemUtils.checkBaseName(fdoPath.getBaseName());

src/main/java/com/google/devtools/build/lib/rules/cpp/FdoHelper.java

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,6 @@
3131
import com.google.devtools.build.lib.util.Pair;
3232
import com.google.devtools.build.lib.vfs.FileSystemUtils;
3333
import com.google.devtools.build.lib.vfs.PathFragment;
34-
import com.google.devtools.build.skyframe.SkyFunction;
35-
import com.google.devtools.build.skyframe.SkyKey;
3634
import javax.annotation.Nullable;
3735

3836
/** Helper responsible for creating {@link FdoContext} */
@@ -82,18 +80,10 @@ public static FdoContext getFdoContext(
8280

8381
if (cppConfiguration.getFdoPath() != null) {
8482
PathFragment fdoZip = cppConfiguration.getFdoPath();
85-
SkyKey fdoKey = CcSkyframeFdoSupportValue.key(fdoZip);
86-
SkyFunction.Environment skyframeEnv = ruleContext.getAnalysisEnvironment().getSkyframeEnv();
87-
CcSkyframeFdoSupportValue ccSkyframeFdoSupportValue =
88-
(CcSkyframeFdoSupportValue) skyframeEnv.getValue(fdoKey);
89-
if (skyframeEnv.valuesMissing()) {
90-
return null;
91-
}
9283
// fdoZip should be set if the profile is a path, fdoInputFile if it is an artifact, but
9384
// never both
9485
Preconditions.checkState(fdoInputFile == null);
95-
fdoInputFile =
96-
FdoInputFile.fromAbsolutePath(ccSkyframeFdoSupportValue.getFdoZipPath().asFragment());
86+
fdoInputFile = FdoInputFile.fromAbsolutePath(fdoZip);
9787
} else if (cppConfiguration.getFdoOptimizeLabel() != null) {
9888
FdoProfileProvider fdoProfileProvider = attributes.getFdoOptimizeProvider();
9989
if (fdoProfileProvider != null) {

src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisMock.java

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,6 @@
3131
import com.google.devtools.build.lib.packages.util.MockCcSupport;
3232
import com.google.devtools.build.lib.packages.util.MockPythonSupport;
3333
import com.google.devtools.build.lib.packages.util.MockToolsConfig;
34-
import com.google.devtools.build.lib.rules.cpp.CcSkyframeFdoSupportFunction;
35-
import com.google.devtools.build.lib.rules.cpp.CcSkyframeFdoSupportValue;
3634
import com.google.devtools.build.lib.rules.repository.LocalRepositoryFunction;
3735
import com.google.devtools.build.lib.rules.repository.LocalRepositoryRule;
3836
import com.google.devtools.build.lib.rules.repository.RepositoryDelegatorFunction;
@@ -149,9 +147,7 @@ public ImmutableMap<SkyFunctionName, SkyFunction> getSkyFunctions(BlazeDirectori
149147
SkyFunctions.BAZEL_MODULE_RESOLUTION,
150148
new BazelModuleResolutionFunction(),
151149
SkyFunctions.CLIENT_ENVIRONMENT_VARIABLE,
152-
new ClientEnvironmentFunction(new AtomicReference<>(ImmutableMap.of())),
153-
CcSkyframeFdoSupportValue.SKYFUNCTION,
154-
new CcSkyframeFdoSupportFunction(directories));
150+
new ClientEnvironmentFunction(new AtomicReference<>(ImmutableMap.of())));
155151
}
156152

157153
// Allow subclasses to add extra repository functions.

src/test/java/com/google/devtools/build/lib/rules/cpp/CcBinaryFSAFDOTest.java

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ private LtoBackendAction setupAndRunToolchainActions(String... config) throws Ex
6767
MockCcSupport.FSAFDO));
6868

6969
List<String> testConfig =
70-
Lists.newArrayList("--fdo_optimize=pkg/profile.afdo", "--compilation_mode=opt");
70+
Lists.newArrayList("--fdo_optimize=/pkg/profile.afdo", "--compilation_mode=opt");
7171
Collections.addAll(testConfig, config);
7272
useConfiguration(Iterables.toArray(testConfig, String.class));
7373

@@ -97,7 +97,6 @@ public void fsafdoEnabledWithImplicit() throws Exception {
9797
" srcs = ['binfile.cc', ],",
9898
" malloc = '//base:system_malloc')");
9999
scratch.file("pkg/binfile.cc", "int main() {}");
100-
scratch.file("pkg/profile.afdo", "");
101100

102101
LtoBackendAction backendAction = setupAndRunToolchainActions("--features=implicit_fsafdo");
103102

@@ -115,7 +114,6 @@ public void fsafdoEnabledWithFeatureWithoutImplicit() throws Exception {
115114
" srcs = ['binfile.cc', ],",
116115
" malloc = '//base:system_malloc')");
117116
scratch.file("pkg/binfile.cc", "int main() {}");
118-
scratch.file("pkg/profile.afdo", "");
119117

120118
LtoBackendAction backendAction =
121119
setupAndRunToolchainActions("--features=-implicit_fsafdo", "--features=fsafdo");
@@ -134,7 +132,6 @@ public void fsafdoEnabledWithExplicitFeature() throws Exception {
134132
" srcs = ['binfile.cc', ],",
135133
" malloc = '//base:system_malloc')");
136134
scratch.file("pkg/binfile.cc", "int main() {}");
137-
scratch.file("pkg/profile.afdo", "");
138135

139136
LtoBackendAction backendAction = setupAndRunToolchainActions("--features=fsafdo");
140137

@@ -152,7 +149,6 @@ public void fsafdoDisabledWithFeatureWithoutImplicit() throws Exception {
152149
" srcs = ['binfile.cc', ],",
153150
" malloc = '//base:system_malloc')");
154151
scratch.file("pkg/binfile.cc", "int main() {}");
155-
scratch.file("pkg/profile.afdo", "");
156152

157153
LtoBackendAction backendAction = setupAndRunToolchainActions();
158154

@@ -173,7 +169,6 @@ public void fsafdoDisabledWithExplicitFeature() throws Exception {
173169
" srcs = ['binfile.cc', ],",
174170
" malloc = '//base:system_malloc')");
175171
scratch.file("pkg/binfile.cc", "int main() {}");
176-
scratch.file("pkg/profile.afdo", "");
177172

178173
LtoBackendAction backendAction =
179174
setupAndRunToolchainActions("--features=implicit_fsafdo", "--features=-fsafdo");

src/test/java/com/google/devtools/build/lib/rules/cpp/CcBinarySplitFunctionsTest.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ private LtoBackendAction setupAndRunToolchainActions(String... config) throws Ex
6767
MockCcSupport.SPLIT_FUNCTIONS));
6868

6969
List<String> testConfig =
70-
Lists.newArrayList("--fdo_optimize=pkg/profile.zip", "--compilation_mode=opt");
70+
Lists.newArrayList("--fdo_optimize=/pkg/profile.zip", "--compilation_mode=opt");
7171
Collections.addAll(testConfig, config);
7272
useConfiguration(Iterables.toArray(testConfig, String.class));
7373

0 commit comments

Comments
 (0)