Reduce Dynamo ViewModel memory leak Part I #9993

QilongTang · 2019-09-20T01:53:15Z

Please Note:

Before submitting the PR, please review How to Contribute to Dynamo
Dynamo Team will meet 1x a month to review PRs found on Github (Issues will be handled separately)
PRs will be reviewed from oldest to newest
If a reviewed PR requires changes by the owner, the owner of the PR has 30 days to respond. If the PR has seen no activity by the next session, it will be either closed by the team or depending on its utility will be taken over by someone on the team
PRs should use either Dynamo's default PR template or one of these other template options in order to be considered for review.
PRs that do not have one of the Dynamo PR templates completely filled out with all declarations satisfied will not be reviewed by the Dynamo team.
PRs made to the DynamoRevit repo will need to be cherry-picked into all the DynamoRevit Release branches that Dynamo supports. Contributors will be responsible for cherry-picking their reviewed commits to the other branches after a LGTM label is added to the PR.

Purpose

This PR is an example of things need to be done to dispose object in correct order to avoid memory leak.

PortViewModels are disposed correctly now
RunSettingsViewModel is disposed correctly now
InfoBubbleViewModels are disposed correctly now
HomeworkspaceViewModel are disposed correctly but still retained because of workspaceView leak on resource dictionary
WPF binding leak addressed. Notifications collection on notificationExtension are disposed correctly now
NodeCategoryViewModels are now disposed correctly
NodeSearchElementViewModels are now disposed correctly
DynamoViewModel no longer keeps a valid duplicate SearchViewModel now

The following is a comparison using our sample Geometry - Solids as file open candidate.

Baseline -> 2.4.0
Memory Consumption Difference between Dynamo StartUp-> File Open Once -> File Open 10x

File Open Once

File Open 10x

After -> Release build of this branch (2.5.0)
Memory Consumption Difference between Dynamo StartUp-> File Open Once -> File Open 10x

File Open Once

File Open 10x

Declarations

Check these if you believe they are true

The code base 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.

Reviewers

@DynamoDS/dynamo

FYIs

…ettingsViewModel

mjkkirschner · 2019-09-20T02:07:30Z

src/DynamoCoreWpf/ViewModels/RunSettingsViewModel.cs

+        /// </summary>
+        internal void Dispose()
+        {
+            this.workspaceViewModel = null;


I think the cases where this needs to be done in c# are small in number, why do we need to do it here?

I believe something here is preventing HomeworkspaceViewModel to be disposed, not sure if it is related to the readonly field or simply calling dispose, will give it another try tomorrow

From the retention paths you captured screenshots of it looks like the DynamoView is indirectly holding onto an instance of RunSettingsViewModel that is in turn holding onto the HomeWorkspaceViewModel. I think we should try and dispose off the HomeWorkspaceViewModel from DynamoView in that case, no?

I believe DynamoView is a singleton as well as the RunSettingsControl. So we won't be able to know when to dispose specifically from the view, but we can do that when closing the currentWorkspace. WorkspaceRemoved event will be triggered and then Dispose() on HomeWorkspaceModel will be called, that's why I added the logic there.

QilongTang · 2019-09-26T18:20:22Z

Running Self CI, I think this PR is good for review. It does not address all the issues, but is a good step.

mjkkirschner · 2019-09-27T04:17:13Z

src/DynamoCoreWpf/ViewModels/RunSettingsViewModel.cs

@@ -81,12 +81,12 @@ public RunTypeItem(RunType runType)
    /// notifications as those notifications are raised when the value is set on the
    /// model.
    /// </summary>
-    public class RunSettingsViewModel : NotificationObject
+    public class RunSettingsViewModel : ViewModelBase


this is an API break.

or - could be, we need to be very careful changing the base class of a public class.

How do we find out? ViewModelBase implements NotificationObject and IDisposal so I believe doing this will not remove any public property, that's actually why I thought it is not API breaking. Same reason below.

mjkkirschner · 2019-09-27T04:18:53Z

src/DynamoCoreWpf/ViewModels/Search/BrowserItemViewModel.cs

@@ -89,7 +89,7 @@ public interface ISearchEntryViewModel : INotifyPropertyChanged, IDisposable
        ElementTypes ElementType { get; }
    }

-    public class NodeCategoryViewModel : NotificationObject, ISearchEntryViewModel
+    public class NodeCategoryViewModel : ViewModelBase, ISearchEntryViewModel 


same here. can this be accomplished without changing the base class by just adding more interfaces?

you can check this doc:
https://stackoverflow.com/questions/1456785/a-definitive-guide-to-api-breaking-changes-in-net

and do some thought experiments or real experiments with extensions using that class before and after.

public abstract class ViewModelBase : NotificationObject,IDisposable I dont think switching out the baseclass here break API backward compatibility because it only adds two APIs

IsDebugBuild

and

Dispose

QilongTang · 2019-10-08T21:12:32Z

Updated screenshots and could be more useful for our discussion

