viewModel memory leaks #8366

mjkkirschner · 2017-11-30T04:19:24Z

Purpose

HomeWorkspaceViewModel was never being garbage collected - there were atleast 8 retention paths and in some cases 11. I found that almost all ViewModels were leaking and never being garbage collected.

I added Dispose methods to the viewModels which are called when the models are removed from their collections - for example, the NodeViewModel is disposed when its corresponding model is removed from the Nodes Collection on the WorkspaceModel.

These dispose methods cleanup event subscriptions that the viewModels had retaining them. These events point towards models, other viewModels, static instances etc...

I will provide a pre and post profile image.

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.

pictures!

Please note that these runs show the difference between opening the graph Geometry_Surfaces.dyn 10 times in a row from the recent files menu. They are a little unfair - since on the master brach, my machine ran out of graphics memory after 6 consecutive opens on the master branch. ⚠️ 😢 (machine is a VM with 512 mb of vram). This did not occur on this branch.

Master

This Branch

Master

This Branch

Reviewers

@ColinDayOrg
@alfarok

FYIs

call them when models are removed or cleared

mjkkirschner · 2017-11-30T04:24:38Z

src/DynamoCoreWpf/ViewModels/Core/DynamoViewModel.cs

            if (currentWorkspaceViewModel == viewModel)
