Skip to content
Open
15 changes: 14 additions & 1 deletion src/DynamoCore/Engine/CodeGeneration/AstBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,20 @@ public IEnumerable<Tuple<NodeModel, IEnumerable<AssociativeNode>>> CompileToAstN

if (context == CompilationContext.DeltaExecution)
{
sortedNodes = sortedNodes.Where(n => n.IsModified);
// If any non-input node has been structurally modified (e.g. a wire reconnect),
// the DesignScript compiler will assign it a new, higher PC. Downstream nodes
// that are not recompiled keep their old, lower PCs. Because ApplyUpdate iterates
// graph nodes in ascending PC order, those downstream nodes execute before the
// upstream node that just changed, consuming stale values. The upstream then runs,
// changes its output, and marks them dirty again — causing every affected node to
// execute twice. To prevent this, compile all non-input downstream nodes whenever a
// non-input node is structurally modified. The LiveRunner force-recompile path then
// injects their (structurally unchanged) ASTs into the delta with new, higher PCs
// that respect topological execution order.
bool hasModifiedNonInputNode = sortedNodes.Any(n => n.IsModified && !n.IsInputNode);
sortedNodes = hasModifiedNonInputNode
? sortedNodes.Where(n => n.IsModified || !n.IsInputNode)
: sortedNodes.Where(n => n.IsModified);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

even if that happens, anyNonInputActuallyModified would be false (since SubtreeHasActualChanges would return false for NodeA), so the force-recompile block never fires. The performance cost is real but contained — wasted compilation work, no correctness issue.

Comment thread
johnpierson marked this conversation as resolved.
Outdated
Comment thread
johnpierson marked this conversation as resolved.
Outdated
}

var result = new List<List<AssociativeNode>>();
Expand Down
61 changes: 61 additions & 0 deletions src/Engine/ProtoScript/Runners/LiveRunner.cs
Original file line number Diff line number Diff line change
Expand Up @@ -690,6 +690,29 @@
}
}

/// <summary>
/// Returns true if the subtree's AST nodes differ from the cached version.
/// Used to distinguish structural changes (reconnect) from identity changes (slider value).
/// </summary>
private bool SubtreeHasActualChanges(Subtree subtree)
{
Subtree cached;
if (!currentSubTreeList.TryGetValue(subtree.GUID, out cached) || cached.AstNodes == null)
return subtree.AstNodes != null && subtree.AstNodes.Any();
if (subtree.AstNodes == null)
return false;
foreach (var node in subtree.AstNodes)
{
bool found = false;
foreach (var prevNode in cached.AstNodes)

Check warning on line 707 in src/Engine/ProtoScript/Runners/LiveRunner.cs

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Loops should be simplified using the "Where" LINQ method

See more on https://sonarcloud.io/project/issues?id=DynamoDS_Dynamo&issues=AZ34ymtF27P6wUUnEfTV&open=AZ34ymtF27P6wUUnEfTV&pullRequest=17095
{
if (prevNode.Equals(node)) { found = true; break; }
}
Comment thread
github-code-quality[bot] marked this conversation as resolved.
Fixed
if (!found) return true;
}
return false;
}

/// <summary>
/// Update the cached ASTs in the subtree given the modified ASTs
/// </summary>
Expand Down Expand Up @@ -816,6 +839,16 @@
}
}

// When any non-input subtree has structural changes (e.g. a reconnect), delta
// compilation recompiles that node at a new, higher PC. Unchanged downstream nodes
// keep their old, lower PCs. ApplyUpdate then finds them first (lower PC = earlier
// in the graph list) and runs them with stale upstream values, then the upstream
// node executes and marks them dirty again — causing each downstream node to execute
// twice. To prevent this, force-recompile all unchanged non-input subtrees so they
// receive new, higher PCs that respect the topological execution order.
bool anyNonInputActuallyModified = modifiedSubTrees.Any(
st => !st.IsInput && st.AstNodes != null && SubtreeHasActualChanges(st));

for (int n = 0; n < modifiedSubTrees.Count(); ++n)
{
var modifiedSubTree = modifiedSubTrees[n];
Expand All @@ -832,6 +865,34 @@
var modifiedASTList = GetModifiedNodes(modifiedSubTree, redefinitionAllowed, out modifiedInputAST);
csData.ModifiedNodesForRuntimeSetValue.AddRange(modifiedInputAST);

// If another non-input subtree was structurally modified and this non-input
// subtree appears unchanged, force-recompile it so its new graph node lands
// at a PC higher than the recompiled upstream node. Also deactivate the old
// graph nodes so they do not co-execute with the new ones in ApplyUpdate.
if (!modifiedSubTree.IsInput && anyNonInputActuallyModified
&& !modifiedASTList.Any() && !modifiedInputAST.Any())
{
Comment thread
johnpierson marked this conversation as resolved.
Subtree cachedSubTree;
if (currentSubTreeList.TryGetValue(modifiedSubTree.GUID, out cachedSubTree)
&& cachedSubTree.AstNodes != null)
{
var cachedBinaryNodes = cachedSubTree.AstNodes.OfType<BinaryExpressionNode>().ToList();
// Deactivate the old graph nodes so they are replaced by freshly compiled ones.
csData.RemovedBinaryNodesFromModification.AddRange(cachedBinaryNodes);
// Stamp the GUID and inject directly into the delta list so the compiler
// assigns new, higher PCs — keeping modifiedASTList empty so
// UpdateCachedASTList does not see these as modifications and double
// the cached AST list.
foreach (var bnode in cachedBinaryNodes)
{
bnode.guid = modifiedSubTree.GUID;
bnode.IsInputExpression = false;
SetNestedLanguageBlockASTGuids(modifiedSubTree.GUID, new List<ProtoCore.AST.Node>() { bnode });
}
deltaAstList.AddRange(cachedBinaryNodes);
}
}

modifiedSubTree.ModifiedAstNodes.Clear();
modifiedSubTree.ModifiedAstNodes.AddRange(modifiedASTList);
modifiedSubTree.ModifiedAstNodes.AddRange(modifiedInputAST);
Expand Down
Loading