Skip to content

Commit

Permalink
Fixes a bug in implementation of Tarjan's SCC algorithm.
Browse files Browse the repository at this point in the history 
Tarjan's algorithm uses the traversal index, not the stack depth.

PiperOrigin-RevId: 702737429
Change-Id: I394897aa07512202ddbcf3f222bfe25abda87070
  • Loading branch information
@aoeui @copybara-github
aoeui authored and copybara-github committed Dec 4, 2024
1 parent cd957c8 commit 3108031
Show file tree
Hide file tree
Showing 5 changed files with 124 additions and 66 deletions.
21 changes: 10 additions & 11 deletions src/main/java/com/google/devtools/build/lib/skyframe/serialization/testutils/Dumper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand All @@ -222,15 +222,15 @@ 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 {
// No fingerprint is available. Deduplicates by object reference.
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;
}
}
Expand All @@ -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) {
Expand All @@ -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;
Expand Down Expand Up @@ -324,8 +324,7 @@ static boolean shouldInline(Class<?> type) {
.add(Class.class)
.build();

/** Returns an object descriptor like {@code <type name>(<id>)}. */
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);
}
}
90 changes: 47 additions & 43 deletions ...in/java/com/google/devtools/build/lib/skyframe/serialization/testutils/Fingerprinter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -87,6 +88,8 @@ public final class Fingerprinter implements GraphDataCollector<Fingerprinter.Rep
*/
private final IdentityHashMap<Object, String> canonicalIds = new IdentityHashMap<>();

private int nextTraversalIndex = 0;

/**
* Stack for detecting cyclic backreferences in depth-first traversal.
*
Expand All @@ -100,11 +103,7 @@ public final class Fingerprinter implements GraphDataCollector<Fingerprinter.Rep

private final RepresentationSink rootSink =
new RepresentationSink(
/* parent= */ null,
/* parentLabel= */ null,
/* descriptor= */ null,
/* obj= */ null,
/* depth= */ -1);
/* parent= */ null, /* parentLabel= */ null, /* descriptor= */ null, /* obj= */ null);

/**
* Computes a fingerprint for {@code obj} using {code registry} for reference constants if
Expand Down Expand Up @@ -139,25 +138,22 @@ final class RepresentationSink implements GraphDataCollector.Sink {
/** Label parent uses for the child associated with this sink. */
@Nullable private final String parentLabel;

private final String descriptor;
private final Descriptor descriptor;
private final Object obj;

private final int depth;
private final ArrayList<String> 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) {
Expand All @@ -175,32 +171,32 @@ 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;
}

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;
}

// 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);
}
}

Expand All @@ -210,10 +206,18 @@ private void handleStronglyConnectedComponent() {
stronglyConnectedComponent.size() + 1, /* expectedValuesPerKey= */ 3);
String fingerprint = computeFingerprint();
elements.put(fingerprint, obj);
for (Map.Entry<Object, ElementInfo> entry : stronglyConnectedComponent.entrySet()) {
elements.put(entry.getValue().partialFingerprint(), entry.getKey());
Iterator<Map.Entry<Object, ElementInfo>> 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<Object, ElementInfo> 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));

Expand Down Expand Up @@ -252,7 +256,8 @@ private void handleStronglyConnectedComponent() {
* <p>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);
}

Expand Down Expand Up @@ -296,46 +301,45 @@ 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) {
sink.addChildRepresentation(label, representation);
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);
}

@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) {
Expand All @@ -355,29 +359,29 @@ 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);
}

@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);
}

/**
* Information collected about nodes in a strongly connected component.
*
* @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<Map.Entry<String, Collection<Object>>> groups) {
Expand Down Expand Up @@ -429,8 +433,8 @@ private static ImmutableList<Map.Entry<String, Collection<Object>>> 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
Expand Down
23 changes: 18 additions & 5 deletions ...va/com/google/devtools/build/lib/skyframe/serialization/testutils/GraphDataCollector.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,25 +45,38 @@ 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.
*
* <p>If it exists in the cache, populates {@code sink} appropriately and returns null. Otherwise,
* 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.
*
* <p>{@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.
Expand All @@ -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);
}
Loading

0 comments on commit 3108031

Please sign in to comment.