collection performance #12974
collection performance #12974
Conversation
| @@ -2478,8 +2495,7 @@ private void LoadAnnotation(ExtraAnnotationViewInfo annotationViewInfo) | |||
| continue; | |||
|
|
|||
| // NOTE: Some nodes may be annotations and not be found here | |||
| var nodeModel = Nodes.FirstOrDefault(node => node.GUID == guidValue); | |||
| if (nodeModel == null) | |||
| if (!nodeMap.TryGetValue(guidValue, out NodeModel nodeModel)) | |||
| @@ -480,18 +480,14 @@ public double EndDotSize | |||
|
|
|||
| public NodeViewModel Nodevm | |||
| { | |||
| get | |||
| { | |||
| return workspaceViewModel.Nodes?.FirstOrDefault(x => x.NodeLogic.GUID == model.Start.Owner.GUID); | |||
There was a problem hiding this comment.
Curious if all the locking and unlocking for each and every smart collection operation has any performance impact? It looks like in most cases, it may be overkill to make it thread safe so aggressively if that's the case, especially if such cross-threading scenarios will be rare.
The SmartObservableCollection is not meant to be a replacement for all Lists/dictionaries or for highly sensitive areas. SmartObservableCollection is already executing a lot of instructions on every collection operation (ex. property change notifications, collection change notifications). |
| /// Returns true if a value of type ValueAs is found at key in the ConcurrentDictionary | ||
| /// Returns false otherwise. The output value 'valueAs' will be null in this case. | ||
| /// </summary> | ||
| public static bool TryGetValueAs<Key, Value, ValueAs>(this ConcurrentDictionary<Key, Value> dictionary, Key key, out ValueAs valueAs) where ValueAs : class |
There was a problem hiding this comment.
Sorry, I'm confused, why did you need to change where ValueAs : Value to where ValueAs : class?
There was a problem hiding this comment.
the constraint where ValueAs : Value does not support nullable (at least not in the currently used c# language version)
To support nullable I had to switch to the class constraint (to be able to return null and not default). This way we do lose the base type constraint...but it is not so important in my opinion
| @@ -2469,6 +2469,8 @@ private void LoadAnnotation(ExtraAnnotationViewInfo annotationViewInfo) | |||
|
|
|||
| var text = annotationViewInfo.Title; | |||
|
|
|||
| Dictionary<Guid, NodeModel> nodeMap = Nodes.ToDictionary(x => x.GUID); | |||
There was a problem hiding this comment.
Could this be done once before all the Annotations were loaded? Is this just to be sure that new nodes were not loaded between some annotations being loaded?
There was a problem hiding this comment.
so there are some fundamental changes to the core data structures that are used to bind the view models to the views in this PR.
What type of performance improvement did you notice, and can you characterize where those improvements came from?
I think it is also worth discussing why the smart collection type should be thread safe, I guess because the other collections used locks beforehand and we do not want to break any existing callers?
| @@ -186,6 +186,11 @@ internal void OnRequestPortContextMenu(ShowHideFlags flag, PortViewModel viewMod | |||
|
|
|||
| #region Properties and Fields | |||
|
|
|||
| /// <summary> | |||
| /// A Dictionary that maps NodeModels (by GUID) to their corresponding ViewModels | |||
There was a problem hiding this comment.
this comment seems incorrect - below you add ConnectorViewModels to this same map, is this a map of all viewModelTypes?
| annotationViewModel.Dispose(); | ||
| } | ||
| Annotations.Clear(); | ||
| } | ||
|
|
||
| void Model_NodesCleared() | ||
| { | ||
| lock (Nodes) |
There was a problem hiding this comment.
lock is removed because this is a concurrent dictionary?
There was a problem hiding this comment.
lock has been reintroduced as it was
| private void subscribeNodeEvents(NodeViewModel nodeViewModel) | ||
| { | ||
| nodeViewModel.SnapInputEvent += nodeViewModel_SnapInputEvent; | ||
| nodeViewModel.NodeLogic.Modified += OnNodeModified; |
There was a problem hiding this comment.
possibility of nodeLogic being null?
| Errors.Remove(nodeViewModel.ErrorBubble); | ||
| //unsub the events we attached below in NodeAdded. | ||
| unsubscribeNodeEvents(nodeViewModel); | ||
|
|
||
| Nodes.Remove(nodeViewModel); |
There was a problem hiding this comment.
it would be nice if these other collections all stayed in sync automatically with the viewmodelmap or vice versa.
There was a problem hiding this comment.
Collections will now sync somewhat automatically ( except when calling Clear on the collections).
|
Idea for more improvement is to DelayGraphExecution when calling |
see this issue for something related, it might be a slightly different case |
Purpose
Improve performance when opening graphs
https://jira.autodesk.com/browse/DYN-5005
Changes:
Performance improvement when dealing with iterations
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
(FILL ME IN) Brief description of the fix / enhancement. Mandatory section
Reviewers
(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)
(FILL ME IN, optional) Any additional notes to reviewers or testers.
FYIs
(FILL ME IN, Optional) Names of anyone else you wish to be notified of