Skip to content

Commit 7a839f6

Browse files
lberkicopybara-github
lberki
authored and
copybara-github
committed
Make the list of implicit deps deterministic.
There were two issues: - The iteration order of the result of Util.findImplicitDeps() depended on hash codes due to the use of HashSet - Two ImmutableSets with the same contents but different iteration order are .equals(), which means that interning could change the iteration order of RuleConfiguredTarget.getImplicitDeps(). RELNOTES: None. PiperOrigin-RevId: 865508372 Change-Id: I7472fbad5d26d18a2ddc0ec214e2f00a6c17a5ab
1 parent 8ca49ef commit 7a839f6

File tree

5 files changed

+73
-73
lines changed

5 files changed

+73
-73
lines changed

‎src/main/java/com/google/devtools/build/lib/analysis/TransitiveDependencyState.java‎

Lines changed: 2 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -13,27 +13,20 @@
1313
// limitations under the License.
1414
package com.google.devtools.build.lib.analysis;
1515

16-
import static com.google.common.collect.Comparators.lexicographical;
1716
import static java.util.Comparator.comparing;
18-
import static java.util.Comparator.naturalOrder;
19-
import static java.util.Comparator.nullsFirst;
2017

2118
import com.google.devtools.build.lib.causes.Cause;
2219
import com.google.devtools.build.lib.cmdline.PackageIdentifier;
2320
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
2421
import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
25-
import com.google.devtools.build.lib.packages.AspectDescriptor;
2622
import com.google.devtools.build.lib.packages.Package;
2723
import com.google.devtools.build.lib.packages.Target;
2824
import com.google.devtools.build.lib.skyframe.AspectKeyCreator.AspectKey;
2925
import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData;
3026
import com.google.devtools.build.lib.skyframe.ConfiguredTargetKey;
3127
import com.google.devtools.build.lib.skyframe.PrerequisitePackageFunction;
32-
import com.google.devtools.build.lib.skyframe.config.BuildConfigurationKey;
3328
import java.util.ArrayList;
3429
import java.util.Collections;
35-
import java.util.Comparator;
36-
import java.util.HashSet;
3730
import java.util.TreeMap;
3831
import javax.annotation.Nullable;
3932

@@ -168,11 +161,11 @@ private static class PackageCollector {
168161

169162
/** Stores transitive {@link Package.Metadata}s of {@link ConfiguredTargetValues}s. */
170163
private final TreeMap<ConfiguredTargetKey, NestedSet<Package.Metadata>>
171-
configuredTargetPackages = new TreeMap<>(CONFIGURED_TARGET_KEY_ORDERING);
164+
configuredTargetPackages = new TreeMap<>(ConfiguredTargetKey.ORDERING);
172165

173166
/** Stores transitive {@link Package.Metadata}s of {@link AspectValue}s. */
174167
private final TreeMap<AspectKey, NestedSet<Package.Metadata>> aspectPackages =
175-
new TreeMap<>(ASPECT_KEY_ORDERING);
168+
new TreeMap<>(AspectKey.ORDERING);
176169

177170
/**
178171
* Constructs the deterministically ordered result.
@@ -196,50 +189,4 @@ private NestedSet<Package.Metadata> buildSet() {
196189
}
197190
}
198191

199-
private static final Comparator<ConfiguredTargetKey> CONFIGURED_TARGET_KEY_ORDERING =
200-
comparing(ConfiguredTargetKey::getLabel)
201-
.thenComparing(ConfiguredTargetKey::getExecutionPlatformLabel, nullsFirst(naturalOrder()))
202-
.thenComparing(
203-
ConfiguredTargetKey::getConfigurationKey,
204-
nullsFirst(comparing(BuildConfigurationKey::getOptionsChecksum)));
205-
206-
private static final Comparator<AspectKey> ASPECT_KEY_ORDERING =
207-
comparing(AspectKey::getBaseConfiguredTargetKey, CONFIGURED_TARGET_KEY_ORDERING)
208-
.thenComparing(
209-
(left, right) -> new AspectKeyDescriptorGraphComparator().compare(left, right));
210-
211-
/**
212-
* Compares the {@link AspectKey} graph structure for specific dependencies.
213-
*
214-
* <p>An {@link AspectKey} for a dependency is determined by {@link
215-
* AspectCollection#buildAspectKey}. This means that the {@link AspectKey} is structured like a
216-
* DAG with the following properties.
217-
*
218-
* <ul>
219-
* <li>The {@link AspectKey#getBaseConfiguredTargetKey} is the same across all nodes.
220-
* <li>Each DAG node has a unique {@link AspectKey#getAspectDescriptor}.
221-
* </ul>
222-
*
223-
* <p>Given the above, it's sufficient to traverse unique {@link AspectDescriptor}s to understand
224-
* the toplogy of both graphs.
225-
*
226-
* <p>NB: a new instance of this comparator must be constructed for each comparison.
227-
*/
228-
private static class AspectKeyDescriptorGraphComparator implements Comparator<AspectKey> {
229-
private final HashSet<AspectDescriptor> visited = new HashSet<>();
230-
231-
@Override
232-
public int compare(AspectKey left, AspectKey right) {
233-
AspectDescriptor leftDescriptor = left.getAspectDescriptor();
234-
AspectDescriptor rightDescriptor = right.getAspectDescriptor();
235-
if (!leftDescriptor.equals(rightDescriptor)) {
236-
return leftDescriptor.getDescription().compareTo(rightDescriptor.getDescription());
237-
}
238-
if (!visited.add(leftDescriptor)) {
239-
return 0;
240-
}
241-
242-
return lexicographical(this).compare(left.getBaseKeys(), right.getBaseKeys());
243-
}
244-
}
245192
}

