NodeModel dispose on workspace close#15856
Closed
bendikberg wants to merge 5 commits intoDynamoDS:masterfrom
Closed
NodeModel dispose on workspace close#15856bendikberg wants to merge 5 commits intoDynamoDS:masterfrom
bendikberg wants to merge 5 commits intoDynamoDS:masterfrom
Conversation
…se call to avoid reacting to events
…l on null or missing values
9 tasks
Contributor
There was a problem hiding this comment.
Pull Request Overview
Improves workspace close performance by disposing nodes and unsubscribing connector events before connectors are deleted.
- Combines modification-event disabling and node disposal in
WorkspaceModel.Clear() - Introduces
ConnectorCollectionChangedevent and disposal logic inPortModelandNodeModel - Adds
Disposeoverrides inModelBase,NodeModel, andPortModelto clean up event subscriptions
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/DynamoCore/Graph/Workspaces/WorkspaceModel.cs | Moved node disposal into initial loop in Clear |
| src/DynamoCore/Graph/Nodes/PortModel.cs | Added connector‐collection event and Dispose |
| src/DynamoCore/Graph/Nodes/NodeModel.cs | Centralized port cleanup in DisposePort and Dispose override |
| src/DynamoCore/Graph/ModelBase.cs | Tracked disposal state with HasBeenDisposed |
| src/DynamoCore/Graph/Connectors/ConnectorModel.cs | Simplified null‐safe connector removal |
Files not reviewed (1)
- src/DynamoCore/PublicAPI.Unshipped.txt: Language not supported
Comments suppressed due to low confidence (2)
src/DynamoCore/Graph/Workspaces/WorkspaceModel.cs:1489
- After disposing each node, consider clearing the
Nodescollection (e.g.,Nodes.Clear()) to release references and prevent potential memory leaks.
foreach (NodeModel node in Nodes)
src/DynamoCore/Graph/Nodes/NodeModel.cs:1340
- [nitpick] The event name
ConnectorCollectionChanged(singular) and handlerConnectorsCollectionChanged(plural) are inconsistent. Consider renaming to one form (e.g.,ConnectorsCollectionChanged) for clarity.
p.ConnectorCollectionChanged += ConnectorsCollectionChanged;
| @@ -347,6 +348,8 @@ internal PortModel(string name, string toolTip) | |||
| Height = Math.Abs(Height) < 0.001 ? Configurations.PortHeightInPixels : Height; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
[nitpick] Add an XML doc comment for ConnectorCollectionChanged to explain when this event is raised and what subscribers should expect.
Suggested change
| /// <summary> | |
| /// Occurs when the collection of connectors associated with this port changes. | |
| /// </summary> | |
| /// <remarks> | |
| /// This event is triggered whenever a connector is added to or removed from the port's connector collection. | |
| /// Subscribers can use the event arguments to determine the nature of the change. | |
| /// </remarks> |
| } | ||
|
|
||
|
|
||
| public override void Dispose() |
There was a problem hiding this comment.
In the Dispose override, invoke base.Dispose() after unsubscribing from InPorts/OutPorts and disposing ports to ensure cleanup completes before raising the Disposed event.
Collaborator
|
Closing this as the changes are merged in a different PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
When a workspace is closed, a majority of the time is spent by nodes reacting to connector deletions. By unsubscribing from connector events before the a workspace closes, the time to get back to the main menu goes down substantially.
For the MaRS graph, this takes workspace close from about 8 to 4 seconds. For the Car_Configurator graph, this takes workspace close from about 2 minutes down to 40 seconds.
Performance
Old
New
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
N/A
Reviewers
@mjkkirschner
FYIs
@dimven