Skip to content

Commit

Permalink
Automated rollback of commit 2a3ab5c.
Browse files Browse the repository at this point in the history
*** Reason for rollback ***

Causing crashes, e.g. []

*** Original change description ***

Enable heuristically dropping GENQUERY_SCOPE nodes

These are often not used after their parent genquery's configured target
value is evaluated.

PiperOrigin-RevId: 519156527
Change-Id: I08d8dfdceb9d3748c84eb9b18f8afdf57f199096
  • Loading branch information
anakanemison authored and copybara-github committed Mar 24, 2023
1 parent 33d2dc9 commit 7917b9a
Show file tree
Hide file tree
Showing 5 changed files with 13 additions and 52 deletions.
17 changes: 5 additions & 12 deletions src/main/java/com/google/devtools/build/lib/rules/genquery/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -17,19 +17,22 @@ java_library(
name = "genquery",
srcs = [
"GenQuery.java",
"GenQueryConfiguration.java",
"GenQueryOutputStream.java",
"GenQueryRule.java",
],
deps = [
":genquery_configuration",
":genquery_package_providers",
"//src/main/java/com/google/devtools/build/lib:keep-going-option",
"//src/main/java/com/google/devtools/build/lib/actions",
"//src/main/java/com/google/devtools/build/lib/actions:artifacts",
"//src/main/java/com/google/devtools/build/lib/analysis:actions/abstract_file_write_action",
"//src/main/java/com/google/devtools/build/lib/analysis:actions/deterministic_writer",
"//src/main/java/com/google/devtools/build/lib/analysis:analysis_cluster",
"//src/main/java/com/google/devtools/build/lib/analysis:config/build_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/core_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment",
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options",
"//src/main/java/com/google/devtools/build/lib/analysis:configured_target",
"//src/main/java/com/google/devtools/build/lib/analysis:rule_definition_environment",
"//src/main/java/com/google/devtools/build/lib/cmdline",
Expand Down Expand Up @@ -61,17 +64,6 @@ java_library(
],
)

java_library(
name = "genquery_configuration",
srcs = ["GenQueryConfiguration.java"],
deps = [
"//src/main/java/com/google/devtools/build/lib/analysis:config/build_options",
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment",
"//src/main/java/com/google/devtools/build/lib/analysis:config/fragment_options",
"//src/main/java/com/google/devtools/common/options",
],
)

java_library(
name = "genquery_package_providers",
srcs = [
Expand All @@ -84,6 +76,7 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/bugreport",
"//src/main/java/com/google/devtools/build/lib/cmdline",
"//src/main/java/com/google/devtools/build/lib/collect/nestedset",
"//src/main/java/com/google/devtools/build/lib/concurrent",
"//src/main/java/com/google/devtools/build/lib/events",
"//src/main/java/com/google/devtools/build/lib/packages",
"//src/main/java/com/google/devtools/build/lib/pkgcache",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ public class GenQueryDirectPackageProviderFactory implements GenQueryPackageProv
* rules sharing the same scope will require only one scope traversal to occur.
*/
@AutoCodec
public static class Key extends AbstractSkyKey.WithCachedHashCode<ImmutableList<Label>> {
static class Key extends AbstractSkyKey.WithCachedHashCode<ImmutableList<Label>> {
private static final SkyKeyInterner<Key> interner = SkyKey.newInterner();

private Key(ImmutableList<Label> arg) {
Expand Down
3 changes: 1 addition & 2 deletions src/main/java/com/google/devtools/build/lib/skyframe/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@ java_library(
":ignored_package_prefixes_function",
":ignored_package_prefixes_value",
":incremental_artifact_conflict_finder",
":incremental_package_roots",
":loading_phase_started_event",
":local_repository_lookup_value",
":map_as_package_roots",
Expand Down Expand Up @@ -330,7 +331,6 @@ java_library(
"//src/main/java/com/google/devtools/build/lib/rules:repository/repository_function",
"//src/main/java/com/google/devtools/build/lib/rules:repository/resolved_file_value",
"//src/main/java/com/google/devtools/build/lib/rules:repository/workspace_file_helper",
"//src/main/java/com/google/devtools/build/lib/rules/genquery:genquery_configuration",
"//src/main/java/com/google/devtools/build/lib/rules/genquery:genquery_package_providers",
"//src/main/java/com/google/devtools/build/lib/skyframe/actiongraph/v2:actiongraph_v2",
"//src/main/java/com/google/devtools/build/lib/skyframe/rewinding",
Expand Down Expand Up @@ -3035,7 +3035,6 @@ java_library(
deps = [
":sky_functions",
"//src/main/java/com/google/devtools/build/lib/actions:file_metadata",
"//src/main/java/com/google/devtools/build/lib/rules/genquery:genquery_package_providers",
"//src/main/java/com/google/devtools/build/lib/vfs",
"//src/main/java/com/google/devtools/build/skyframe",
"//src/main/java/com/google/devtools/build/skyframe:graph_inconsistency_java_proto",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,7 @@

import static com.google.common.base.Preconditions.checkState;

import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.actions.FileValue;
import com.google.devtools.build.lib.rules.genquery.GenQueryDirectPackageProviderFactory;
import com.google.devtools.build.lib.vfs.FileStateKey;
import com.google.devtools.build.skyframe.GraphInconsistencyReceiver;
import com.google.devtools.build.skyframe.SkyFunctionName;
Expand All @@ -38,12 +36,6 @@
*/
public class NodeDroppingInconsistencyReceiver implements GraphInconsistencyReceiver {

private static final ImmutableMap<SkyFunctionName, SkyFunctionName> EXPECTED_MISSING_CHILDREN =
ImmutableMap.of(
FileValue.FILE, FileStateKey.FILE_STATE,
SkyFunctions.DIRECTORY_LISTING, SkyFunctions.DIRECTORY_LISTING_STATE,
SkyFunctions.CONFIGURED_TARGET, GenQueryDirectPackageProviderFactory.GENQUERY_SCOPE);

@Override
public void noteInconsistencyAndMaybeThrow(
SkyKey key, @Nullable Collection<SkyKey> otherKeys, Inconsistency inconsistency) {
Expand All @@ -61,8 +53,9 @@ public void noteInconsistencyAndMaybeThrow(
*/
public static boolean isExpectedInconsistency(
SkyKey key, @Nullable Collection<SkyKey> otherKeys, Inconsistency inconsistency) {
SkyFunctionName expectedMissingChildType = EXPECTED_MISSING_CHILDREN.get(key.functionName());
if (expectedMissingChildType == null) {
boolean isFileKey = key.functionName().equals(FileValue.FILE);
boolean isDirectoryListingKey = key.functionName().equals(SkyFunctions.DIRECTORY_LISTING);
if (!isFileKey && !isDirectoryListingKey) {
return false;
}
if (inconsistency == Inconsistency.RESET_REQUESTED) {
Expand All @@ -72,7 +65,9 @@ public static boolean isExpectedInconsistency(
|| inconsistency == Inconsistency.BUILDING_PARENT_FOUND_UNDONE_CHILD) {
// For already declared child missing inconsistency, key is the parent while `otherKeys`
// are the children (dependency nodes).
return otherKeys.stream().allMatch(SkyFunctionName.functionIs(expectedMissingChildType));
SkyFunctionName expectedStateKeyType =
isFileKey ? FileStateKey.FILE_STATE : SkyFunctions.DIRECTORY_LISTING_STATE;
return otherKeys.stream().allMatch(SkyFunctionName.functionIs(expectedStateKeyType));
}
return false;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,6 @@
import com.google.devtools.build.lib.analysis.config.transitions.NoTransition;
import com.google.devtools.build.lib.analysis.config.transitions.NullTransition;
import com.google.devtools.build.lib.analysis.configuredtargets.MergedConfiguredTarget;
import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget;
import com.google.devtools.build.lib.analysis.constraints.RuleContextConstraintSemantics;
import com.google.devtools.build.lib.analysis.starlark.StarlarkBuildSettingsDetailsValue;
import com.google.devtools.build.lib.analysis.starlark.StarlarkTransition;
Expand Down Expand Up @@ -158,7 +157,6 @@
import com.google.devtools.build.lib.remote.options.RemoteOptions;
import com.google.devtools.build.lib.remote.options.RemoteOutputsMode;
import com.google.devtools.build.lib.repository.ExternalPackageHelper;
import com.google.devtools.build.lib.rules.genquery.GenQueryConfiguration.GenQueryOptions;
import com.google.devtools.build.lib.rules.genquery.GenQueryDirectPackageProviderFactory;
import com.google.devtools.build.lib.rules.repository.ResolvedFileFunction;
import com.google.devtools.build.lib.rules.repository.ResolvedHashesFunction;
Expand Down Expand Up @@ -3119,9 +3117,6 @@ public void evaluated(
argument);
SkyKey directoryListingStateKey = DirectoryListingStateValue.key((RootedPath) argument);
memoizingEvaluator.getInMemoryGraph().remove(directoryListingStateKey);
} else if (directDeps != null
&& skyKey.functionName().equals(SkyFunctions.CONFIGURED_TARGET)) {
maybeDropGenQueryDep(newValue, directDeps);
}
}

Expand Down Expand Up @@ -3178,27 +3173,6 @@ public void evaluated(
}
}

private void maybeDropGenQueryDep(SkyValue newValue, GroupedDeps directDeps) {
if (!(newValue instanceof RuleConfiguredTargetValue)) {
return;
}
var t = (RuleConfiguredTarget) ((RuleConfiguredTargetValue) newValue).getConfiguredTarget();
if (!t.getRuleClassString().equals("genquery")) {
return;
}
BuildOptions options = t.getConfigurationKey().getOptions();
if (!options.contains(GenQueryOptions.class)
|| !options.get(GenQueryOptions.class).skipTtvs) {
return;
}
for (SkyKey key : directDeps.getAllElementsAsIterable()) {
if (key instanceof GenQueryDirectPackageProviderFactory.Key) {
memoizingEvaluator.getInMemoryGraph().remove(key);
return;
}
}
}

private void recursivelyRemoveGlobFromGraph(GlobDescriptor root) {
memoizingEvaluator.getInMemoryGraph().remove(root);
ImmutableList<GlobDescriptor> adjacentDeps = globDeps.remove(root);
Expand Down

0 comments on commit 7917b9a

Please sign in to comment.