Skip to content

Commit

Permalink
fix(perf): Make getAncestors call faster
Browse files Browse the repository at this point in the history 
This change unrolls the recursion of the `getAncestorImpl` call into an imperative call.
getAncestors is actually called A TON! (especially since all `context.get` evaluations depend on it).
If the execution depth is large (e.g. canary with many stages) this call can take a while and, in some cases, throw a StackOverflow excetion.
Additionally, I added some logic to cache the `getAncesors` calls in the expression evaluation as it can't change

For context: `getAncestors` is executed 64 times for a simple wait stage execution

Results:
Time for `getAncestors` was reduced by a factor of 7x (1000 executions on 100 stage pipeline went from 7.2s to 0.9s)
Further more, due to caching the number of calls to `getAncestors` is reduces by ~3 (from 64 to 19)

So a total improvement time about 21x

As another example, planning a kayenta stage with 300 intervals went from 23.6s to 1.2s
  • Loading branch information
@marchello2000
marchello2000 committed Nov 19, 2019
1 parent 7ee2ac3 commit 19a1438
Show file tree
Hide file tree
Showing 2 changed files with 63 additions and 79 deletions.
136 changes: 58 additions & 78 deletions orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/model/Stage.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,25 +38,14 @@
import com.google.common.base.Strings;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Sets;
import com.netflix.spinnaker.kork.exceptions.SpinnakerException;
import com.netflix.spinnaker.orca.ExecutionStatus;
import com.netflix.spinnaker.orca.jackson.OrcaObjectMapper;
import com.netflix.spinnaker.orca.pipeline.model.support.RequisiteStageRefIdDeserializer;
import de.huxhorn.sulky.ulid.ULID;
import java.io.IOException;
import java.io.Serializable;
import java.util.ArrayList;
import java.util.Base64;
import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.*;
import java.util.function.Predicate;
import java.util.stream.Stream;
import javax.annotation.Nonnull;
Expand Down Expand Up @@ -365,14 +354,10 @@ public Task taskById(String taskId) {
*/
public List<Stage> ancestors() {
if (execution != null) {
Set<String> visited = Sets.newHashSetWithExpectedSize(execution.getStages().size());
return ImmutableList.<Stage>builder()
.add(this)
.addAll(getAncestorsImpl(visited, false))
.build();
} else {
return emptyList();
return getAncestorsImpl(false);
}

return emptyList();
}

/**
Expand All @@ -384,81 +369,76 @@ public List<Stage> ancestors() {
*/
public List<Stage> directAncestors() {
if (execution != null) {
Set<String> visited = Sets.newHashSetWithExpectedSize(execution.getStages().size());
return ImmutableList.<Stage>builder()
.add(this)
.addAll(getAncestorsImpl(visited, true))
.build();
} else {
return emptyList();
return getAncestorsImpl(true);
}

return emptyList();
}

/**
* Worker method to get the list of all ancestors (parents and, optionally, prerequisite stages)
* of the current stage.
*
* @param visited list of visited nodes
* @param directParentOnly true to only include direct parents of the stage, false to also include
* stages this stage depends on (via requisiteRefIds)
* @return list of ancestor stages
*/
private List<Stage> getAncestorsImpl(Set<String> visited, boolean directParentOnly) {
visited.add(this.refId);
public ImmutableList<Stage> getAncestorsImpl(boolean directParentOnly) {
Set<String> visited = new HashSet<>();
List<Stage> allAncestorStages = new ArrayList<>();
Queue<Stage> toVisit = new ArrayDeque<>();

if (!requisiteStageRefIds.isEmpty() && !directParentOnly) {
// Get stages this stage depends on via requisiteStageRefIds:
List<Stage> previousStages =
execution.getStages().stream()
.filter(it -> requisiteStageRefIds.contains(it.refId))
.filter(it -> !visited.contains(it.refId))
.collect(toList());
List<Stage> syntheticStages =
execution.getStages().stream()
.filter(
s ->
previousStages.stream()
.map(Stage::getId)
.anyMatch(id -> id.equals(s.parentStageId)))
.collect(toList());
return ImmutableList.<Stage>builder()
.addAll(previousStages)
.addAll(syntheticStages)
.addAll(
previousStages.stream()
.flatMap(it -> it.getAncestorsImpl(visited, directParentOnly).stream())
.collect(toList()))
.build();
} else if (parentStageId != null && !visited.contains(parentStageId)) {
// Get parent stages, but exclude already visited ones:

List<Stage> ancestors = new ArrayList<>();
if (getSyntheticStageOwner() == SyntheticStageOwner.STAGE_AFTER) {
ancestors.addAll(
toVisit.add(this);

while (!toVisit.isEmpty()) {
Stage curStage = toVisit.remove();

if (visited.contains(curStage.id)) {
continue;
}

visited.add(curStage.id);
allAncestorStages.add(curStage);

// 1. Find all stages we depend on through requisiteStageRefIds and push them on the queue
if (!curStage.requisiteStageRefIds.isEmpty() && !directParentOnly) {
List<Stage> refIdPriorStages =
execution.getStages().stream()
.filter(
it ->
parentStageId.equals(it.parentStageId)
&& it.getSyntheticStageOwner() == SyntheticStageOwner.STAGE_BEFORE)
.collect(toList()));
.filter(it -> curStage.requisiteStageRefIds.contains(it.refId))
.collect(toList());

toVisit.addAll(refIdPriorStages);
}

ancestors.addAll(
execution.getStages().stream()
.filter(it -> it.id.equals(parentStageId))
.findFirst()
.<List<Stage>>map(
parent ->
ImmutableList.<Stage>builder()
.add(parent)
.addAll(parent.getAncestorsImpl(visited, directParentOnly))
.build())
.orElse(emptyList()));

return ancestors;
} else {
return emptyList();
// 2. If we have a parent stage
if (curStage.parentStageId != null && !visited.contains(curStage.parentStageId)) {
List<Stage> ancestors = new ArrayList<>();

// 2.1 AND we are an STAGE_AFTER, find all the stages with same parent as us but that are
// marked STAGE_BEFORE
if (curStage.getSyntheticStageOwner() == SyntheticStageOwner.STAGE_AFTER) {
ancestors.addAll(
execution.getStages().stream()
.filter(
it ->
curStage.parentStageId.equals(it.parentStageId)
&& it.getSyntheticStageOwner() == SyntheticStageOwner.STAGE_BEFORE)
.collect(toList()));
}

// 2.2 Add our direct parent to the queue
try {
ancestors.add(curStage.getParent());
} catch (IllegalArgumentException e) {
// It's not really possible for the parent not to exist.. But unittests beg to differ
// Not logging anything here since not having a parent would have failed and loffed in
// other places
}
toVisit.addAll(ancestors);
}
}

return ImmutableList.<Stage>builder().addAll(allAncestorStages).build();
}

/**
Expand Down
6 changes: 5 additions & 1 deletion orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/model/StageContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ public class StageContext extends ForwardingMap<String, Object> {

private final Stage stage;
private final Map<String, Object> delegate;
private List<Stage> ancestors;

public StageContext(Stage stage) {
this(stage, new HashMap<>());
Expand Down Expand Up @@ -56,7 +57,10 @@ public Object get(@Nullable Object key) {
if (delegate().containsKey(key)) {
return super.get(key);
} else {
return stage.ancestors().stream()
if (ancestors == null) {
ancestors = stage.ancestors();
}
return ancestors.stream()
.filter(it -> it.getOutputs().containsKey(key))
.findFirst()
.map(it -> it.getOutputs().get(key))
Expand Down

0 comments on commit 19a1438

Please sign in to comment.