+                if(currentWorkspaceViewModel != null)
+                {
+                    currentWorkspaceViewModel.Dispose();


main entry point for disposal of viewModels - if we remove a workspace from the workspace collection - dispose it, this causes dispose to get called on nodeViewModel, NoteViewModel, PortViewModel, ConnectorViewModel, etc.

ramramps · 2017-11-30T11:56:00Z

src/DynamoCoreWpf/ViewModels/Core/NoteViewModel.cs

+        public virtual void Dispose()
+        {
+            _model.PropertyChanged -= note_PropertyChanged;
+            DynamoSelection.Instance.Selection.CollectionChanged -= SelectionOnCollectionChanged;


should there be dispose on viewmodelbase covering few dispose (like selection)?

I will give that a shot - I think the base implementation may be empty (nothing shared between all viewModels), but thats okay, it will be more clear.

Possibly the base implementation should be abstract to force all derived classes to properly dispose their contents?

still the base class can handle few common things?

jnealb · 2017-11-30T14:56:58Z

@mjkkirschner @alfarok @ramramps @ColinDayOrg @smangarole What is the likely-hood of introducing bugs here? What should QA be on the lookout for? Custom Node workflows?

alfarok · 2017-11-30T15:01:27Z

src/DynamoCoreWpf/ViewModels/Core/HomeWorkspaceViewModel.cs

+            RunSettingsViewModel.PropertyChanged -= RunSettingsViewModel_PropertyChanged;
+            RunSettingsViewModel = null;
            DynamoViewModel.Model.ShutdownStarted -= Model_ShutdownStarted;
+            //RunSettingsViewModel.Dispose();


@mjkkirschner is this required or can it be removed?

will remove.

mjkkirschner · 2017-11-30T16:32:27Z

@jnealb I would be most concerned about situations where viewModels are being disposed but are still being used by a view that is visible to the user - this should not happen but there could be edge cases - in that situation we would see the view no longer responding to events on the model.

remove some comments

mjkkirschner · 2017-11-30T20:53:45Z

finding some more retention paths for HomeWorkspaceViewModel when workspace has watch3d nodes, investigating.

ColinDayOrg · 2017-11-30T21:07:17Z

src/DynamoCoreWpf/ViewModels/Core/DynamoViewModel.cs


        private void CloseHomeWorkspace(object parameter)
        {
+            //TODO potentially dispose viewModel here.


Lately I have been adding "// TODO, QNTM-XXXX:" instead of just "// TODO" so that there is a connection between the todo and why it was created. It also makes it searchable by number. I'm not sure this is really necessary (or a good thing, though it seems good to me, opinions requested).

ColinDayOrg · 2017-11-30T21:07:41Z

src/DynamoCoreWpf/ViewModels/Core/NodeViewModel.cs


+        public virtual void Dispose()
+        {
+            //TODO add other event removal here.


Same TODO comment here.

ColinDayOrg · 2017-11-30T21:10:11Z

src/DynamoCoreWpf/ViewModels/Core/NodeViewModel.cs

+
+            DynamoViewModel.Model.PropertyChanged -= Model_PropertyChanged;
+            DynamoViewModel.Model.DebugSettings.PropertyChanged -= DebugSettings_PropertyChanged;
+            if (IsDebugBuild)


General question: Is the IsDebugBuild check necessary here? If an event is -= and the event had not been previously subscribed would this cause an issue? If not, it seems like it may possibly be safer just to unsubscribe it in case there is somewhere it is subscribed in the future outside of a IsDebugBuild check. No action necessary here, just asking a question.

No, there should be no harm - unfortunately this call doesn't work in all cases - the engine controller gets recreated at some point when a new workspace is opened - sometimes - it is before this event is unsubscribed - that is bad.

I need to investigate this as well - thankfully this should not effect release builds.

ColinDayOrg · 2017-11-30T21:13:27Z

src/DynamoCoreWpf/ViewModels/Core/WorkspaceViewModel.cs

            Model.ConnectorDeleted += Connectors_ConnectorDeleted;
            Model.PropertyChanged += ModelPropertyChanged;
+
+            this.refreshViewHandler = (sender, e) => { RefreshViewOnSelectionChange(); };


Maybe this should be a method instead of a lambda expression for clarity/continuity?

ColinDayOrg · 2017-11-30T21:14:46Z

src/DynamoCoreWpf/ViewModels/Core/WorkspaceViewModel.cs

+            }
            _nodes.Clear();
            Errors.Clear();
+            //TODO dispose the cleared errors?


Same TODO comment as above

ColinDayOrg · 2017-11-30T21:17:56Z

Generally LGTM, with some comments. I'm not sure here is right place to address it, or even if there is a good answer, but I find it troubling that there is a pattern of subscribing to events in one place and unsubscribing from those events in another place, with no real connection between what was subscribed and what was unsubscribed. It seems like some kind of more comprehensive solution could be used like "UnsubscribeAllEvents" or something like that.

watch3d viewmodel was not unsubscribing from methods on dynamo model - all watch3d view models leaked

…kspaces they are in.

mjkkirschner · 2017-12-03T17:59:12Z

src/DynamoCoreWpf/ViewModels/Watch3D/HelixWatch3DViewModel.cs

                    if (model3D != null)
                    {
                        model3D.Detach();
+                        model3D.Dispose();


when we remove geometry from the background preview or watch3d node - actually dispose the tessellated objects, don't just detach them.

mjkkirschner · 2017-12-03T17:59:53Z

src/Libraries/Watch3DNodeModelsWpf/HelixWatch3DNodeViewModel.cs

+        protected override void Dispose(bool disposing)
+        {
+            base.Dispose(true);
+            UnregisterNodeEventHandlers(this.watchNode);


this is debugging code - will clean up.

mjkkirschner · 2017-12-03T18:00:16Z

src/DynamoCoreWpf/ViewModels/Watch3D/DefaultWatch3DViewModel.cs

        }

-        private void UnregisterEventHandlers()
+        protected void UnregisterEventHandlers()


these changes are debugging code - will clean up.

get rid of dispose call that does nothing

# Conflicts: # src/Libraries/Watch3DNodeModelsWpf/HelixWatch3DNodeViewModel.cs # src/Libraries/Watch3DNodeModelsWpf/Watch3DNodeViewCustomization.cs

mjkkirschner · 2017-12-05T02:54:38Z

@ColinDayOrg ready for a look, please see images in description.

ColinDayOrg · 2017-12-05T17:54:52Z

src/DynamoCoreWpf/Controls/IncanvasSearchControl.xaml.cs

            if (Application.Current != null)
            {
-                Application.Current.Deactivated += (s, args) => { OnRequestShowInCanvasSearch(ShowHideFlags.Hide); };
+                Application.Current.Deactivated += currentApplicationDeactivated;


Not really a code review comment, but I think this type of change should be in the coding standards (i.e. no attaching lambda functions to event handlers, unless there is a specific reason).

will add this to style guide.

ColinDayOrg · 2017-12-05T18:16:56Z

Generally, I think that these changes will make the code base in a better state, so LGTM. That being said, I think that calling Dispose directly may have possible side effects, such as other objects having references to the objects that were disposed. (I am currently running into exactly this while working on a different issue). Possibly we may want to look into using SafeHandle (i.e. C# auto_ptr style handle).

https://docs.microsoft.com/en-us/dotnet/standard/garbage-collection/implementing-dispose

Also, I'm not exactly sure what the images attached above are supposed to be showing me. I can see that they are different, but the memory used does not seem to be that different, the second image still has a ramp.

mjkkirschner · 2017-12-05T20:54:19Z

We're kind of overloading the use of dispose here - most of the changes are are not related to unmanaged memory - do calling dispose does not actually destroy the object - it just removes references so that the object can be garbage collected.

I do think that a better strategy would be good longterm or a guide for devs new to this repo.

Theres definitely still leaks after this PR so there are ramp ups but the total memory saved is approximately 50 megs (not including unknown gpu memory saved) also it's not exactly a fair comparison as the memory pressure on my VM was less in the other test and so garbage collection was probably not occurring as frequently.

mjkkirschner · 2017-12-07T03:26:52Z

so after this last commit (shot in the dark) the self serve - CI passes. - I will run it again though and see what happens.

mjkkirschner added 2 commits November 27, 2017 16:18

add dispose methods to viewModels

0dd68b4

call them when models are removed or cleared

hwsvm disposing correctly

d2f898e

mjkkirschner commented Nov 30, 2017

View reviewed changes

ramramps reviewed Nov 30, 2017

View reviewed changes

alfarok reviewed Nov 30, 2017

View reviewed changes

only base class implements idisposable

24ab559

remove some comments

mjkkirschner added the WIP label Nov 30, 2017

ColinDayOrg reviewed Nov 30, 2017

View reviewed changes

mjkkirschner added 2 commits November 30, 2017 19:54

when disposing nvms - make sure to call unsubscribe methods

5e77042

watch3d viewmodel was not unsubscribing from methods on dynamo model - all watch3d view models leaked

working but messy - this cleans up watch3d nodeviewModels and the wor…

66009d2

…kspaces they are in.

mjkkirschner added the DNM Do not merge. For PRs. label Dec 2, 2017

mjkkirschner commented Dec 3, 2017

View reviewed changes

mjkkirschner added 3 commits December 4, 2017 21:00

make things private again

6e11d37

get rid of dispose call that does nothing

review comments

ae2b13e

Merge branch 'upmaster' into hwmMemleak

dd135c4

# Conflicts: # src/Libraries/Watch3DNodeModelsWpf/HelixWatch3DNodeViewModel.cs # src/Libraries/Watch3DNodeModelsWpf/Watch3DNodeViewCustomization.cs

mjkkirschner added PTAL Please Take A Look 👀 and removed DNM Do not merge. For PRs. WIP labels Dec 5, 2017

fix build.

0a0d999

ColinDayOrg reviewed Dec 5, 2017

View reviewed changes

shot in the dark

52ae4fb

get rid of extra refresh call

b2abed5

mjkkirschner merged commit dae4505 into DynamoDS:master Dec 7, 2017

mjkkirschner changed the title ~~[WIP] viewModel memory leaks~~ viewModel memory leaks Dec 7, 2017

viewModel memory leaks #8366

viewModel memory leaks #8366

Uh oh!

Conversation

mjkkirschner commented Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Declarations

pictures!

Master

This Branch

Master

This Branch

Reviewers

FYIs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnealb commented Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alfarok Nov 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner commented Nov 30, 2017

Uh oh!

mjkkirschner commented Nov 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ColinDayOrg commented Nov 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner commented Dec 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ColinDayOrg commented Dec 5, 2017

Uh oh!

mjkkirschner commented Dec 5, 2017

Uh oh!

mjkkirschner commented Dec 7, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

mjkkirschner commented Nov 30, 2017 •

edited

Loading

mjkkirschner Nov 30, 2017 •

edited

Loading

jnealb commented Nov 30, 2017 •

edited

Loading

alfarok Nov 30, 2017 •

edited

Loading