| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 658
DYN-8358 cleanup dependency graph from old nodes #15869
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
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.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? |
|
Do you have the reported failure test |
Same here. The dependency graph still grows but at a much slower rate. |
|
@sm6srw We see EngOps Build check failing here, expected? |
|
@chubakueno Looking at this again, my wild guess of the PR check failure is that this fork is not up-to-date with master. I decide to trust that you had all the checks passing earlier this week. I am leaning towards that we merge and test the mock cluster placement -> undo repeated to validate it, as you already did locally. @BogdanZavu What do you think? |
Sounds good to me. Nice work! |
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