From 3108031f93a59883d02f447e57ee0c62afb45c0c Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 4 Dec 2024 08:36:15 -0800 Subject: [PATCH] Fixes a bug in implementation of Tarjan's SCC algorithm. Tarjan's algorithm uses the traversal index, not the stack depth. PiperOrigin-RevId: 702737429 Change-Id: I394897aa07512202ddbcf3f222bfe25abda87070 --- .../serialization/testutils/Dumper.java | 21 +++-- .../testutils/Fingerprinter.java | 90 ++++++++++--------- .../testutils/GraphDataCollector.java | 23 +++-- .../testutils/GraphTraverser.java | 11 +-- .../testutils/FingerprinterTest.java | 45 +++++++++- 5 files changed, 124 insertions(+), 66 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/Dumper.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/Dumper.java index dd8cfef2d867d8..03ebf25d82bd1d 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/Dumper.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/Dumper.java @@ -209,7 +209,7 @@ public void outputPrimitive(PrimitiveInfo info, Object parent, TextSink sink) { @Override @Nullable - public String checkCache(@Nullable String label, Class type, Object obj, TextSink sink) { + public Descriptor checkCache(@Nullable String label, Class type, Object obj, TextSink sink) { int nextId = referenceIds.size(); String fingerprint; if (fingerprints != null && ((fingerprint = fingerprints.get(obj)) != null)) { @@ -222,7 +222,7 @@ public String checkCache(@Nullable String label, Class type, Object obj, Text if (previousId != null) { // An object having this fingerprint has been observed previously. Outputs only a // backreference. - sink.output(label, getDescriptor(type, previousId)); + sink.output(label, getDescriptor(type, previousId).toString()); return null; } } else { @@ -230,7 +230,7 @@ public String checkCache(@Nullable String label, Class type, Object obj, Text Integer previousId = referenceIds.putIfAbsent(obj, nextId); if (previousId != null) { // This instance has been observed previously. Outputs only a backreference. - sink.output(label, getDescriptor(type, previousId)); + sink.output(label, getDescriptor(type, previousId).toString()); return null; } } @@ -239,14 +239,14 @@ public String checkCache(@Nullable String label, Class type, Object obj, Text @Override public void outputByteArray( - @Nullable String label, String descriptor, byte[] bytes, TextSink sink) { + @Nullable String label, Descriptor descriptor, byte[] bytes, TextSink sink) { sink.output(label, descriptor + " [" + HEX_FORMAT.formatHex(bytes) + ']'); } @Override public void outputInlineArray( - @Nullable String label, String descriptor, Object arr, TextSink sink) { - var builder = new StringBuilder(descriptor).append(" ["); + @Nullable String label, Descriptor descriptor, Object arr, TextSink sink) { + var builder = new StringBuilder(descriptor.toString()).append(" ["); int length = Array.getLength(arr); for (int i = 0; i < length; i++) { if (i > 0) { @@ -260,14 +260,14 @@ public void outputInlineArray( @Override public void outputEmptyAggregate( - @Nullable String label, String descriptor, Object unused, TextSink sink) { + @Nullable String label, Descriptor descriptor, Object unused, TextSink sink) { sink.output(label, descriptor + " []"); } @Override @SuppressWarnings("CanIgnoreReturnValueSuggester") public TextSink initAggregate( - @Nullable String label, String descriptor, Object unused, TextSink sink) { + @Nullable String label, Descriptor descriptor, Object unused, TextSink sink) { sink.output(label, descriptor + " ["); sink.indent(); return sink; @@ -324,8 +324,7 @@ static boolean shouldInline(Class type) { .add(Class.class) .build(); - /** Returns an object descriptor like {@code ()}. */ - private static String getDescriptor(Class type, int id) { - return getTypeName(type) + '(' + id + ')'; + private static Descriptor getDescriptor(Class type, int id) { + return new Descriptor(getTypeName(type), id); } } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/Fingerprinter.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/Fingerprinter.java index aad965af671ed3..b71c54f4aca83d 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/Fingerprinter.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/Fingerprinter.java @@ -32,6 +32,7 @@ import java.util.Comparator; import java.util.HexFormat; import java.util.IdentityHashMap; +import java.util.Iterator; import java.util.Map; import javax.annotation.Nullable; @@ -87,6 +88,8 @@ public final class Fingerprinter implements GraphDataCollector canonicalIds = new IdentityHashMap<>(); + private int nextTraversalIndex = 0; + /** * Stack for detecting cyclic backreferences in depth-first traversal. * @@ -100,11 +103,7 @@ public final class Fingerprinter implements GraphDataCollector childRepresentations = new ArrayList<>(); - private int minUpReference = Integer.MAX_VALUE; + private int lowLink = Integer.MAX_VALUE; private RepresentationSink( @Nullable RepresentationSink parent, @Nullable String parentLabel, - String descriptor, - Object obj, - int depth) { + Descriptor descriptor, + Object obj) { this.parent = parent; this.parentLabel = parentLabel; this.descriptor = descriptor; this.obj = obj; - this.depth = depth; } private void addChildRepresentation(@Nullable String label, String representation) { @@ -175,14 +171,14 @@ private void addChildRepresentation(@Nullable String label, String representatio */ private void signalUpReference(String childHeader, int upReference) { childRepresentations.add(childHeader); - minUpReference = min(minUpReference, upReference); + lowLink = min(lowLink, upReference); } @Override public void completeAggregate() { stack.remove(obj); - if (minUpReference == depth) { + if (lowLink == descriptor.traversalIndex()) { // This is the top of a strongly connected component. handleStronglyConnectedComponent(); return; @@ -190,7 +186,7 @@ public void completeAggregate() { String fingerprint = computeFingerprint(); - if (minUpReference > depth) { + if (lowLink > descriptor.traversalIndex()) { // This object is complete. Publishes its fingerprint and reports it to the parent. publishCompleteFingerprint(fingerprint); return; @@ -198,9 +194,9 @@ public void completeAggregate() { // A child contained up references above this object. Marks this object as part of a strongly // connected component along with its local, partial fingerprint. - stronglyConnectedComponent.put(obj, new ElementInfo(fingerprint, minUpReference)); + stronglyConnectedComponent.put(obj, new ElementInfo(fingerprint, lowLink)); if (parent != null) { - parent.signalUpReference(prependLabel(parentLabel, descriptor), minUpReference); + parent.signalUpReference(prependLabel(parentLabel, descriptor.description()), lowLink); } } @@ -210,10 +206,18 @@ private void handleStronglyConnectedComponent() { stronglyConnectedComponent.size() + 1, /* expectedValuesPerKey= */ 3); String fingerprint = computeFingerprint(); elements.put(fingerprint, obj); - for (Map.Entry entry : stronglyConnectedComponent.entrySet()) { - elements.put(entry.getValue().partialFingerprint(), entry.getKey()); + Iterator> entryIterator = + stronglyConnectedComponent.entrySet().iterator(); + // Transfers objects from the `stronglyConnectedComponents` that are underneath `obj` into + // `elements`. There can be objects in `stronglyConnectedComponents` that siblings or cousins + // when an ancestor of `obj` belongs to a different strongly connected component. + while (entryIterator.hasNext()) { + Map.Entry entry = entryIterator.next(); + if (entry.getValue().lowLink() >= descriptor.traversalIndex()) { + elements.put(entry.getValue().partialFingerprint(), entry.getKey()); + entryIterator.remove(); + } } - stronglyConnectedComponent.clear(); Object root = tryDetermineRoot(asSortedGroups(elements)); @@ -252,7 +256,8 @@ private void handleStronglyConnectedComponent() { *

This is partial if {@link #obj} is part of a strongly connected component. */ private String computeFingerprint() { - String representation = descriptor + ": [" + String.join(", ", childRepresentations) + ']'; + String representation = + descriptor.description() + ": [" + String.join(", ", childRepresentations) + ']'; return fingerprintString(representation); } @@ -296,7 +301,7 @@ public void outputPrimitive(PrimitiveInfo info, Object parent, RepresentationSin @Override @Nullable - public String checkCache( + public Descriptor checkCache( @Nullable String label, Class type, Object obj, RepresentationSink sink) { String representation = canonicalIds.get(obj); if (representation != null) { @@ -304,29 +309,28 @@ public String checkCache( return null; } - String descriptor = getTypeName(type); + String description = getTypeName(type); ElementInfo info = stronglyConnectedComponent.get(obj); if (info != null) { - sink.signalUpReference(prependLabel(label, descriptor), info.minUpReference); + sink.signalUpReference(prependLabel(label, description), info.lowLink); return null; } - int depth = stack.size(); - Integer previousDepth = stack.putIfAbsent(obj, depth); + Integer previousDepth = stack.putIfAbsent(obj, nextTraversalIndex); if (previousDepth != null) { - sink.signalUpReference(prependLabel(label, descriptor), previousDepth); + sink.signalUpReference(prependLabel(label, description), previousDepth); return null; } - // There's nothing cached. Returns the descriptor. - return descriptor; + // There's nothing cached. Returns a descriptor. + return new Descriptor(description, nextTraversalIndex++); } @Override public void outputByteArray( - @Nullable String label, String descriptor, byte[] bytes, RepresentationSink sink) { - String representation = descriptor + ": [" + HEX_FORMAT.formatHex(bytes) + ']'; + @Nullable String label, Descriptor descriptor, byte[] bytes, RepresentationSink sink) { + String representation = descriptor.description() + ": [" + HEX_FORMAT.formatHex(bytes) + ']'; String fingerprint = fingerprintString(representation); canonicalIds.put(bytes, fingerprint); sink.addChildRepresentation(label, fingerprint); @@ -334,8 +338,8 @@ public void outputByteArray( @Override public void outputInlineArray( - @Nullable String label, String descriptor, Object arr, RepresentationSink sink) { - StringBuilder representation = new StringBuilder(descriptor).append(": ["); + @Nullable String label, Descriptor descriptor, Object arr, RepresentationSink sink) { + StringBuilder representation = new StringBuilder(descriptor.description()).append(": ["); int length = Array.getLength(arr); for (int i = 0; i < length; i++) { if (i > 0) { @@ -355,8 +359,8 @@ public void outputInlineArray( @Override public void outputEmptyAggregate( - @Nullable String label, String descriptor, Object obj, RepresentationSink sink) { - String representation = descriptor + " []"; + @Nullable String label, Descriptor descriptor, Object obj, RepresentationSink sink) { + String representation = descriptor.description() + " []"; String fingerprint = fingerprintString(representation.toString()); canonicalIds.put(obj, fingerprint); sink.addChildRepresentation(label, fingerprint); @@ -364,8 +368,8 @@ public void outputEmptyAggregate( @Override public RepresentationSink initAggregate( - @Nullable String label, String descriptor, Object obj, RepresentationSink parent) { - return new RepresentationSink(parent, label, descriptor, obj, parent.depth + 1); + @Nullable String label, Descriptor descriptor, Object obj, RepresentationSink parent) { + return new RepresentationSink(parent, label, descriptor, obj); } /** @@ -373,11 +377,11 @@ public RepresentationSink initAggregate( * * @param partialFingerprint a partial description of a strongly connected component element, * omitting information about any nodes that are strongly connected. - * @param minUpReference strongly connected components are detected by seeing references up the - * stack during depth first search. This value is the minimum stack depth referenced by this - * node or any of its descendants. + * @param lowLink strongly connected components are detected by seeing references up the stack + * during depth first search. This value is the minimum stack depth referenced by this node or + * any of its descendants. */ - record ElementInfo(String partialFingerprint, int minUpReference) {} + record ElementInfo(String partialFingerprint, int lowLink) {} @Nullable private Object tryDetermineRoot(ImmutableList>> groups) { @@ -429,8 +433,8 @@ private static ImmutableList>> asSortedGrou .collect(toImmutableList()); } - private static String prependLabel(@Nullable String label, String descriptor) { - return label == null ? descriptor : label + descriptor; + private static String prependLabel(@Nullable String label, String description) { + return label == null ? description : label + description; } @VisibleForTesting diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/GraphDataCollector.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/GraphDataCollector.java index e73f05cc28f738..844df33b23b5b5 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/GraphDataCollector.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/GraphDataCollector.java @@ -45,6 +45,19 @@ interface Sink { void outputPrimitive(PrimitiveInfo info, Object parent, SinkT sink); + /** + * Metadata about an object. + * + * @param description describes the type of the object + * @param traversalIndex the index at which this object was first encountered during traversal + */ + record Descriptor(String description, int traversalIndex) { + @Override + public final String toString() { + return description + "(" + traversalIndex + ")"; + } + } + /** * Checks whether {@code obj} already exists in the cache. * @@ -52,18 +65,18 @@ interface Sink { * returns a descriptor to be used (that does not include {@code label}). */ @Nullable - String checkCache(@Nullable String label, Class type, Object obj, SinkT sink); + Descriptor checkCache(@Nullable String label, Class type, Object obj, SinkT sink); - void outputByteArray(@Nullable String label, String descriptor, byte[] bytes, SinkT sink); + void outputByteArray(@Nullable String label, Descriptor descriptor, byte[] bytes, SinkT sink); /** * Outputs an array of elements of inlined type. * *

{@code arr} could be an array of primitives, which cannot be cast to {@code Object[]}. */ - void outputInlineArray(@Nullable String label, String descriptor, Object arr, SinkT sink); + void outputInlineArray(@Nullable String label, Descriptor descriptor, Object arr, SinkT sink); - void outputEmptyAggregate(@Nullable String label, String descriptor, Object obj, SinkT sink); + void outputEmptyAggregate(@Nullable String label, Descriptor descriptor, Object obj, SinkT sink); /** * Non-empty maps, collections, arrays and ordinary objects are handled using this method. @@ -75,5 +88,5 @@ interface Sink { * @param descriptor a description of {@code obj}, for example, a text description of its type * @param sink where to write the aggregate */ - SinkT initAggregate(@Nullable String label, String descriptor, Object obj, SinkT sink); + SinkT initAggregate(@Nullable String label, Descriptor descriptor, Object obj, SinkT sink); } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/GraphTraverser.java b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/GraphTraverser.java index e044e5bc96ab73..14a1079b97568d 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/GraphTraverser.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/GraphTraverser.java @@ -21,6 +21,7 @@ import com.google.devtools.build.lib.skyframe.serialization.testutils.FieldInfoCache.FieldInfo; import com.google.devtools.build.lib.skyframe.serialization.testutils.FieldInfoCache.ObjectInfo; import com.google.devtools.build.lib.skyframe.serialization.testutils.FieldInfoCache.PrimitiveInfo; +import com.google.devtools.build.lib.skyframe.serialization.testutils.GraphDataCollector.Descriptor; import java.lang.ref.WeakReference; import java.util.Collection; import java.util.Map; @@ -62,7 +63,7 @@ void traverseObject(@Nullable String label, Object obj, SinkT sink) { return; } - String descriptor = collector.checkCache(label, type, obj, sink); + Descriptor descriptor = collector.checkCache(label, type, obj, sink); if (descriptor == null) { return; // cache hit } @@ -86,7 +87,7 @@ void traverseObject(@Nullable String label, Object obj, SinkT sink) { } private void traverseArray( - @Nullable String label, String descriptor, Class type, Object obj, SinkT sink) { + @Nullable String label, Descriptor descriptor, Class type, Object obj, SinkT sink) { var componentType = type.getComponentType(); if (componentType.equals(byte.class)) { collector.outputByteArray(label, descriptor, (byte[]) obj, sink); @@ -116,7 +117,7 @@ private void traverseArray( } private void traverseMapEntries( - @Nullable String label, String descriptor, Map map, SinkT sink) { + @Nullable String label, Descriptor descriptor, Map map, SinkT sink) { if (map.isEmpty()) { collector.outputEmptyAggregate(label, descriptor, map, sink); return; @@ -131,7 +132,7 @@ private void traverseMapEntries( } private void traverseCollectionElements( - @Nullable String label, String descriptor, Collection collection, SinkT sink) { + @Nullable String label, Descriptor descriptor, Collection collection, SinkT sink) { if (collection.isEmpty()) { collector.outputEmptyAggregate(label, descriptor, collection, sink); return; @@ -145,7 +146,7 @@ private void traverseCollectionElements( } private void traverseObjectFields( - @Nullable String label, String descriptor, Class type, Object obj, SinkT sink) { + @Nullable String label, Descriptor descriptor, Class type, Object obj, SinkT sink) { ImmutableList fieldInfo = getFieldInfo(type); if (fieldInfo.isEmpty()) { collector.outputEmptyAggregate(label, descriptor, obj, sink); diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/serialization/testutils/FingerprinterTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/serialization/testutils/FingerprinterTest.java index 9c1b17df9b24fc..d80b2e50aa6e86 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/serialization/testutils/FingerprinterTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/serialization/testutils/FingerprinterTest.java @@ -547,10 +547,10 @@ public void childCycle() { childVisitOrder.forEach( (obj, index) -> expectedFingerprints.put(obj, childRootFingerprint + '[' + index + ']')); - // `one` ends up being identified as the root of the parent loop. + // `two` ends up being identified as the root of the parent loop. dumpOut = new StringBuilder(); IdentityHashMap parentVisitOrder = - computeVisitOrder(/* registry= */ null, expectedFingerprints, one, dumpOut); + computeVisitOrder(/* registry= */ null, expectedFingerprints, two, dumpOut); String parentRootFingerprint = fingerprintString(dumpOut.toString()); parentVisitOrder.forEach( (obj, index) -> expectedFingerprints.put(obj, parentRootFingerprint + '[' + index + ']')); @@ -661,6 +661,47 @@ public void unlabeledCyclicComplex_fingerprints() { } } + @Test + public void depthOverlappingRecursion_fingerprintsCorrectly() { + // Sets up a graph consisting of a cycle with another cycle hanging off of it. + // A -> B -> C -> A (first cycle) + // B -> D -> E -> F -> D (second cycle) + // + // This graph has interesting backtracking behavior at B. Since C loops back to A, C stays on + // the stack when it backtracks. But B doesn't backtrack until after exploring the DEF cycle. + // So C stays on the stack when D backtracks and completes a strongly connected component. + // + // This graph structure checks the behavior of D's backtracking when there's data on the stack + // that does not belong to the same SCC. + var a = new Object[2]; + var b = new Object[3]; + var c = new Object[2]; + var d = new Object[2]; + var e = new Object[2]; + var f = new Object[2]; + + a[0] = "A"; + b[0] = "B"; + c[0] = "C"; + d[0] = "D"; // D, E, and F are locally ambiguous. So if C ends up included in the SCC defined + e[0] = "D"; // by DEF, it will be unique and become the root. This then breaks the assertion + f[0] = "D"; // below. + + a[1] = b; + b[1] = c; + b[2] = d; + c[1] = a; + d[1] = e; + e[1] = f; + f[1] = d; + + IdentityHashMap fingerprints = computeFingerprints(a); + + IdentityHashMap fingerprintsDef = computeFingerprints(d); + + assertThat(fingerprints).containsAtLeastEntriesIn(fingerprintsDef); + } + private static String fingerprintNode(int id, String left, String right) { return fingerprintString( NAMESPACE + "Node: [id=" + id + ", left=" + left + ", right=" + right + "]");