DYN-6449 Graph Loading Performance Improving #14929

RobertGlobant20 · 2024-02-09T00:38:14Z

Purpose

Improvements for loading a Graph in Dynamo
Using Jetbrains dotTrace I noticed that creating the NodeView for Watch nodes was taking too much time when loading the Graph (due that was creating the PreviewControl), then due that Watch nodes doesn't have Preview I've added code for preventing the creation of the PreviewControl, this improved the Graph loading time. Also I've noticed that the call nodeView.UpdateLayout() was consuming loading time so I deleted the calls to UpdateLayout since seems that is not needed

Declarations

Check these if you believe they are true

The codebase is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
User facing strings, if any, are extracted into *.resx files
All tests pass using the self-service CI.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning and are documented in the API Changes document.
This PR modifies some build requirements and the readme is updated
This PR contains no files larger than 50 MB

Release Notes

Improvements for loading a Graph in Dynamo

Reviewers

@QilongTang
@reddyashish

FYIs

@Amoursol

Using Jetbrains dotTrace I noticed that creating the NodeView for Watch nodes was taking too much time when loading the Graph (due that was creating the PreviewControl), then due that Watch nodes doesn't have Preview I've added code for preventing the creation of the PreviewControl, this improved the Graph loading time. Also I've noticed that the call nodeView.UpdateLayout() was consuming loading time so I've modified the code in a way that the method call will be executed asynchronous in background.

RobertGlobant20 · 2024-02-09T00:42:01Z

This are the time samples that I got before my fix when loading the graph with 1488 nodes ( the time was taking at the moment of clicking the dyn file in the Dynamo StartPage until all the nodes are shown correctly).

226,931 ms(3.7 mins)
195,582 ms(3.2 mins)
248,535 ms(4.14 mins)

RobertGlobant20 · 2024-02-09T00:44:17Z

This are the time samples that I got after my fix when loading the graph with 1488 nodes ( the time was taken at the moment of clicking the dyn file in the Dynamo StartPage until all the nodes are shown correctly).

112,159 ms(1.8 mins)
103,861 ms(1.73 mins)
114,598 ms(1.9 mins)

github-actions · 2024-02-09T00:57:13Z

UI Smoke Tests

Test: success. 2 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests - net8.0

src/DynamoCoreWpf/NodeViewCustomization/NodeViewCustomizations/FunctionNodeViewCustomization.cs

pinzart90 · 2024-02-09T01:42:04Z

src/DynamoCoreWpf/Views/Core/NodeView.xaml.cs

