Skip to content

Commit

Permalink
Optimize PathFinder and fix recursion bug
Browse files Browse the repository at this point in the history 
This commit optimizes PathFinder.Path to be an internal linked list to
reduce path copies while traversing potential recursive paths.

This commit also fixes a bug where mutually recursive structures through
optional members were erroneously flagged as invalid.
  • Loading branch information
@mtdowling
mtdowling authored and Michael Dowling committed Apr 25, 2022
1 parent 16621af commit 5e7af7a
Show file tree
Hide file tree
Showing 4 changed files with 167 additions and 98 deletions.
6 changes: 6 additions & 0 deletions config/spotbugs/filter.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,4 +111,10 @@
<Class name="software.amazon.smithy.model.knowledge.HttpBindingIndex"/>
<Bug pattern="NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE"/>
</Match>

<!-- current is used to create an internal linked list's next value from a list -->
<Match>
<Class name="software.amazon.smithy.model.selector.PathFinder$Path"/>
<Bug pattern="UC_USELESS_OBJECT"/>
</Match>
</FindBugsFilter>
171 changes: 118 additions & 53 deletions smithy-model/src/main/java/software/amazon/smithy/model/selector/PathFinder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,16 @@
package software.amazon.smithy.model.selector;

import java.util.AbstractList;
import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashSet;
import java.util.Iterator;
import java.util.List;
import java.util.NoSuchElementException;
import java.util.Optional;
import java.util.Queue;
import java.util.Set;
import java.util.function.Predicate;
import java.util.logging.Logger;
import software.amazon.smithy.model.Model;
import software.amazon.smithy.model.SourceException;
Expand All @@ -36,6 +40,7 @@
import software.amazon.smithy.model.shapes.ShapeId;
import software.amazon.smithy.model.shapes.StructureShape;
import software.amazon.smithy.model.shapes.ToShapeId;
import software.amazon.smithy.utils.FunctionalUtils;
import software.amazon.smithy.utils.ListUtils;

/**
Expand All @@ -62,6 +67,7 @@ public final class PathFinder {

private final Model model;
private final NeighborProvider reverseProvider;
private Predicate<Relationship> filter = FunctionalUtils.alwaysTrue();

private PathFinder(Model model) {
this.model = model;
Expand All @@ -78,6 +84,15 @@ public static PathFinder create(Model model) {
return new PathFinder(model);
}

/**
* Sets a predicate function to prevents traversing specific relationships.
*
* @param predicate Predicate that must return true in order to continue traversing relationships.
*/
public void relationshipFilter(Predicate<Relationship> predicate) {
this.filter = predicate;
}

