Skip to content

Commit

Permalink
perf(pipeline): Improve execution times for dense pipeline graphs (#4824
Browse files Browse the repository at this point in the history
)

* test(pipeline): Define StartStageHandler performance

when given a complex pipeline with multiple layers of upstream stages

* fix(pipeline): Memoize anyUpstreamStagesFailed results

This turns the anyUpstreamStagesFailed calculation from one that
scales exponentially based on the number of (branches+downstream stages)
in a pipeline to one that scales linearly based on the total number
of stages in a pipeline. This is a significant performance improvement,
especially for very large and complicated pipelines

* refactor(stage): Move anyUpstreamStagesFailed to StartStageHandler

since that's the only place where it gets used

* fix(stage): Avoid using a ConcurrentHashMap

for memoization. The recursive anyUpstreamStagesFailed(StageExecution)
function runs in a single thread, so ConcurrentHashMap is not
necessary here

* docs(test): Use a more concise test name

* perf(stage): First check if a stage has been visited

before checking for parent stages. stage.getRequisiteStageRefIds
is a more expensive call because the underlying implementation creates
a copy of a List. Therefore, start with the cheaper operation first
hoping to short-circuit and avoid the more expensive check

* perf(stage): Filter out non-synthetic stages

The javadocs state that the syntheticStageOwner property is null
for non-synthetic stages. Use this information to filter out non-synthetic
stages before attempting a potentially expensive operation
to check for synthetic parents of previousStages

* perf(stage): Precompute requisiteStageRefIds

StageExecutionImpl.getRequisiteStageRefIds() returns a copy of a
Set. This is a costly operation that has the potential to get repeated
for every unvisited stage. To avoid this, compute the value before
entering a loop

* perf(stage): Only use withAuth when needed

withAuth is only necessary when starting a stage. Since withAuth is
very computationally expensive for complex pipelines, only call it
when it is absolutely necessary.

* perf(stage): Remove duplicate call to withStage

StartStageHandler already makes a call to message.withStage at the
beginning of the handle() method. Therefore, this call within the
catch block is unnecessary

* chore(import): Clean up unused imports

---------

Co-authored-by: Daniel Zheng <d.zheng@salesforce.com>
  • Loading branch information
dzhengg and Daniel Zheng authored Jan 17, 2025
1 parent ab81739 commit 93f27cc
Show file tree
Hide file tree
Showing 5 changed files with 644 additions and 34 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import com.netflix.spinnaker.orca.api.pipeline.SyntheticStageOwner;
import com.netflix.spinnaker.orca.api.pipeline.models.StageExecution;
import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.Set;

Expand All @@ -41,15 +42,17 @@ static List<StageExecution> getAncestorsImpl(
StageExecution stage, Set<String> visited, boolean directParentOnly) {
visited.add(stage.getRefId());

if (!stage.getRequisiteStageRefIds().isEmpty() && !directParentOnly) {
if (!directParentOnly && !stage.getRequisiteStageRefIds().isEmpty()) {
// Get stages this stage depends on via requisiteStageRefIds:
Collection<String> requisiteStageRefIds = stage.getRequisiteStageRefIds();
List<StageExecution> previousStages =
stage.getExecution().getStages().stream()
.filter(it -> stage.getRequisiteStageRefIds().contains(it.getRefId()))
.filter(it -> !visited.contains(it.getRefId()))
.filter(it -> requisiteStageRefIds.contains(it.getRefId()))
.collect(toList());
List<StageExecution> syntheticStages =
stage.getExecution().getStages().stream()
.filter(s -> s.getSyntheticStageOwner() != null)
.filter(
s ->
previousStages.stream()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import com.netflix.spinnaker.orca.api.pipeline.SyntheticStageOwner.STAGE_BEFORE
import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus
import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus.CANCELED
import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus.FAILED_CONTINUE
import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus.NOT_STARTED
import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus.SKIPPED
import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus.STOPPED
import com.netflix.spinnaker.orca.api.pipeline.models.ExecutionStatus.SUCCEEDED
Expand Down Expand Up @@ -83,9 +82,6 @@ fun StageExecution.upstreamStages(): List<StageExecution> =
fun StageExecution.allUpstreamStagesComplete(): Boolean =
upstreamStages().all { it.status in listOf(SUCCEEDED, FAILED_CONTINUE, SKIPPED) }

fun StageExecution.anyUpstreamStagesFailed(): Boolean =
upstreamStages().any { it.status in listOf(TERMINAL, STOPPED, CANCELED) || it.status == NOT_STARTED && it.anyUpstreamStagesFailed() }

fun StageExecution.syntheticStages(): List<StageExecution> =
execution.stages.filter { it.parentStageId == id }

Expand Down
Loading

0 comments on commit 93f27cc

Please sign in to comment.