-
Notifications
You must be signed in to change notification settings - Fork 621
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
collection performance #12974
base: master
Are you sure you want to change the base?
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)) |
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.
👍
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yikes
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'm confused, why did you need to change where ValueAs : Value
to where ValueAs : class
?
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lock is removed because this is a concurrent dictionary?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
*.resx
filesRelease 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