| CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 658
DYN-8407 Workspace close optimization #16201
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
…se call to avoid reacting to events
…l on null or missing values
| private void DisposePort(PortModel portModel, bool nodeDisposing = false) | ||
| { | ||
| portModel.PropertyChanged -= OnPortPropertyChanged; | ||
| portModel.ConnectorCollectionChanged -= ConnectorsCollectionChanged; | ||
|
|
||
| // if this node is being disposed, we don't need to set node state and destroy the connectors, | ||
| // as the connectors will be deleted elsewhere | ||
| if (!nodeDisposing) | ||
| { | ||
| portModel.DestroyConnectors(); | ||
| SetNodeStateBasedOnConnectionAndDefaults(); | ||
| } | ||
| portModel.Dispose(); | ||
| } |
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.
This would look cleaner if this logic is refactored in the PortModel.Dispose method somehow.
| foreach (NodeModel node in Nodes) | ||
| { | ||
| node.RaisesModificationEvents = false; | ||
| // Dispose here so that all nodes stop listening to disconnect events before | ||
| // the connectors are deleted. Otherwise remaining undisposed nodes will react | ||
| // to delete events when an input connector is deleted. | ||
| node.Dispose(); | ||
| } | ||
|
|
||
| foreach (NodeModel el in Nodes) |
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.
Why can't both these for loops be combined?
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.
We want to dispose the nodes first, otherwise when the connectors are deleted the undisposed nodes will react to events and will take more time. Refer to comment in line 1526.
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.
Understood. This brings me to my next question - shouldn't the nodes be unsubscribed from those events before they are deleted? There could be a memory leak if they are not unsubscribed?
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.
Yes, unsubscribing the nodes before disposing them in the workspaceviewmodel.
reddyashish
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-8407
|
@reddyashish Can you share how much is the optimization gain/reduction in closing time after this change? |
|
@zeusongit Yes added the close times before and after these changes. |
|
Looking into the failing tests. |
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.
Pull Request Overview
This PR addresses workspace closing optimizations by improving disposal logic across related view and model classes to reduce resource leakage and improve load times.
- Updated disposal of nodes, annotations, and ports within WorkspaceViewModel, WorkspaceModel, and NodeModel.
- Added unsubscribing from collection change events in PortModel.
- Wrapped connector removals in try-catch to mitigate deletion exceptions.
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/DynamoCoreWpf/ViewModels/Core/WorkspaceViewModel.cs | Removed node disposal loop in Model_NodesCleared, potentially risking undisposed node view models. |
| src/DynamoCore/Graph/Workspaces/WorkspaceModel.cs | Improved node disposal by deactivating events and ensuring nodes are disposed before clearing. |
| src/DynamoCore/Graph/Nodes/PortModel.cs | Introduced event subscription/unsubscription for connector collection changes. |
| src/DynamoCore/Graph/Nodes/NodeModel.cs | Added a dedicated DisposePort() method to better manage port disposal. |
| src/DynamoCore/Graph/ModelBase.cs | Added a HasBeenDisposed flag to prevent multiple disposals. |
| src/DynamoCore/Graph/Connectors/ConnectorModel.cs | Wrapped connector removals in try-catch and provided logging on exception. |
Files not reviewed (1)
- src/DynamoCore/PublicAPI.Unshipped.txt: Language not supported
Comments suppressed due to low confidence (1)
src/DynamoCoreWpf/ViewModels/Core/WorkspaceViewModel.cs:949
- Removing the loop that disposes each node view model may lead to memory leaks if the nodes are not being disposed elsewhere. Consider ensuring that all node view models are properly disposed when the Nodes collection is cleared.
foreach (var nodeViewModel in Nodes)
|
Now that the whole build log is loaded, I noticed that it was missing another public API declaration. This should fix it. |
| p.Connectors.CollectionChanged += (coll, args) => | ||
| { | ||
| // Call the collection changed handler, replacing | ||
| // the 'sender' with the port, which is required | ||
| // for the disconnect operations. | ||
| ConnectorsCollectionChanged(p, args); | ||
| }; |
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.
What about unsubscribing p.Connectors.CollectionChanged? Since it's attached to an anonymous function, you can't unsubscribe it unless you name the function (lambda).
| InPorts.CollectionChanged -= PortsCollectionChanged; | ||
| foreach(var port in InPorts) | ||
| { | ||
| DisposePort(port, nodeDisposing: true); | ||
| } | ||
|
|
||
| OutPorts.CollectionChanged -= PortsCollectionChanged; | ||
| foreach(var port in OutPorts) | ||
| { | ||
| DisposePort(port, nodeDisposing: true); | ||
| } | ||
| } |
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.
It’s usually recommended to clean up your resources (unsubscribe from events, etc.) before calling base.Dispose().
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.
Is HasBeenDisposed being set to true in the base Dispose() method? If not, you need to set it to true at the end of this function.
| if (HasBeenDisposed) return; | ||
|
|
||
| base.Dispose(); | ||
| Connectors.CollectionChanged -= Connectors_CollectionChanged; |
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.
It’s usually recommended to clean up your resources (unsubscribe from events, etc.) before calling base.Dispose().
|
|
||
| public override void Dispose() | ||
| { | ||
| if (HasBeenDisposed) return; |
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.
Is HasBeenDisposed being set to true in the base Dispose() method? If not, you need to set it to true at the end of this function.
Purpose
Based on the changes here: #15856
After these changes, the workspace close time is reduced but there is still room for optimization.
Profiling results:
Before these changes, time to close
After new changes:
Investigating the WorkspaceViewModel, where Model_NodesCleared() and Model_NotesCleared() are triggered when the nodes are cleared in the workspace model. These 2 calls take 33% of the total close time so looking into it more. After investigation, found that when the objects under the ViewModel are not disposed correctly when a workspace is closed, it will affect the load time for the next workspace.
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
DYN-8407 Optimizing workspace close operations by refactoring the dispose methods.
Reviewers
@DynamoDS/eidos