‎src/main/java/com/google/devtools/build/lib/analysis/Util.java‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414

1515
package com.google.devtools.build.lib.analysis;
1616

17-
import com.google.common.collect.ImmutableSet;
17+
import com.google.common.collect.ImmutableList;
1818
import com.google.common.collect.Sets;
1919
import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
2020
import com.google.devtools.build.lib.analysis.config.CommonOptions;
@@ -79,7 +79,7 @@ public static boolean containsHyphen(PathFragment path) {
7979
* filtering). note: nodes that are depended on both implicitly and explicitly are considered
8080
* explicit.
8181
*/
82-
public static ImmutableSet<ConfiguredTargetKey> findImplicitDeps(RuleContext ruleContext) {
82+
public static ImmutableList<ConfiguredTargetKey> findImplicitDeps(RuleContext ruleContext) {
8383
Set<ConfiguredTargetKey> maybeImplicitDeps = CompactHashSet.create();
8484
Set<ConfiguredTargetKey> explicitDeps = CompactHashSet.create();
8585
// Consider rule attribute dependencies.
@@ -129,7 +129,8 @@ public static ImmutableSet<ConfiguredTargetKey> findImplicitDeps(RuleContext rul
129129
}
130130
}
131131
}
132-
return ImmutableSet.copyOf(Sets.difference(maybeImplicitDeps, explicitDeps));
132+
return ImmutableList.sortedCopyOf(
133+
ConfiguredTargetKey.ORDERING, Sets.difference(maybeImplicitDeps, explicitDeps));
133134
}
134135