/**
* Finds all of the possible paths from the starting shape to all shapes
* connected to the starting shape that match the given selector.
Expand Down Expand Up @@ -116,7 +131,7 @@ private List<Path> searchFromShapeToSet(ToShapeId startingShape, Collection<Shap
if (shape == null || candidates.isEmpty()) {
return ListUtils.of();
} else {
return new Search(reverseProvider, shape, candidates).execute();
return new Search(reverseProvider, shape, candidates, filter).execute();
}
}

Expand Down Expand Up @@ -182,35 +197,82 @@ private Optional<Path> createPathTo(ToShapeId operationId, String memberName, Re
return Optional.empty();
}

List<Relationship> relationships = new ArrayList<>();
relationships.add(Relationship.create(operation, rel, struct));
relationships.add(Relationship.create(struct, RelationshipType.STRUCTURE_MEMBER, member));
relationships.add(Relationship.create(member, RelationshipType.MEMBER_TARGET, target));
return Optional.of(new Path(relationships));
Path path = new Path(Relationship.create(member, RelationshipType.MEMBER_TARGET, target), null);
path = new Path(Relationship.create(struct, RelationshipType.STRUCTURE_MEMBER, member), path);
path = new Path(Relationship.create(operation, rel, struct), path);
return Optional.of(path);
}

/**
* An immutable {@code Relationship} path from a starting shape to an end shape.
*/
public static final class Path extends AbstractList<Relationship> {
private final List<Relationship> relationships;
private final Relationship value;
private Path next;
private final int size;

public Path(List<Relationship> relationships) {
if (relationships.isEmpty()) {
throw new IllegalArgumentException("Relationships cannot be empty!");
}

this.relationships = relationships;
this.size = relationships.size();
this.value = relationships.get(0);

if (relationships.size() == 1) {
next = null;
} else {
Path current = this;
for (int i = 1; i < relationships.size(); i++) {
current.next = new Path(relationships.get(i), null);
current = current.next;
}
}
}

private Path(Relationship value, Path next) {
this.value = value;
this.next = next;
this.size = 1 + ((next == null) ? 0 : next.size);
}

@Override
public int size() {
return relationships.size();
return size;
}

@Override
public Relationship get(int index) {
return relationships.get(index);
Path current = this;
for (int i = 0; i < index; i++) {
current = current.next;
if (current == null) {
throw new IndexOutOfBoundsException("Invalid index " + index + "; size " + size());
}
}
return current.value;
}

@Override
public Iterator<Relationship> iterator() {
return new Iterator<Relationship>() {
private Path current = Path.this;

@Override
public boolean hasNext() {
return current != null;
}

@Override
public Relationship next() {
if (current == null) {
throw new NoSuchElementException();
}
Relationship result = current.value;
current = current.next;
return result;
}
};
}

/**
Expand All @@ -223,15 +285,17 @@ public Relationship get(int index) {
* @return Returns the list of shapes.
*/
public List<Shape> getShapes() {
List<Shape> results = new ArrayList<>(relationships.size());
for (Relationship rel : relationships) {
List<Shape> results = new ArrayList<>(size());
Iterator<Relationship> iterator = iterator();
for (int i = 0; i < size(); i++) {
Relationship rel = iterator.next();
results.add(rel.getShape());
// Add the shape pointed to by the tail to the result set if present
// without need to get the tail after iterating (an O(N) operation).
if (i == size() - 1) {
rel.getNeighborShape().ifPresent(results::add);
}
}
Relationship last = relationships.get(relationships.size() - 1);
if (last.getNeighborShape().isPresent()) {
results.add(last.getNeighborShape().get());
}

return results;
}

Expand All @@ -241,7 +305,7 @@ public List<Shape> getShapes() {
* @return Returns the starting shape of the Path.
*/
public Shape getStartShape() {
return relationships.get(0).getShape();
return value.getShape();
}

/**
Expand All @@ -253,12 +317,20 @@ public Shape getStartShape() {
* @throws SourceException if the last relationship is invalid.
*/
public Shape getEndShape() {
Relationship last = relationships.get(relationships.size() - 1);
Relationship last = tail();
return last.getNeighborShape().orElseThrow(() -> new SourceException(
"Relationship points to a shape that is invalid: " + last,
last.getShape()));
}

private Relationship tail() {
Path current = this;
while (current.next != null) {
current = current.next;
}
return current.value;
}

/**
* Converts the path to valid {@link Selector} syntax.
*
Expand All @@ -269,7 +341,7 @@ public String toString() {
StringBuilder result = new StringBuilder();
result.append("[id|").append(getStartShape().getId()).append("]");

for (Relationship rel : relationships) {
for (Relationship rel : this) {
if (rel.getRelationshipType() == RelationshipType.MEMBER_TARGET) {
result.append(" > ");
} else {
Expand All @@ -290,53 +362,46 @@ private static final class Search {
private final NeighborProvider provider;
private final Collection<Shape> candidates;
private final List<Path> results = new ArrayList<>();

Search(NeighborProvider provider, Shape startingShape, Collection<Shape> candidates) {
private final Predicate<Relationship> filter;

Search(
NeighborProvider provider,
Shape startingShape,
Collection<Shape> candidates,
Predicate<Relationship> filter
) {
this.startingShape = startingShape;
this.candidates = candidates;
this.provider = provider;
this.filter = filter;
}

List<Path> execute() {
Set<ShapeId> visited = new HashSet<>();
for (Shape candidate : candidates) {
traverseUp(candidate, new ArrayList<>(), visited);
visited.clear();
Queue<Path> queue = new ArrayDeque<>();
for (Shape current : candidates) {
addRelationships(current, queue, null);
while (!queue.isEmpty()) {
Path path = queue.poll();
Shape shape = path.value.getShape();
if (shape.getId().equals(startingShape.getId())) {
results.add(path);
} else {
addRelationships(shape, queue, path);
}
}
}

return results;
}

private void traverseUp(Shape current, List<Relationship> path, Set<ShapeId> visited) {
if (!path.isEmpty() && current.getId().equals(startingShape.getId())) {
// Add the path to the result set if the target shape was reached.
// But, don't add the path if no nodes have been traversed.
results.add(new Path(path));
return;
}

// Short circuit any possible recursion. While it's not possible
// to enter a recursive path with the built-in NeighborProvider
// implementations and the bottom-up traversal, it is possible
// that a custom neighbor provider has a different behavior, so
// this check remains just in case.
if (visited.contains(current.getId())) {
return;
}

visited.add(current.getId());

private void addRelationships(Shape current, Queue<Path> queue, Path currentPath) {
for (Relationship relationship : provider.getNeighbors(current)) {
// Don't traverse up through containers.
if (relationship.getDirection() == RelationshipDirection.DIRECTED) {
List<Relationship> newPath = new ArrayList<>(path.size() + 1);
newPath.add(relationship);
newPath.addAll(path);
traverseUp(relationship.getShape(), newPath, visited);
if (filter.test(relationship)) {
queue.add(new Path(relationship, currentPath));
}
}
}

visited.remove(current.getId());
}
}
}
Loading

0 comments on commit 5e7af7a

Please sign in to comment.