mjkkirschner · 2019-10-14T15:46:02Z

src/Notifications/NotificationsViewExtension.cs

-        public ObservableCollection<Logging.NotificationMessage> Notifications { get; private set; }
+
+        private ObservableCollection<Logging.NotificationMessage> notifications;
+        public ObservableCollection<Logging.NotificationMessage> Notifications


so this is kind of argg - we need to implement INotifyPropertyChanged to have .net gc this collection?

Can we add a comment saying as much so that in a year we don't delete it all when we see it is unused 😄

Only because the collection is binding to WPF UI, I can add comments

QilongTang · 2019-10-18T06:19:08Z

All Unit tests passing, Ready for review

src/DynamoCoreWpf/ViewModels/Core/WorkspaceViewModel.cs

src/DynamoCoreWpf/Views/Preview/InfoBubbleView.xaml.cs

mjkkirschner · 2019-10-21T21:19:33Z

LGTM

Initial Commit, example to remove HomeWorkspaceViewModel leak on RunS…

19ce1b0

…ettingsViewModel

mjkkirschner reviewed Sep 20, 2019

View reviewed changes

QilongTang added 3 commits September 25, 2019 14:20

Update baseclass for RunSettingsViewModel

fdbe3fe

Dispose WorkspaceDependencyView using Dispose call

7a41ce8

Remove property no longer needed

9d4b0b6

QilongTang mentioned this pull request Sep 26, 2019

fix exception being thrown during shutdown of workspace dep viewer #10007

Closed

7 tasks

Update base class

305159f

QilongTang changed the title ~~Prevent Dynamo Workspace close memory leak~~ Prevent Dynamo Workspace close memory leak Part I Sep 27, 2019

mjkkirschner reviewed Sep 27, 2019

View reviewed changes

QilongTang added 2 commits October 8, 2019 14:21

Merge branch 'master' into DynamoMemoryLeak

21b99fa

Merge branch 'master' into DynamoMemoryLeak

dab24bb

Address memory leak on notifications property

81d2bcc

mjkkirschner reviewed Oct 14, 2019

View reviewed changes

QilongTang added 13 commits October 14, 2019 20:17

Some improvement on eventhandlers leak

278b2f5

Code Clean up

412bd54

Merge branch 'master' into DynamoMemoryLeak

d82618a

Code Clean Up

e13ca60

Use Auto property

5937131

Code Clean up

b3bd3ab

Update SearchViewModel Dispose function to start from all roots

c9b1d3a

Obsolete SearchViewModel initialization on DynamoViewModel

338291e

Dispose InfoBuubleView and workspaceView correctly

7aca3f3

Some Code CleanUp

b71a0ac

Dispose temp root cates correctly

5f09381

Regressions

30134c0

regression none sense :)

259123c

QilongTang changed the title ~~Prevent Dynamo Workspace close memory leak Part I~~ Reduce Dynamo ViewModel memory leak Part I Oct 21, 2019

mjkkirschner reviewed Oct 21, 2019

View reviewed changes

src/DynamoCoreWpf/ViewModels/Core/WorkspaceViewModel.cs Show resolved Hide resolved

mjkkirschner reviewed Oct 21, 2019

View reviewed changes

src/DynamoCoreWpf/Views/Preview/InfoBubbleView.xaml.cs Outdated Show resolved Hide resolved

mjkkirschner added the LGTM Looks good to me label Oct 22, 2019

comments

4a1bdf4

QilongTang merged commit 1ad9596 into master Oct 22, 2019

QilongTang deleted the DynamoMemoryLeak branch October 22, 2019 17:29

QilongTang mentioned this pull request Feb 12, 2020

RC2.0.4 Cherry-pick #10385

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce Dynamo ViewModel memory leak Part I #9993

Reduce Dynamo ViewModel memory leak Part I #9993

QilongTang commented Sep 20, 2019 •

edited

Loading

mjkkirschner Sep 20, 2019

QilongTang Sep 20, 2019

aparajit-pratap Sep 24, 2019

QilongTang Sep 26, 2019

QilongTang commented Sep 26, 2019

mjkkirschner Sep 27, 2019

mjkkirschner Sep 27, 2019

QilongTang Sep 27, 2019

mjkkirschner Sep 27, 2019

mjkkirschner Sep 27, 2019

QilongTang Oct 8, 2019

QilongTang commented Oct 8, 2019

mjkkirschner Oct 14, 2019

QilongTang Oct 14, 2019

QilongTang commented Oct 18, 2019

mjkkirschner commented Oct 21, 2019

Reduce Dynamo ViewModel memory leak Part I #9993

Reduce Dynamo ViewModel memory leak Part I #9993

Conversation

QilongTang commented Sep 20, 2019 • edited Loading

Purpose

Declarations

Reviewers

FYIs

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 Sep 26, 2019

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 Oct 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QilongTang commented Oct 18, 2019

mjkkirschner commented Oct 21, 2019

QilongTang commented Sep 20, 2019 •

edited

Loading