Skip to content
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

DYN-8358 cleanup dependency graph from old nodes #15869

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 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
11 changes: 8 additions & 3 deletions src/Engine/ProtoAssociative/CodeGen.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2156,7 +2156,8 @@ public override int Emit(ProtoCore.AST.Node codeBlockNode, ProtoCore.Associative
hasReturnStatement = EmitCodeBlock(codeblock.Body, ref inferedType, ProtoCore.CompilerDefinitions.SubCompilePass.UnboundIdentifier, false);
if (compilePass == ProtoCore.CompilerDefinitions.CompilePass.GlobalScope && !hasReturnStatement)
{
EmitReturnNull();
var guidList = codeblock.Body.Select(x=> (x as BinaryExpressionNode)?.guid).Where(x=>x!=null).ToList();
EmitReturnNull(guidList.Any() ? guidList.Last() : null);
}

compilePass++;
Expand Down Expand Up @@ -5909,7 +5910,7 @@ private void EmitGroupExpressionNode(AssociativeNode node, ref ProtoCore.Type in
}
}

protected override void EmitReturnNull()
protected override void EmitReturnNull(Guid? guid = null)
{
int startpc = pc;

Expand All @@ -5928,6 +5929,10 @@ protected override void EmitReturnNull()
retNode.updateBlock.startpc = startpc;
retNode.updateBlock.endpc = pc - 1;
retNode.isReturn = true;
if (guid != null)
{
retNode.guid = (Guid)guid;
}

PushGraphNode(retNode);
}
Expand Down Expand Up @@ -6130,4 +6135,4 @@ protected override void DfsTraverse(ProtoCore.AST.Node pNode, ref ProtoCore.Type
int blockId = codeBlock.codeBlockId;
}
}
}
}
42 changes: 34 additions & 8 deletions src/Engine/ProtoCore/AssociativeGraph.cs
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,12 @@ public static void BuildGraphNodeDependencies(List<AssociativeGraph.GraphNode> g
{
return;
}

for (int i = 0; i < graphNodesInScope.Count; ++i)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you add comments to this block, seems pretty important for others to understand

Copy link
Contributor Author

@chubakueno chubakueno Feb 28, 2025

Choose a reason for hiding this comment

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

The ChildrenNodes and ParentNodes are being rebuilt every time, but before this PR they were accumulating more dependencies each time BuildGraphNodeDependencies was called. So this is more of a bug correction.

{
AssociativeGraph.GraphNode currentNode = graphNodesInScope[i];
currentNode.ParentNodes.Clear();
currentNode.ChildrenNodes.Clear();
}
// Get the current graphnode to check against the list
// [a = 10] -> this one
// c = 1
Expand Down Expand Up @@ -967,33 +972,32 @@ internal IEnumerable<Guid> ClearCycles(IEnumerable<GraphNode> graphNodes)
Stack<GraphNode> stack = new Stack<GraphNode>();
stack.Push(this);

var visited = graphNodes.ToDictionary(node => node, node => false);
var visited = new HashSet<int>();

var guids = new List<Guid>();
var guids = new HashSet<Guid>();
while(stack.Any())
{
var node = stack.Pop();
if (!visited[node])
if (!visited.Contains(node.UID))
{
guids.Add(node.guid);
if (node.isCyclic)
{
node.isCyclic = false;
node.isActive = true;

}
visited[node] = true;
visited.Add(node.UID);
}

foreach(var cNode in node.ChildrenNodes)
{
if (!visited[cNode])
if (!visited.Contains(cNode.UID))
{
stack.Push(cNode);
}
}
}
return guids;
return guids.ToList();
}

public void PushDependent(GraphNode dependent)
Expand Down Expand Up @@ -1500,6 +1504,28 @@ private ulong GetGraphNodeKey(int classIndex, int procIndex)
return (((ulong)ci) << 32) | pi;
}

internal void Purge(List<AST.AssociativeAST.AssociativeNode> oldNodes)
{
HashSet<Guid> oldGuids = [];
foreach (AST.AssociativeAST.AssociativeNode astNode in oldNodes)
{
var x = (astNode as AST.AssociativeAST.BinaryExpressionNode)?.guid;
if (x != null)
{
oldGuids.Add(x.Value);
}
}
if(oldGuids.Count == 0)
{
return;
}
List<GraphNode> nodeList;
if(graphNodeMap.TryGetValue(GetGraphNodeKey(Constants.kInvalidPC, Constants.kInvalidPC), out nodeList))
{
nodeList.RemoveAll(node => oldGuids.Contains(node.guid));
}
}

public DependencyGraph(Core core)
{
this.core = core;
Expand Down
2 changes: 1 addition & 1 deletion src/Engine/ProtoCore/CodeGen.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2363,7 +2363,7 @@ protected bool InsideFunction()

// used to manully emit "return = null" instruction if a function or language block does not have a return statement
// there is update code involved in associativen code gen, so it is not implemented here
protected abstract void EmitReturnNull();
protected abstract void EmitReturnNull(Guid? guid = null);

protected abstract void DfsTraverse(Node node, ref ProtoCore.Type inferedType, bool isBooleanOp = false, ProtoCore.AssociativeGraph.GraphNode graphNode = null,
ProtoCore.CompilerDefinitions.SubCompilePass subPass = ProtoCore.CompilerDefinitions.SubCompilePass.None, ProtoCore.AST.Node parentNode = null);
Expand Down
2 changes: 1 addition & 1 deletion src/Engine/ProtoImperative/CodeGen.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2543,7 +2543,7 @@ private void EmitSetExpressionUID(int exprId)
//AppendInstruction(instr);
}

protected override void EmitReturnNull()
protected override void EmitReturnNull(Guid? guid = null)
{
EmitPushNull();
EmitReturnToRegister();
Expand Down
3 changes: 3 additions & 0 deletions src/Engine/ProtoScript/Runners/LiveRunner.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1660,6 +1660,7 @@ private void CompileAndExecuteForDeltaExecution(List<AssociativeNode> astList)
}

ResetForDeltaExecution();
runnerCore.CodeBlockList[(int)Language.Associative].instrStream.dependencyGraph.Purge(dispatchASTList);
CompileAndExecute(dispatchASTList);
PostExecution();
}
Expand Down Expand Up @@ -1721,6 +1722,8 @@ private void SynchronizeInternal(GraphSyncData syncData)
var guids = runtimeCore.ExecutedAstGuids.ToList();
executedAstGuids[syncData.SessionID] = guids;
runtimeCore.RemoveExecutedAstGuids();

runnerCore.CodeBlockList[(int)Language.Associative].instrStream.dependencyGraph.Purge(changeSetComputer.csData.DeletedBinaryExprASTNodes);
}
}

Expand Down
Loading