135136
private static void addLabelsAndConfigs(

‎src/main/java/com/google/devtools/build/lib/analysis/configuredtargets/RuleConfiguredTarget.java‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@
2020
import com.google.common.base.Joiner;
2121
import com.google.common.collect.ImmutableList;
2222
import com.google.common.collect.ImmutableMap;
23-
import com.google.common.collect.ImmutableSet;
2423
import com.google.common.collect.Interner;
2524
import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
2625
import com.google.devtools.build.lib.actions.ActionLookupKey;
@@ -71,14 +70,14 @@
7170
public final class RuleConfiguredTarget extends AbstractConfiguredTarget {
7271

7372
/** A set of this target's implicitDeps. */
74-
private final ImmutableSet<ConfiguredTargetKey> implicitDeps;
73+
private final ImmutableList<ConfiguredTargetKey> implicitDeps;
7574

7675
/**
7776
* An interner for the implicitDeps set. {@link Util.findImplicitDeps} is called upon every
7877
* construction of a RuleConfiguredTarget and we expect many of these targets to contain the same
7978
* set of implicit deps so this reduces the memory load per build.
8079
*/
81-
private static final Interner<ImmutableSet<ConfiguredTargetKey>> IMPLICIT_DEPS_INTERNER =
80+
private static final Interner<ImmutableList<ConfiguredTargetKey>> IMPLICIT_DEPS_INTERNER =
8281
BlazeInterners.newWeakInterner();
8382

8483
private final TransitiveInfoProviderMap providers;
@@ -93,7 +92,7 @@ private RuleConfiguredTarget(
9392
boolean isCreatedInSymbolicMacro,
9493
TransitiveInfoProviderMap providers,
9594
ImmutableMap<Label, ConfigMatchingProvider> configConditions,
96-
ImmutableSet<ConfiguredTargetKey> implicitDeps,
95+
ImmutableList<ConfiguredTargetKey> implicitDeps,
9796
RuleClassId ruleClassId,
9897
ImmutableList<ActionAnalysisMetadata> actions) {
9998
super(actionLookupKey, visibility);
@@ -163,7 +162,7 @@ public RuleConfiguredTarget(
163162
isCreatedInSymbolicMacro,
164163
providers,
165164
configConditions,
166-
ImmutableSet.of(),
165+
ImmutableList.of(),
167166
ruleClassId,
168167
ImmutableList.of());
169168
checkState(providers.get(IncompatiblePlatformProvider.PROVIDER) != null, actionLookupKey);
@@ -180,7 +179,7 @@ public RuleConfiguredTarget(
180179
NestedSet<PackageGroupContents> visibility,
181180
TransitiveInfoProviderMap providers,
182181
ImmutableMap<Label, ConfigMatchingProvider> configConditions,
183-
ImmutableSet<ConfiguredTargetKey> implicitDeps,
182+
ImmutableList<ConfiguredTargetKey> implicitDeps,
184183
RuleClassId ruleClassId,
185184
ImmutableList<ActionAnalysisMetadata> actions) {
186185
super(lookupKey, visibility);
@@ -217,7 +216,7 @@ public boolean isRuleConfiguredTarget() {
217216
return true;
218217
}
219218

220-
public ImmutableSet<ConfiguredTargetKey> getImplicitDeps() {
219+
public ImmutableList<ConfiguredTargetKey> getImplicitDeps() {
221220
return implicitDeps;
222221
}
223222

‎src/main/java/com/google/devtools/build/lib/skyframe/AspectKeyCreator.java‎

Lines changed: 50 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,11 @@
1313
// limitations under the License.
1414
package com.google.devtools.build.lib.skyframe;
1515

16+
import static com.google.common.collect.Comparators.lexicographical;
1617
import static com.google.common.collect.ImmutableList.toImmutableList;
18+
import static java.util.Comparator.comparing;
1719

1820
import com.google.common.base.MoreObjects;
19-
import com.google.common.base.Objects;
2021
import com.google.common.collect.ImmutableList;
2122
import com.google.common.collect.ImmutableMap;
2223
import com.google.devtools.build.lib.actions.ActionLookupKey;
@@ -34,6 +35,8 @@
3435
import com.google.devtools.build.skyframe.SkyFunctionName;
3536
import com.google.devtools.build.skyframe.SkyKey;
3637
import java.util.Comparator;
38+
import java.util.HashSet;
39+
import java.util.Objects;
3740
import javax.annotation.Nullable;
3841

3942
/** The class responsible for creating & interning the various types of AspectKeys. */
@@ -100,6 +103,9 @@ public final int hashCode() {
100103
@AutoCodec
101104
public abstract static class AspectKey extends AspectBaseKey implements CqueryNode {
102105
private static final SkyKeyInterner<AspectKey> interner = SkyKey.newInterner();
106+
public static final Comparator<AspectKey> ORDERING =
107+
comparing(AspectKey::getBaseConfiguredTargetKey, ConfiguredTargetKey.ORDERING)
108+
.thenComparing((left, right) -> new DescriptorGraphComparator().compare(left, right));
103109

104110
private final AspectDescriptor aspectDescriptor;
105111

@@ -215,9 +221,9 @@ public boolean equals(Object other) {
215221
return false;
216222
}
217223
return hashCode() == that.hashCode()
218-
&& Objects.equal(getBaseKeys(), that.getBaseKeys())
219-
&& Objects.equal(getBaseConfiguredTargetKey(), that.getBaseConfiguredTargetKey())
220-
&& Objects.equal(aspectDescriptor, that.aspectDescriptor);
224+
&& Objects.equals(getBaseKeys(), that.getBaseKeys())
225+
&& Objects.equals(getBaseConfiguredTargetKey(), that.getBaseConfiguredTargetKey())
226+
&& Objects.equals(aspectDescriptor, that.aspectDescriptor);
221227
}
222228

223229
public String prettyPrint() {
@@ -303,6 +309,41 @@ public String getDescription() {
303309
baseKeys.stream().map(AspectKey::getDescription).collect(toImmutableList()));
304310
}
305311
}
312+
313+
/**
314+
* Compares the {@link AspectKey} graph structure for specific dependencies.
315+
*
316+
* <p>An {@link AspectKey} for a dependency is determined by {@link
317+
* com.google.devtools.build.lib.analysis.AspectCollection#buildAspectKey}. This means that the
318+
* {@link AspectKey} is structured like a DAG with the following properties.
319+
*
320+
* <ul>
321+
* <li>The {@link AspectKey#getBaseConfiguredTargetKey} is the same across all nodes.
322+
* <li>Each DAG node has a unique {@link AspectKey#getAspectDescriptor}.
323+
* </ul>
324+
*
325+
* <p>Given the above, it's sufficient to traverse unique {@link AspectDescriptor}s to
326+
* understand the toplogy of both graphs.
327+
*
328+
* <p>NB: a new instance of this comparator must be constructed for each comparison.
329+
*/
330+
private static class DescriptorGraphComparator implements Comparator<AspectKey> {
331+
private final HashSet<AspectDescriptor> visited = new HashSet<>();
332+
333+
@Override
334+
public int compare(AspectKey left, AspectKey right) {
335+
AspectDescriptor leftDescriptor = left.getAspectDescriptor();
336+
AspectDescriptor rightDescriptor = right.getAspectDescriptor();
337+
if (!leftDescriptor.equals(rightDescriptor)) {
338+
return leftDescriptor.getDescription().compareTo(rightDescriptor.getDescription());
339+
}
340+
if (!visited.add(leftDescriptor)) {
341+
return 0;
342+
}
343+
344+
return lexicographical(this).compare(left.getBaseKeys(), right.getBaseKeys());
345+
}
346+
}
306347
}
307348

308349
/**
@@ -386,11 +427,12 @@ public boolean equals(Object o) {
386427
if (!(o instanceof TopLevelAspectsKey that)) {
387428
return false;
388429
}
430+
389431
return hashCode() == that.hashCode()
390-
&& Objects.equal(targetLabel, that.targetLabel)
391-
&& Objects.equal(getBaseConfiguredTargetKey(), that.getBaseConfiguredTargetKey())
392-
&& Objects.equal(topLevelAspectsClasses, that.topLevelAspectsClasses)
393-
&& Objects.equal(topLevelAspectsParameters, that.topLevelAspectsParameters);
432+
&& Objects.equals(targetLabel, that.targetLabel)
433+
&& Objects.equals(getBaseConfiguredTargetKey(), that.getBaseConfiguredTargetKey())
434+
&& Objects.equals(topLevelAspectsClasses, that.topLevelAspectsClasses)
435+
&& Objects.equals(topLevelAspectsParameters, that.topLevelAspectsParameters);
394436
}
395437

396438
@Override

‎src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetKey.java‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,9 @@
1616

1717
import static com.google.common.base.Preconditions.checkNotNull;
1818
import static com.google.devtools.build.lib.util.HashCodes.hashObjects;
19+
import static java.util.Comparator.comparing;
20+
import static java.util.Comparator.naturalOrder;
21+
import static java.util.Comparator.nullsFirst;
1922

2023
import com.google.common.base.MoreObjects;
2124
import com.google.devtools.build.lib.actions.ActionLookupKey;
@@ -34,6 +37,7 @@
3437
import com.google.protobuf.CodedInputStream;
3538
import com.google.protobuf.CodedOutputStream;
3639
import java.io.IOException;
40+
import java.util.Comparator;
3741
import java.util.Objects;
3842
import javax.annotation.Nullable;
3943

@@ -64,6 +68,13 @@ public class ConfiguredTargetKey implements ActionLookupKey {
6468
*/
6569
private static final SkyKey.SkyKeyInterner<ConfiguredTargetKey> interner = SkyKey.newInterner();
6670

71+
public static final Comparator<ConfiguredTargetKey> ORDERING =
72+
comparing(ConfiguredTargetKey::getLabel)
73+
.thenComparing(ConfiguredTargetKey::getExecutionPlatformLabel, nullsFirst(naturalOrder()))
74+
.thenComparing(
75+
ConfiguredTargetKey::getConfigurationKey,
76+
nullsFirst(comparing(BuildConfigurationKey::getOptionsChecksum)));
77+
6778
private final Label label;
6879
@Nullable private final BuildConfigurationKey configurationKey;
6980
private final int hashCode;

0 commit comments

Comments
 (0)