improve performance of bubbleINfo #15129
Conversation
UI Smoke TestsTest: success. 2 passed, 0 failed. |
|
I'm trying to understand how these changes improve things - are they similar to this? |
Yes. One part of the slowdown was caused by that. Another was that the bubble UI was rendering collapsed components that were binding to properties without data context. |
| // Block Infos updates during the many errors/warnings/notifications added here | ||
| // InfoBubbles will be updated on NodeViewModel's EvaluationCompleted handler. | ||
| using (node.PropertyChangeManager.SetPropsToSuppress(nameof(NodeModel.Infos), nameof(NodeModel.State))) | ||
| using (Disposable.Create(() => { node.BlockInfoBubbleUpdates = true; }, () => { node.BlockInfoBubbleUpdates = false; })) |
There was a problem hiding this comment.
I haven't refreshed my memory on the PropertyChangeManager and how it suppresses property-changed events, but my question is are both this and the blockInfoBubbleUpdates required? How are these different if both ensure the UI updates on the info bubbles happen at the end of the graph execution? In other words, aren't the updates batched for the end of the graph execution already (with the introduction of PropertyChangeManager?
There was a problem hiding this comment.
PropertyChangeManager handles property changes. There are still CollectionChanged events that slip through the cracks. PropertyChangeManager was never adapted to suppress CollectionChanged events (Not sure it can easily)
BlockInfoBubbleUpdates is required for now to block UpdateBubbleContent calls during collection changed events.
There is an event in the NodeViewModel class, NodeModel.Infos.CollectionChanged += () => {UpdateBubbleContent()};
RobertGlobant20
left a comment
There was a problem hiding this comment.
LGTM, did you find a way to measure the performance improvement done by this fix?
| public void UpdateBubbleContent() | ||
| { | ||
| if (DynamoViewModel == null) return; | ||
| if (NodeModel.BlockInfoBubbleUpdates) |
There was a problem hiding this comment.
You can simplify this with:
if (NodeModel.BlockInfoBubbleUpdates == true || DynamoViewModel == null) return;
There was a problem hiding this comment.
I find it easier to read this way.
Manually measured. I could not find any reliable automated way to measure UI responsiveness |
| // Block Infos updates during the many errors/warnings/notifications added here | ||
| // InfoBubbles will be updated on NodeViewModel's EvaluationCompleted handler. | ||
| using (node.PropertyChangeManager.SetPropsToSuppress(nameof(NodeModel.Infos), nameof(NodeModel.State))) | ||
| using (Disposable.Create(() => { node.BlockInfoBubbleUpdates = true; }, () => { node.BlockInfoBubbleUpdates = false; })) |
There was a problem hiding this comment.
Probably could have stopped collection notifications on nodeModel.Infos, but a dedicated flag for updates to bubbleInfo seemed easier to do and understand
| // Block Infos updates during the many errors/warnings/notifications added here | ||
| // InfoBubbles will be updated on NodeViewModel's EvaluationCompleted handler. | ||
| using (node.PropertyChangeManager.SetPropsToSuppress(nameof(NodeModel.Infos), nameof(NodeModel.State))) | ||
| using (Disposable.Create(() => { node.BlockInfoBubbleUpdates = true; }, () => { node.BlockInfoBubbleUpdates = false; })) |
There was a problem hiding this comment.
PropertyChangeManager handles property changes. There are still CollectionChanged events that slip through the cracks. PropertyChangeManager was never adapted to suppress CollectionChanged events (Not sure it can easily)
BlockInfoBubbleUpdates is required for now to block UpdateBubbleContent calls during collection changed events.
There is an event in the NodeViewModel class, NodeModel.Infos.CollectionChanged += () => {UpdateBubbleContent()};
| public void UpdateBubbleContent() | ||
| { | ||
| if (DynamoViewModel == null) return; | ||
| if (NodeModel.BlockInfoBubbleUpdates) |
There was a problem hiding this comment.
I find it easier to read this way.
Well... I was asking because I haven't found any reliable way to measure Dynamo graph loading performance, When the performance improvement is of seconds if I use dotTrace I get similar times with/without the fix :( |
|
https://master-5.jenkins.autodesk.com/job/Dynamo/job/DynamoSelfServe/job/pullRequestValidation/15905 |
DYN-6438 - long time to run Graph when you disconnect a node (i.e when nodes have errors/warnings)
Add “collapsed” visibility for all bubble controls when no data binding exists.
Reduce thread switching and update all node bubbles at the end of graph to xecution
Without fix, Graph Run takes ~ 1 min.
With fix, Graph Run takes ~ 3 seconds.