Skip to content

Dyn-3640 - Recover execution after clearing cyclic dependency from graph #11896

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Aug 10, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/DynamoCore/Graph/Workspaces/WorkspaceModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1267,6 +1267,9 @@ internal void AddAndRegisterNode(NodeModel node, bool centered = false)

HasUnsavedChanges = true;

if (node is CodeBlockNodeModel cbn
&& string.IsNullOrEmpty(cbn.Code)) return;

RequestRun();
}

Expand Down
14 changes: 8 additions & 6 deletions src/Engine/ProtoAssociative/CodeGen.cs
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,8 @@ private List<GraphNode> StrongConnectComponent(GraphNode node, ref int index, Di
return null;
}

private bool CyclicDependencyTest(ProtoCore.AssociativeGraph.GraphNode node, ref SymbolNode cyclicSymbol1, ref SymbolNode cyclicSymbol2)
// Returns true if there is no cyclic dependency, returns false otherwise.
private bool CyclicDependencyTest(GraphNode node, ref SymbolNode cyclicSymbol1, ref SymbolNode cyclicSymbol2)
{
if (null == node || node.updateNodeRefList.Count == 0)
{
Expand All @@ -339,16 +340,15 @@ private bool CyclicDependencyTest(ProtoCore.AssociativeGraph.GraphNode node, ref

var indexMap = new Dictionary<GraphNode, int>();
var lowlinkMap = new Dictionary<GraphNode, int>();
var S = new Stack<GraphNode>();
var s = new Stack<GraphNode>();
int index = 0;

for (int n = 0; n < codeBlock.instrStream.dependencyGraph.GraphList.Count; ++n)
foreach (var subNode in codeBlock.instrStream.dependencyGraph.GraphList)
{
ProtoCore.AssociativeGraph.GraphNode subNode = codeBlock.instrStream.dependencyGraph.GraphList[n];
indexMap[subNode] = Constants.kInvalidIndex;
}

var dependencyList = StrongConnectComponent(node, ref index, lowlinkMap, indexMap, S);
var dependencyList = StrongConnectComponent(node, ref index, lowlinkMap, indexMap, s);
if (dependencyList == null)
{
return true;
Expand All @@ -360,7 +360,7 @@ private bool CyclicDependencyTest(ProtoCore.AssociativeGraph.GraphNode node, ref

foreach (var n in dependencyList.GroupBy(x => x.guid).Select(y => y.First()))
{
core.BuildStatus.LogWarning(ProtoCore.BuildData.WarningID.InvalidStaticCyclicDependency,
core.BuildStatus.LogWarning(WarningID.InvalidStaticCyclicDependency,
ProtoCore.Properties.Resources.kInvalidStaticCyclicDependency, core.CurrentDSFileName, graphNode: n);
}
firstNode.isCyclic = true;
Expand Down Expand Up @@ -5467,6 +5467,8 @@ private void EmitBinaryExpressionNode(AssociativeNode node, ref ProtoCore.Type i
{
if (!CyclicDependencyTest(graphNode, ref cyclicSymbol1, ref cyclicSymbol2))
{
//buildStatus.LogSemanticError("cyclic dependency", core.CurrentDSFileName, node.line, node.col, graphNode);

Validity.Assert(null != cyclicSymbol1);
Validity.Assert(null != cyclicSymbol2);

Expand Down
37 changes: 34 additions & 3 deletions src/Engine/ProtoCore/AssociativeGraph.cs
Original file line number Diff line number Diff line change
Expand Up @@ -892,8 +892,8 @@ public class GraphNode
/// a = b.c.d
///
/// [0] t0 = b -> List empty
/// [1] t1 = t0.b -> List empty
/// [2] t2 = t1.c -> List empty
/// [1] t1 = t0.c -> List empty
/// [2] t2 = t1.d -> List empty
/// [3] a = t2 -> This is the last SSA stmt, its graphnode contains a list of graphnodes {t0,t1,t2}
///
/// </summary>
Expand Down Expand Up @@ -961,6 +961,37 @@ public void PushChildNode(GraphNode child)
child.ParentNodes.Add(this);
}

internal void ClearCycles(IEnumerable<GraphNode> graphNodes)
{
Stack<GraphNode> stack = new Stack<GraphNode>();
stack.Push(this);

var visited = graphNodes.ToDictionary(node => node, node => false);

while(stack.Any())
{
var node = stack.Pop();
if (!visited[node])
{
if (node.isCyclic)
{
node.isCyclic = false;
node.isActive = true;
}

visited[node] = true;
}

foreach(var cNode in node.ChildrenNodes)
{
if (!visited[cNode])
{
stack.Push(cNode);
}
}
}
}

public void PushDependent(GraphNode dependent)
{
if (!allowDependents)
Expand Down Expand Up @@ -1447,7 +1478,7 @@ public class DependencyGraph
private readonly Core core;
private List<GraphNode> graphList;

// For quickly get a list of graph nodes at some scope.
// Get a list of graph nodes at some scope.
private Dictionary<ulong, List<GraphNode>> graphNodeMap;

public List<GraphNode> GraphList
Expand Down
4 changes: 0 additions & 4 deletions src/Engine/ProtoCore/BuildStatus.cs
Original file line number Diff line number Diff line change
Expand Up @@ -663,10 +663,6 @@ public void LogWarning(BuildData.WarningID warningID,
};
warnings.Add(entry);

if (core.Options.IsDeltaExecution)
{
}

if (LogWarnings)
{
#if DEBUG
Expand Down
6 changes: 4 additions & 2 deletions src/Engine/ProtoCore/DSASM/Executive.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1326,7 +1326,7 @@ private int UpdateGraph(int exprUID, bool isSSAAssign)

if (gnode.isCyclic)
{
// If the graphnode is cyclic, mark it as not dirst so it wont get executed
// If the graphnode is cyclic, mark it as not dirty so it wont get executed
// Sets its cyclePoint graphnode to be not dirty so it also doesnt execute.
// The cyclepoint is the other graphNode that the current node cycles with
gnode.isDirty = false;
Expand All @@ -1338,15 +1338,17 @@ private int UpdateGraph(int exprUID, bool isSSAAssign)
}
}
}

// Get all redefined graphnodes
int classScope = Constants.kInvalidIndex;
int functionScope = Constants.kInvalidIndex;
GetCallerInformation(out classScope, out functionScope);

var nodesInScope = istream.dependencyGraph.GetGraphNodesAtScope(classScope, functionScope);

List<AssociativeGraph.GraphNode> redefinedNodes =
AssociativeEngine.Utils.GetRedefinedGraphNodes(runtimeCore, Properties.executingGraphNode, nodesInScope, classScope, functionScope);
Validity.Assert(redefinedNodes != null);

foreach(AssociativeGraph.GraphNode gnode in redefinedNodes)
{
// GC all the temporaries associated with the redefined variable
Expand Down
43 changes: 33 additions & 10 deletions src/Engine/ProtoScript/Runners/LiveRunner.cs
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,8 @@ private void ApplyChangeSetModified(ChangeSetData changeSet)

DeactivateGraphnodes(changeSet.RemovedBinaryNodesFromModification);

ReActivateGraphNodesInCycle(changeSet.RemovedBinaryNodesFromModification);

// Undefine a function that was removed
UndefineFunctions(changeSet.RemovedFunctionDefNodesFromModification);

Expand Down Expand Up @@ -254,6 +256,35 @@ private void ApplyChangeSetForceExecute(ChangeSetData changeSet)
}
}

private void ReActivateGraphNodesInCycle(List<AssociativeNode> nodeList)
{
var assocGraph = core.DSExecutable.instrStreamList[0].dependencyGraph;
var graphNodes = assocGraph.GetGraphNodesAtScope(Constants.kInvalidIndex, Constants.kInvalidIndex);

foreach (var node in nodeList)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will nodeList usually be quite small?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are only those nodes that are modified in a single delta execution run. I guess it could be a long list if the user is in manual mode and has made a large number of edits to their graph before hitting Run.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worth profiling this change or monitoring the perf benchmark tests after this goes in to see if there is a significant slowdown as a result of this additional check on each delta execution.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

{
var bNode = node as BinaryExpressionNode;
if (bNode == null) continue;

var identifier = bNode.LeftNode as IdentifierNode;
if (identifier == null) continue;

GraphNode rootNode = null;
foreach(var gNode in graphNodes)
Copy link
Member

@mjkkirschner mjkkirschner Jul 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this all graphNodes in the entire graph?

Copy link
Contributor Author

@aparajit-pratap aparajit-pratap Jul 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, graphNodes are only those that are in global scope (class index and function index = -1 or invalid index).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't think of a case where we can have a cycle in a single code block node scope or function definition scope as we disallow variable redefinitions in code block nodes 🤔

{
if(identifier.Value == gNode.updateNodeRefList[0].nodeList[0].symbol.name)
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these always valid ?
gNode.updateNodeRefList[0].nodeList[0]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

rootNode = gNode;
break;
}
}
if (rootNode == null) return;

// walk the dependency graph for the rootNode and clear cycles from dependent graph nodes
rootNode.ClearCycles(graphNodes);
}
}

/// <summary>
/// Deactivate a single graphnode regardless of its associated dependencies
/// </summary>
Expand Down Expand Up @@ -1401,21 +1432,13 @@ private bool CompileAndExecute(List<AssociativeNode> astList)
private void PostExecution()
{
ApplyUpdate();
HandleWarnings();
}

/// <summary>
/// Handle warnings that will be reported to the frontend
/// </summary>
private void HandleWarnings()
{
SuppressResovledUnboundVariableWarnings();
SuppressResolvedUnboundVariableWarnings();
}

/// <summary>
/// Removes all warnings that were initially unbound variables but were resolved at runtime
/// </summary>
private void SuppressResovledUnboundVariableWarnings()
private void SuppressResolvedUnboundVariableWarnings()
{
runnerCore.BuildStatus.RemoveUnboundVariableWarnings(runnerCore.DSExecutable.UpdatedSymbols);

Expand Down