From 7e058ddf15a3f43327a61dd05f15ea2ebebd2e37 Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Mon, 20 Jan 2025 17:44:31 +0100 Subject: [PATCH 1/3] Refactor workflow context and execution handling. Optimized activity execution context management by introducing parent-child relationships and improving immutability. Adjusted several APIs to enhance clarity, performance, and maintainability, including the use of `AsReadOnly` collections and removal of redundant code. --- .../Flowchart/Activities/Flowchart.cs | 12 +++---- .../ActivityExecutionContext.Complete.cs | 6 ++-- ...ivityExecutionContext.ExecutionLogEntry.cs | 2 +- .../Contexts/ActivityExecutionContext.cs | 15 +++++++-- .../Contexts/WorkflowExecutionContext.cs | 2 +- .../ActivityExecutionContextExtensions.cs | 2 +- .../Models/ActivityNode.cs | 32 +++++++++++++++---- .../Services/ActivityVisitor.cs | 6 ++-- .../Services/QueueBasedActivityScheduler.cs | 2 +- 9 files changed, 53 insertions(+), 26 deletions(-) diff --git a/src/modules/Elsa.Workflows.Core/Activities/Flowchart/Activities/Flowchart.cs b/src/modules/Elsa.Workflows.Core/Activities/Flowchart/Activities/Flowchart.cs index 4ee602a37..5824451ac 100644 --- a/src/modules/Elsa.Workflows.Core/Activities/Flowchart/Activities/Flowchart.cs +++ b/src/modules/Elsa.Workflows.Core/Activities/Flowchart/Activities/Flowchart.cs @@ -31,9 +31,7 @@ public class Flowchart : Container /// /// The activity to execute when the flowchart starts. /// - [Port] - [Browsable(false)] - public IActivity? Start { get; set; } + [Port] [Browsable(false)] public IActivity? Start { get; set; } /// /// A list of connections between activities. @@ -98,8 +96,8 @@ public class Flowchart : Container { var workflowExecutionContext = context.WorkflowExecutionContext; var activityIds = Activities.Select(x => x.Id).ToList(); - var descendantContexts = context.GetDescendents().Where(x => x.ParentActivityExecutionContext == context).ToList(); - var activityExecutionContexts = descendantContexts.Where(x => activityIds.Contains(x.Activity.Id)).ToList(); + var descendantContexts = context.GetDescendents().Where(x => x.ParentActivityExecutionContext == context); + var hasRunningActivityInstances = descendantContexts.Where(x => activityIds.Contains(x.Activity.Id)).Any(x => x.Status == ActivityStatus.Running); var hasPendingWork = workflowExecutionContext.Scheduler.List().Any(workItem => { @@ -117,8 +115,6 @@ public class Flowchart : Container return ancestors.Any(x => x == context); }); - var hasRunningActivityInstances = activityExecutionContexts.Any(x => x.Status == ActivityStatus.Running); - return hasRunningActivityInstances || hasPendingWork; } @@ -186,7 +182,7 @@ public class Flowchart : Container var executionCount = scope.GetExecutionCount(activity); var haveInboundActivitiesExecuted = inboundActivities.All(x => scope.GetExecutionCount(x) > executionCount); - if (haveInboundActivitiesExecuted) + if (haveInboundActivitiesExecuted) await flowchartContext.ScheduleActivityAsync(activity, OnChildCompletedAsync); } else diff --git a/src/modules/Elsa.Workflows.Core/Contexts/ActivityExecutionContext.Complete.cs b/src/modules/Elsa.Workflows.Core/Contexts/ActivityExecutionContext.Complete.cs index 978c64d91..1a278d1db 100644 --- a/src/modules/Elsa.Workflows.Core/Contexts/ActivityExecutionContext.Complete.cs +++ b/src/modules/Elsa.Workflows.Core/Contexts/ActivityExecutionContext.Complete.cs @@ -9,7 +9,7 @@ public partial class ActivityExecutionContext /// /// Complete the current activity. This should only be called by activities that explicitly suppress automatic-completion. /// - public async ValueTask CompleteActivityAsync(object? result = default) + public async ValueTask CompleteActivityAsync(object? result = null) { var outcomes = result as Outcomes; @@ -28,8 +28,8 @@ public partial class ActivityExecutionContext return; // Cancel any non-completed child activities. - var childContexts = WorkflowExecutionContext.ActivityExecutionContexts.Where(x => x.ParentActivityExecutionContext == this && x.CanCancelActivity()).ToList(); - + var childContexts = Children.Where(x => x.CanCancelActivity()).ToList(); + foreach (var childContext in childContexts) await childContext.CancelActivityAsync(); diff --git a/src/modules/Elsa.Workflows.Core/Contexts/ActivityExecutionContext.ExecutionLogEntry.cs b/src/modules/Elsa.Workflows.Core/Contexts/ActivityExecutionContext.ExecutionLogEntry.cs index e05a262c6..260733811 100644 --- a/src/modules/Elsa.Workflows.Core/Contexts/ActivityExecutionContext.ExecutionLogEntry.cs +++ b/src/modules/Elsa.Workflows.Core/Contexts/ActivityExecutionContext.ExecutionLogEntry.cs @@ -13,7 +13,7 @@ public partial class ActivityExecutionContext /// The source of the activity. For example, the source file name and line number in case of composite activities. /// Any contextual data related to this event. /// Returns the created . - public WorkflowExecutionLogEntry AddExecutionLogEntry(string eventName, string? message = default, string? source = default, object? payload = default) + public WorkflowExecutionLogEntry AddExecutionLogEntry(string eventName, string? message = null, string? source = null, object? payload = null) { var logEntry = new WorkflowExecutionLogEntry( Id, diff --git a/src/modules/Elsa.Workflows.Core/Contexts/ActivityExecutionContext.cs b/src/modules/Elsa.Workflows.Core/Contexts/ActivityExecutionContext.cs index 5393a9f80..099f71c40 100644 --- a/src/modules/Elsa.Workflows.Core/Contexts/ActivityExecutionContext.cs +++ b/src/modules/Elsa.Workflows.Core/Contexts/ActivityExecutionContext.cs @@ -21,6 +21,7 @@ public partial class ActivityExecutionContext : IExecutionContext, IDisposable private readonly ISystemClock _systemClock; private readonly List _bookmarks = []; private long _executionCount; + private ActivityExecutionContext? _parentActivityExecutionContext; /// /// Initializes a new instance of the class. @@ -39,7 +40,7 @@ public partial class ActivityExecutionContext : IExecutionContext, IDisposable { _systemClock = systemClock; WorkflowExecutionContext = workflowExecutionContext; - ParentActivityExecutionContext = parentActivityExecutionContext; + _parentActivityExecutionContext = parentActivityExecutionContext; ExpressionExecutionContext = expressionExecutionContext; Activity = activity; ActivityDescriptor = activityDescriptor; @@ -84,7 +85,15 @@ public partial class ActivityExecutionContext : IExecutionContext, IDisposable /// /// The parent activity execution context, if any. /// - public ActivityExecutionContext? ParentActivityExecutionContext { get; internal set; } + public ActivityExecutionContext? ParentActivityExecutionContext + { + get => _parentActivityExecutionContext; + internal set + { + _parentActivityExecutionContext = value; + _parentActivityExecutionContext?.Children.Add(this); + } + } /// /// The expression execution context. @@ -159,6 +168,8 @@ public partial class ActivityExecutionContext : IExecutionContext, IDisposable /// /// As of tool version 3.0, all activity Ids are already unique, so there's no need to construct a hierarchical ID public string NodeId => ActivityNode.NodeId; + + public ISet Children { get; } = new HashSet(); /// /// A list of bookmarks created by the current activity. diff --git a/src/modules/Elsa.Workflows.Core/Contexts/WorkflowExecutionContext.cs b/src/modules/Elsa.Workflows.Core/Contexts/WorkflowExecutionContext.cs index 545ba3bbc..56788e661 100644 --- a/src/modules/Elsa.Workflows.Core/Contexts/WorkflowExecutionContext.cs +++ b/src/modules/Elsa.Workflows.Core/Contexts/WorkflowExecutionContext.cs @@ -346,7 +346,7 @@ public partial class WorkflowExecutionContext : IExecutionContext /// public IReadOnlyCollection ActivityExecutionContexts { - get => _activityExecutionContexts.ToList(); + get => _activityExecutionContexts.AsReadOnly(); internal set => _activityExecutionContexts = value.ToList(); } diff --git a/src/modules/Elsa.Workflows.Core/Extensions/ActivityExecutionContextExtensions.cs b/src/modules/Elsa.Workflows.Core/Extensions/ActivityExecutionContextExtensions.cs index 090cff8b8..edc7dafd9 100644 --- a/src/modules/Elsa.Workflows.Core/Extensions/ActivityExecutionContextExtensions.cs +++ b/src/modules/Elsa.Workflows.Core/Extensions/ActivityExecutionContextExtensions.cs @@ -202,7 +202,7 @@ public static partial class ActivityExecutionContextExtensions /// public static IEnumerable GetDescendents(this ActivityExecutionContext context) { - var children = context.WorkflowExecutionContext.ActivityExecutionContexts.Where(x => x.ParentActivityExecutionContext == context).ToList(); + var children = context.WorkflowExecutionContext.ActivityExecutionContexts.Where(x => x.ParentActivityExecutionContext == context); foreach (var child in children) { diff --git a/src/modules/Elsa.Workflows.Core/Models/ActivityNode.cs b/src/modules/Elsa.Workflows.Core/Models/ActivityNode.cs index 44e39ac00..3768857cb 100644 --- a/src/modules/Elsa.Workflows.Core/Models/ActivityNode.cs +++ b/src/modules/Elsa.Workflows.Core/Models/ActivityNode.cs @@ -5,6 +5,10 @@ namespace Elsa.Workflows.Models; /// public class ActivityNode { + private readonly List _parents = new(); + private readonly List _children = new(); + private string? _nodeId; + /// /// Initializes a new instance of the class. /// @@ -23,8 +27,13 @@ public class ActivityNode { get { - var ancestorIds = Ancestors().Reverse().Select(x => x.Activity.Id).ToList(); - return ancestorIds.Any() ? $"{string.Join(":", ancestorIds)}:{Activity.Id}" : Activity.Id; + if (_nodeId == null) + { + var ancestorIds = Ancestors().Reverse().Select(x => x.Activity.Id).ToList(); + _nodeId = ancestorIds.Any() ? $"{string.Join(":", ancestorIds)}:{Activity.Id}" : Activity.Id; + } + + return _nodeId; } } @@ -41,12 +50,23 @@ public class ActivityNode /// /// Gets the parents of this node. /// - public ICollection Parents { get; set; } = new List(); - + public IReadOnlyCollection Parents => _parents.AsReadOnly(); + /// /// Gets the children of this node. /// - public ICollection Children { get; set; } = new List(); + public ICollection Children => _children.AsReadOnly(); + + public void AddParent(ActivityNode parent) + { + _parents.Add(parent); + _nodeId = null; + } + + public void AddChild(ActivityNode child) + { + _children.Add(child); + } /// /// Gets the descendants of this node. @@ -85,7 +105,7 @@ public class ActivityNode /// Gets the siblings of this node. /// public IEnumerable Siblings() => Parents.SelectMany(parent => parent.Children); - + /// /// Gets the siblings and cousins of this node. /// diff --git a/src/modules/Elsa.Workflows.Core/Services/ActivityVisitor.cs b/src/modules/Elsa.Workflows.Core/Services/ActivityVisitor.cs index fcdb66519..c95c7beec 100644 --- a/src/modules/Elsa.Workflows.Core/Services/ActivityVisitor.cs +++ b/src/modules/Elsa.Workflows.Core/Services/ActivityVisitor.cs @@ -71,12 +71,12 @@ public class ActivityVisitor : IActivityVisitor if (childNode == null) { - childNode = new ActivityNode(activity, activityPort.PortName); + childNode = new(activity, activityPort.PortName); collectedNodes.Add(childNode); } - childNode.Parents.Add(pair.Node); - pair.Node.Children.Add(childNode); + childNode.AddParent(pair.Node); + pair.Node.AddChild(childNode); collectedActivities.Add(activity); await VisitRecursiveAsync((childNode, activity), visitorContext, cancellationToken); } diff --git a/src/modules/Elsa.Workflows.Core/Services/QueueBasedActivityScheduler.cs b/src/modules/Elsa.Workflows.Core/Services/QueueBasedActivityScheduler.cs index b13584b05..47b56e7fb 100644 --- a/src/modules/Elsa.Workflows.Core/Services/QueueBasedActivityScheduler.cs +++ b/src/modules/Elsa.Workflows.Core/Services/QueueBasedActivityScheduler.cs @@ -21,7 +21,7 @@ public class QueueBasedActivityScheduler : IActivityScheduler public ActivityWorkItem Take() => _queue.Dequeue(); /// - public IEnumerable List() => _queue.ToList(); + public IEnumerable List() => _queue; /// public bool Any(Func predicate) => _queue.Any(predicate); From a49bf0c05fb467de6460c81df70eee49f5ac5eb2 Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Mon, 20 Jan 2025 18:07:14 +0100 Subject: [PATCH 2/3] Refactor descendant context check in Flowchart activity. Replaced usage of `GetDescendents` with `Children` property for better clarity and efficiency when checking running activity instances. This simplifies the logic and aligns with the existing structure of activity context handling. --- .../Activities/Flowchart/Activities/Flowchart.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/modules/Elsa.Workflows.Core/Activities/Flowchart/Activities/Flowchart.cs b/src/modules/Elsa.Workflows.Core/Activities/Flowchart/Activities/Flowchart.cs index 5824451ac..022ff7a9e 100644 --- a/src/modules/Elsa.Workflows.Core/Activities/Flowchart/Activities/Flowchart.cs +++ b/src/modules/Elsa.Workflows.Core/Activities/Flowchart/Activities/Flowchart.cs @@ -96,8 +96,8 @@ public class Flowchart : Container { var workflowExecutionContext = context.WorkflowExecutionContext; var activityIds = Activities.Select(x => x.Id).ToList(); - var descendantContexts = context.GetDescendents().Where(x => x.ParentActivityExecutionContext == context); - var hasRunningActivityInstances = descendantContexts.Where(x => activityIds.Contains(x.Activity.Id)).Any(x => x.Status == ActivityStatus.Running); + var children = context.Children; + var hasRunningActivityInstances = children.Where(x => activityIds.Contains(x.Activity.Id)).Any(x => x.Status == ActivityStatus.Running); var hasPendingWork = workflowExecutionContext.Scheduler.List().Any(workItem => { From d31661926d00a302dd4d6639ea345e82988303dc Mon Sep 17 00:00:00 2001 From: Sipke Schoorstra Date: Mon, 20 Jan 2025 18:29:13 +0100 Subject: [PATCH 3/3] Simplify activity context handling in workflows. Replaced `GetActiveChildren` with a direct `Children` property in `Flowchart.cs` to streamline logic. Removed redundant `GetDescendents`, `GetActiveChildren`, and `GetChildren` methods from `ActivityExecutionContextExtensions.cs`. Also updated GitHub workflows to allow performance-related branches. --- .github/workflows/packages.yml | 1 + .../Flowchart/Activities/Flowchart.cs | 2 +- .../ActivityExecutionContextExtensions.cs | 44 ------------------- 3 files changed, 2 insertions(+), 45 deletions(-) diff --git a/.github/workflows/packages.yml b/.github/workflows/packages.yml index f594544c1..df4fbec7c 100644 --- a/.github/workflows/packages.yml +++ b/.github/workflows/packages.yml @@ -5,6 +5,7 @@ on: branches: - 'main' - 'bug/*' + - 'perf/*' release: types: [ prereleased, published ] env: diff --git a/src/modules/Elsa.Workflows.Core/Activities/Flowchart/Activities/Flowchart.cs b/src/modules/Elsa.Workflows.Core/Activities/Flowchart/Activities/Flowchart.cs index 022ff7a9e..3fa0921ea 100644 --- a/src/modules/Elsa.Workflows.Core/Activities/Flowchart/Activities/Flowchart.cs +++ b/src/modules/Elsa.Workflows.Core/Activities/Flowchart/Activities/Flowchart.cs @@ -227,7 +227,7 @@ public class Flowchart : Container if (!hasPendingWork) { - var hasFaultedActivities = context.GetActiveChildren().Any(x => x.Status == ActivityStatus.Faulted); + var hasFaultedActivities = context.Children.Any(x => x.Status == ActivityStatus.Faulted); if (!hasFaultedActivities) { diff --git a/src/modules/Elsa.Workflows.Core/Extensions/ActivityExecutionContextExtensions.cs b/src/modules/Elsa.Workflows.Core/Extensions/ActivityExecutionContextExtensions.cs index edc7dafd9..739b0d8eb 100644 --- a/src/modules/Elsa.Workflows.Core/Extensions/ActivityExecutionContextExtensions.cs +++ b/src/modules/Elsa.Workflows.Core/Extensions/ActivityExecutionContextExtensions.cs @@ -197,50 +197,6 @@ public static partial class ActivityExecutionContextExtensions } } - /// - /// Returns a flattened list of the current context's descendants. - /// - public static IEnumerable GetDescendents(this ActivityExecutionContext context) - { - var children = context.WorkflowExecutionContext.ActivityExecutionContexts.Where(x => x.ParentActivityExecutionContext == context); - - foreach (var child in children) - { - yield return child; - - foreach (var descendent in GetDescendents(child)) - yield return descendent; - } - } - - /// - /// Returns a flattened list of the current context's immediate active children. - /// - public static IEnumerable GetActiveChildren(this ActivityExecutionContext context) => - context.WorkflowExecutionContext.ActivityExecutionContexts.Where(x => x.ParentActivityExecutionContext == context); - - /// - /// Returns a flattened list of the current context's immediate children. - /// - public static IEnumerable GetChildren(this ActivityExecutionContext context) => - context.WorkflowExecutionContext.ActivityExecutionContexts.Where(x => x.ParentActivityExecutionContext == context); - - /// - /// Returns a flattened list of the current context's descendants. - /// - public static IEnumerable GetDescendants(this ActivityExecutionContext context) - { - var children = context.WorkflowExecutionContext.ActivityExecutionContexts.Where(x => x.ParentActivityExecutionContext == context).ToList(); - - foreach (var child in children) - { - yield return child; - - foreach (var descendant in child.GetDescendants()) - yield return descendant; - } - } - /// /// Send a signal up the current hierarchy of ancestors. ///