@@ -551,6 +551,9 @@ private void OnNodeViewMouseLeave(object sender, MouseEventArgs e)
        {
            ViewModel.ZIndex = oldZIndex;

+            //Watch nodes doesn't have Preview so we should avoid to use any method/property in PreviewControl class due that Preview is created automatically
+            if (ViewModel.NodeModel != null && ViewModel.NodeModel is CoreNodeModels.Watch) return;
+


another Idea would be to stop loading controls that are not immediately visible to the user (like context menu stuff, tooltips etc) These could be loaded on demand when the user interacts with the UI.

@pinzart90 I think for this case we will need a specific Jira task since I noticed that just the PreviewControl is being used in several places and due to the current implementation of this property, when you use it automatically creates the instance.
For example in this piece of code when using PreviewControl.StaysOpen, the instance is created automatically:

This is the declaration of the Property and the references in code

Yep, a lot of code assumes the existence of the PreviewControl. We should create a task for it if we can say for sure it would benefit performance.
It should either be properly refactored to be optional or create it hidden for certain node types (so that it does not contribute to the layout tree calculations) Not sure about the perf results of either.

Removing the nodeView.UpdateLayout(); call in the PythonNode.

RobertGlobant20 · 2024-02-12T21:45:17Z

I've done some testing using the dyn files located in Dynamo\test\core\python but I didn't see any difference by removing the nodeView.UpdateLayout() call, then as @pinzart90 suggested we will need to do a focused test in python nodes.
FYI: @avidit

pinzart90 · 2024-02-13T03:05:04Z

src/DynamoCoreWpf/NodeViewCustomization/NodeViewCustomizations/FunctionNodeViewCustomization.cs

@@ -59,7 +61,6 @@ public void CustomizeView(Function function, NodeView nodeView)

            publishCustomNodeItem.Command = nodeView.ViewModel.DynamoViewModel.PublishSelectedNodesCommand;
            publishCustomNodeItem.CommandParameter = functionNodeModel;
-
            nodeView.UpdateLayout();


is this updateLayout call useful ?
Looks like the customization only creates some context menu items. THey do not appear to be updated when the customization is applied.

@pinzart90 like the other case I don't know the purpose of calling UpdateLayout(); I think is the same author than the case of the Python node.
I leave it because in the dotTrace performance report showed that was not consuming too much time when loading the graph for this specific line (due that the graph provided had few custom nodes) but probably we should deleted also.
Do you suggest this call also could be deleted?

it looks like it could be deleted. But maybe we should leave it for another task

I've also deleted this call, seems that was created by the same person that the Python node one.
a548050

Deleting not necessary call to nodeView.UpdateLayout() for CustomNodes

QilongTang · 2024-02-14T03:11:02Z

src/DynamoCoreWpf/Views/Core/NodeView.xaml.cs

@@ -551,6 +551,9 @@ private void OnNodeViewMouseLeave(object sender, MouseEventArgs e)
        {
            ViewModel.ZIndex = oldZIndex;

+            //Watch nodes doesn't have Preview so we should avoid to use any method/property in PreviewControl class due that Preview is created automatically
+            if (ViewModel.NodeModel != null && ViewModel.NodeModel is CoreNodeModels.Watch) return;


Does this only apply to watch node?

Well, When running dotTrace I noticed that we were creating the PreviewControl for Watch node when this kind of nodes doesn't have Preview.
Probably we will need to get a list of nodes that doesn't support preview or do you know if there is already a list?

QilongTang · 2024-02-14T03:12:07Z

src/DynamoCoreWpf/NodeViewCustomization/NodeViewCustomizations/FunctionNodeViewCustomization.cs

@@ -59,8 +61,6 @@ public void CustomizeView(Function function, NodeView nodeView)

            publishCustomNodeItem.Command = nodeView.ViewModel.DynamoViewModel.PublishSelectedNodesCommand;
            publishCustomNodeItem.CommandParameter = functionNodeModel;
-
-            nodeView.UpdateLayout();


Was this moved to other places based on your PR description?

Updated the PR description

QilongTang · 2024-02-14T19:21:08Z

@avidit I feel we need to test the cases where input ports were added/removed based on Python script change or custom drop downs etc

RobertGlobant20 requested review from QilongTang and reddyashish February 9, 2024 00:38

pinzart90 reviewed Feb 9, 2024

View reviewed changes

src/DynamoCoreWpf/NodeViewCustomization/NodeViewCustomizations/FunctionNodeViewCustomization.cs Outdated Show resolved Hide resolved

pinzart90 reviewed Feb 9, 2024

View reviewed changes

QilongTang added this to the 3.1 milestone Feb 9, 2024

DYN-6449 Graph Loading Performance Improving Code Review

2091c35

Removing the nodeView.UpdateLayout(); call in the PythonNode.

pinzart90 reviewed Feb 13, 2024

View reviewed changes

DYN-6449 Graph Loading Performance Improving Code Review

a548050

Deleting not necessary call to nodeView.UpdateLayout() for CustomNodes

QilongTang reviewed Feb 14, 2024

View reviewed changes

QilongTang approved these changes Feb 14, 2024

View reviewed changes

QilongTang merged commit a4d26ea into DynamoDS:master Feb 14, 2024
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DYN-6449 Graph Loading Performance Improving #14929

DYN-6449 Graph Loading Performance Improving #14929

RobertGlobant20 commented Feb 9, 2024 •

edited

Loading

RobertGlobant20 commented Feb 9, 2024

RobertGlobant20 commented Feb 9, 2024

github-actions bot commented Feb 9, 2024 •

edited

Loading

pinzart90 Feb 9, 2024

RobertGlobant20 Feb 12, 2024

pinzart90 Feb 13, 2024

RobertGlobant20 commented Feb 12, 2024

pinzart90 Feb 13, 2024

RobertGlobant20 Feb 13, 2024

pinzart90 Feb 13, 2024

RobertGlobant20 Feb 13, 2024

QilongTang Feb 14, 2024

RobertGlobant20 Feb 14, 2024

QilongTang Feb 14, 2024

RobertGlobant20 Feb 14, 2024

QilongTang commented Feb 14, 2024

DYN-6449 Graph Loading Performance Improving #14929

DYN-6449 Graph Loading Performance Improving #14929

Conversation

RobertGlobant20 commented Feb 9, 2024 • edited Loading

Purpose

Declarations

Release Notes

Reviewers

FYIs

RobertGlobant20 commented Feb 9, 2024

RobertGlobant20 commented Feb 9, 2024

github-actions bot commented Feb 9, 2024 • edited Loading

UI Smoke Tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobertGlobant20 commented Feb 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QilongTang commented Feb 14, 2024

RobertGlobant20 commented Feb 9, 2024 •

edited

Loading

github-actions bot commented Feb 9, 2024 •

edited

Loading