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 all 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
15 changes: 12 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)
{
int startpc = pc;

Expand All @@ -5928,9 +5929,17 @@ 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);
}
protected override void EmitReturnNull()
{
EmitReturnNull(null);
}

private ProtoCore.Type BuildArgumentTypeFromVarDeclNode(VarDeclNode argNode, GraphNode graphNode = null)
{
Expand Down Expand Up @@ -6130,4 +6139,4 @@ protected override void DfsTraverse(ProtoCore.AST.Node pNode, ref ProtoCore.Type
int blockId = codeBlock.codeBlockId;
}
}
}
}
19 changes: 11 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,31 @@ 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.Add(node.UID))
{
guids.Add(node.guid);
if (node.isCyclic)
{
node.isCyclic = false;
node.isActive = true;

}
visited[node] = true;
}

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
1 change: 1 addition & 0 deletions src/Engine/ProtoCore/CodeGen.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2363,6 +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(Guid? guid);
protected abstract void EmitReturnNull();

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

protected override void EmitReturnNull()
protected override void EmitReturnNull(Guid? guid = null)
{
EmitPushNull();
EmitReturnToRegister();
}

protected override void EmitReturnNull()
{
EmitReturnNull(null);
}

protected void EmitGropuExpressionNode(ImperativeNode node, ref ProtoCore.Type inferedType)
{
GroupExpressionNode group = node as GroupExpressionNode;
Expand Down
15 changes: 15 additions & 0 deletions src/Engine/ProtoScript/Runners/LiveRunner.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1709,6 +1709,13 @@ private void SynchronizeInternal(GraphSyncData syncData)
// Get AST list that need to be executed
var finalDeltaAstList = changeSetComputer.GetDeltaASTList(syncData);

//nodes which will be defined or redefined after compilation and execution
var pendingUIDsForDeletion = changeSetComputer.csData.DeletedBinaryExprASTNodes
.Select(x => (x as BinaryExpressionNode)?.guid)
.Where(x => x != null)
.Select(x => x.Value)
.ToHashSet();

// Prior to execution, apply state modifications to the VM given the delta AST's
bool anyForcedExecutedNodes = changeSetComputer.csData.ForceExecuteASTList.Any();
changeSetApplier.Apply(runnerCore, runtimeCore, changeSetComputer.csData);
Expand All @@ -1721,6 +1728,14 @@ private void SynchronizeInternal(GraphSyncData syncData)
var guids = runtimeCore.ExecutedAstGuids.ToList();
executedAstGuids[syncData.SessionID] = guids;
runtimeCore.RemoveExecutedAstGuids();

// There should be a CodeBlock in CodeBlockList by now
if (runnerCore.CodeBlockList.Any())
{
var nodes = runnerCore.CodeBlockList[(int)Language.Associative].instrStream.dependencyGraph.GetGraphNodesAtScope(Constants.kInvalidPC, Constants.kInvalidPC);
//delete all nodes which were redefined. For those nodes that were defined for the first time, this will be a no-op
nodes.RemoveAll(x=>!x.isActive || pendingUIDsForDeletion.Contains(x.guid));
}
}
}

Expand Down
Loading