DYN-8358 cleanup dependency graph from old nodes #15869
Conversation
| @@ -57,7 +57,12 @@ public static void BuildGraphNodeDependencies(List<AssociativeGraph.GraphNode> g | |||
| { | |||
| return; | |||
| } | |||
|
|
|||
| for (int i = 0; i < graphNodesInScope.Count; ++i) | |||
There was a problem hiding this comment.
Would you add comments to this block, seems pretty important for others to understand
There was a problem hiding this comment.
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.
| @@ -1500,6 +1504,28 @@ private ulong GetGraphNodeKey(int classIndex, int procIndex) | |||
| return (((ulong)ci) << 32) | pi; | |||
| } | |||
|
|
|||
| internal void Purge(List<AST.AssociativeAST.AssociativeNode> oldNodes) | |||
There was a problem hiding this comment.
Maybe add some comments here? In general, the team try to have all the new functions covered by comments
There was a problem hiding this comment.
Looks solid to me, waiting for @aparajit-pratap @saintentropy to comment
|
The Jira ticket check fails when Jira issue is not part of the PR title, I will do an example fix for you. When Jira issue correctly referenced in PR title, this PR will be linked to Jira task correctly for future reference |
QilongTang
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-8358
|
seems like some real test failures. |
yup, @chubakueno are you able to access PR check test failures? |
|
|
||
| var guids = new List<Guid>(); | ||
| var guids = new HashSet<Guid>(); |
There was a problem hiding this comment.
perhaps guids should remain a list ? we're only adding and we return a list
|
Do you have the reported failure test |
| { | ||
| 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)); |
There was a problem hiding this comment.
Is this enough ? Isn't it possible that the children or parents of each node contain references to the nodes we remove ?
So should we also remove pendingUIDsForDeletion from the children and parents of each node ?
Same here. The dependency graph still grows but at a much slower rate. |
| @@ -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()) | |||
There was a problem hiding this comment.
we can also check here is pendingUIDsForDeletion is empty
Purpose
The dependency graph currently accumulates old versios of the nodes (modified and deleted) indefinitely while the Dynamo workspace is running and being edited. This PR aims to clean up old node versions to keep the quadratic complexity of
BuildGraphNodeDependenciescontrolled.Related performance spike: #15864
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
Clean up dependency graph to improve performance and prevent progressive Dynamo slowing down.
Reviewers
@QilongTang
FYIs
(FILL ME IN, Optional) Names of anyone else you wish to be notified of