Add proper serialization constructor and equals/hashCode for EnvironmentLabels. PiperOrigin-RevId: 188078054
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/constraints/EnvironmentCollection.java b/src/main/java/com/google/devtools/build/lib/analysis/constraints/EnvironmentCollection.java index c1ce9f0..430078e 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/constraints/EnvironmentCollection.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/constraints/EnvironmentCollection.java
@@ -15,6 +15,7 @@ package com.google.devtools.build.lib.analysis.constraints; import com.google.auto.value.AutoValue; +import com.google.common.base.MoreObjects; import com.google.common.collect.ImmutableCollection; import com.google.common.collect.ImmutableMultimap; import com.google.common.collect.ImmutableSet; @@ -89,6 +90,15 @@ /** An empty collection. */ static final EnvironmentCollection EMPTY = new EnvironmentCollection(ImmutableMultimap.of()); + @Override + public String toString() { + return MoreObjects.toStringHelper(this) + .add("size", map.size()) + .add("hashCode", map.hashCode()) + .add("map", map) + .toString(); + } + /** * Builder for {@link EnvironmentCollection}. */
diff --git a/src/main/java/com/google/devtools/build/lib/packages/EnvironmentGroup.java b/src/main/java/com/google/devtools/build/lib/packages/EnvironmentGroup.java index a0e8b23..34090af 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/EnvironmentGroup.java +++ b/src/main/java/com/google/devtools/build/lib/packages/EnvironmentGroup.java
@@ -15,7 +15,6 @@ package com.google.devtools.build.lib.packages; import com.google.common.base.Predicate; -import com.google.common.base.Verify; import com.google.common.collect.HashMultimap; import com.google.common.collect.Iterables; import com.google.common.collect.Multimap; @@ -29,28 +28,26 @@ import com.google.devtools.build.lib.events.Location; import java.util.ArrayList; import java.util.Collections; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Set; /** * Model for the "environment_group' rule: the piece of Bazel's rule constraint system that binds - * thematically related environments together and determines which environments a rule supports - * by default. See {@link com.google.devtools.build.lib.analysis.constraints.ConstraintSemantics} - * for precise semantic details of how this information is used. + * thematically related environments together and determines which environments a rule supports by + * default. See {@link com.google.devtools.build.lib.analysis.constraints.ConstraintSemantics} for + * precise semantic details of how this information is used. * * <p>Note that "environment_group" is implemented as a loading-time function, not a rule. This is - * to support proper discovery of defaults: Say rule A has no explicit constraints and depends - * on rule B, which is explicitly constrained to environment ":bar". Since A declares nothing - * explicitly, it's implicitly constrained to DEFAULTS (whatever that is). Therefore, the - * dependency is only allowed if DEFAULTS doesn't include environments beyond ":bar". To figure - * that out, we need to be able to look up the environment group for ":bar", which is what this - * class provides. + * to support proper discovery of defaults: Say rule A has no explicit constraints and depends on + * rule B, which is explicitly constrained to environment ":bar". Since A declares nothing + * explicitly, it's implicitly constrained to DEFAULTS (whatever that is). Therefore, the dependency + * is only allowed if DEFAULTS doesn't include environments beyond ":bar". To figure that out, we + * need to be able to look up the environment group for ":bar", which is what this class provides. * - * <p>If we implemented this as a rule, we'd have to provide that lookup via rule dependencies, - * e.g. something like: - * - * <code> + * <p>If we implemented this as a rule, we'd have to provide that lookup via rule dependencies, e.g. + * something like: <code> * environment( * name = 'bar', * group = [':sample_environments'], @@ -62,7 +59,7 @@ * to determine what other environments belong to the group is to have the group somehow reference * them. That would produce circular dependencies in the build graph, which is no good. */ -@Immutable +@Immutable // This is a lie, but this object is only mutable until its containing package is loaded. public class EnvironmentGroup implements Target { private final EnvironmentLabels environmentLabels; private final Location location; @@ -106,6 +103,7 @@ } public EnvironmentLabels getEnvironmentLabels() { + environmentLabels.checkInitialized(); return environmentLabels; } @@ -167,15 +165,17 @@ } } + Map<Label, NestedSet<Label>> fulfillersMap = new HashMap<>(); // Now that we know which environments directly fulfill each other, compute which environments // transitively fulfill each other. We could alternatively compute this on-demand, but since // we don't expect these chains to be very large we opt toward computing them once at package // load time. - Verify.verify(environmentLabels.fulfillersMap.isEmpty()); + environmentLabels.assertNotInitialized(); for (Label envName : environmentLabels.environments) { - setTransitiveFulfillers(envName, directFulfillers, environmentLabels.fulfillersMap); + setTransitiveFulfillers(envName, directFulfillers, fulfillersMap); } + environmentLabels.setFulfillersMap(fulfillersMap); return events; }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/EnvironmentLabels.java b/src/main/java/com/google/devtools/build/lib/packages/EnvironmentLabels.java index e756a8e..c375c7a 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/EnvironmentLabels.java +++ b/src/main/java/com/google/devtools/build/lib/packages/EnvironmentLabels.java
@@ -14,19 +14,26 @@ package com.google.devtools.build.lib.packages; -import com.google.common.base.Verify; +import com.google.common.base.MoreObjects; +import com.google.common.base.Preconditions; import com.google.common.collect.ImmutableSet; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.nestedset.NestedSet; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import java.util.Collection; -import java.util.HashMap; +import java.util.Collections; import java.util.Map; +import java.util.Objects; import java.util.Set; /** * Parts of an {@link EnvironmentGroup} that are needed for analysis. Since {@link EnvironmentGroup} * keeps a reference to a {@link Package} object, it is too heavyweight to store in analysis. + * + * <p>Constructor should only be called by {@link EnvironmentGroup}, and this object must never be + * accessed externally until after {@link EnvironmentGroup#processMemberEnvironments} is called. The + * mutability of fulfillersMap means that we must take care to wait until it is set before doing + * anything with this class. */ @AutoCodec public class EnvironmentLabels { @@ -35,22 +42,52 @@ final ImmutableSet<Label> defaults; /** * Maps a member environment to the set of environments that directly fulfill it. Note that we - * can't populate this map until all Target instances for member environments have been - * initialized, which may occur after group instantiation (this makes the class mutable). + * can't set this map until all Target instances for member environments have been initialized, + * which occurs after group instantiation (this makes the class mutable). */ - final Map<Label, NestedSet<Label>> fulfillersMap = new HashMap<>(); + private Map<Label, NestedSet<Label>> fulfillersMap = null; EnvironmentLabels(Label label, Collection<Label> environments, Collection<Label> defaults) { + this(label, environments, defaults, null); + } + + /** + * Only for use by serialization: the mutable fulfillersMap object is not properly initialized + * otherwise during deserialization. + */ + @AutoCodec.VisibleForSerialization + @AutoCodec.Instantiator + EnvironmentLabels( + Label label, + Collection<Label> environments, + Collection<Label> defaults, + Map<Label, NestedSet<Label>> fulfillersMap) { this.label = label; this.environments = ImmutableSet.copyOf(environments); this.defaults = ImmutableSet.copyOf(defaults); + this.fulfillersMap = fulfillersMap; + } + + void assertNotInitialized() { + Preconditions.checkState(fulfillersMap == null, this); + } + + void checkInitialized() { + Preconditions.checkNotNull(fulfillersMap, this); + } + + void setFulfillersMap(Map<Label, NestedSet<Label>> fulfillersMap) { + Preconditions.checkState(this.fulfillersMap == null, this); + this.fulfillersMap = Collections.unmodifiableMap(fulfillersMap); } public Set<Label> getEnvironments() { + checkInitialized(); return environments; } public Set<Label> getDefaults() { + checkInitialized(); return defaults; } @@ -59,6 +96,7 @@ * belong to this group. */ public boolean isDefault(Label environment) { + checkInitialized(); return defaults.contains(environment); } @@ -73,10 +111,79 @@ * <p>If no environments fulfill the input, returns an empty set. */ public Iterable<Label> getFulfillers(Label environment) { - return Verify.verifyNotNull(fulfillersMap.get(environment)); + checkInitialized(); + return fulfillersMap.get(environment); } public Label getLabel() { + checkInitialized(); return label; } + + @Override + public String toString() { + return MoreObjects.toStringHelper(this) + .add("label", label) + .add("sizes", environments.size() + ", " + defaults.size() + ", " + fulfillersMap.size()) + .add("environments", environments) + .add("defaults", defaults) + .add("fulfillersMap", fulfillersMap) + .toString(); + } + + @Override + public int hashCode() { + checkInitialized(); + return Objects.hash(label, environments, defaults, fulfillersMap.keySet()); + } + + /** + * Compares {@code map1} and {@code map2} using deep equality for their values. Should be feasible + * because comparison will usually only happen between == objects, so this is hit rarely. If + * objects are equal, but have been deserialized separately so not ==, this should still be ok + * because these nested sets are not particularly big, and there are very few EnvironmentGroups + * (and therefore EnvironmentLabels) in any given build. + * + * <p>This will have to be revisited if it turns out to be noticeably expensive. It should be + * sound to not compare the values of the fulfillerMaps at all, since they are determined from the + * package each EnvironmentLabel is associated with, and so as long as EnvironmentLabels from + * different source states but the same package are not compared, the values shouldn't be + * necessary. + */ + private static boolean fulfillerMapsEqual( + Map<Label, NestedSet<Label>> map1, Map<Label, NestedSet<Label>> map2) { + if (map1 == map2) { + return true; + } + if (map1.size() != map2.size()) { + return false; + } + for (Map.Entry<Label, NestedSet<Label>> entry : map1.entrySet()) { + NestedSet<Label> secondValue = map2.get(entry.getKey()); + // Do shallowEquals check first for speed. + if (secondValue == null + || (!entry.getValue().shallowEquals(secondValue) + && !entry.getValue().toCollection().equals(secondValue.toCollection()))) { + return false; + } + } + return true; + } + + @Override + public boolean equals(Object o) { + checkInitialized(); + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + EnvironmentLabels that = (EnvironmentLabels) o; + that.checkInitialized(); + return label.equals(that.label) + && environments.equals(that.environments) + && defaults.equals(that.defaults) + && fulfillerMapsEqual(this.fulfillersMap, that.fulfillersMap); + } }