Skip to content
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.

Sign up for GitHub

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

Reduce Dynamo ViewModel memory leak Part I #9993

Merged
merged 22 commits into from
Oct 22, 2019
Merged

Reduce Dynamo ViewModel memory leak Part I #9993

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
19ce1b0
Initial Commit, example to remove HomeWorkspaceViewModel leak on RunS…
QilongTang Sep 19, 2019
fdbe3fe
Update baseclass for RunSettingsViewModel
QilongTang Sep 25, 2019
7a41ce8
Dispose WorkspaceDependencyView using Dispose call
QilongTang Sep 25, 2019
9d4b0b6
Remove property no longer needed
QilongTang Sep 26, 2019
305159f
Update base class
QilongTang Sep 27, 2019
21b99fa
Merge branch 'master' into DynamoMemoryLeak
QilongTang Oct 8, 2019
dab24bb
Merge branch 'master' into DynamoMemoryLeak
QilongTang Oct 8, 2019
81d2bcc
Address memory leak on notifications property
QilongTang Oct 10, 2019
278b2f5
Some improvement on eventhandlers leak
QilongTang Oct 15, 2019
412bd54
Code Clean up
QilongTang Oct 15, 2019
d82618a
Merge branch 'master' into DynamoMemoryLeak
QilongTang Oct 15, 2019
e13ca60
Code Clean Up
QilongTang Oct 15, 2019
5937131
Use Auto property
QilongTang Oct 15, 2019
b3bd3ab
Code Clean up
QilongTang Oct 15, 2019
c9b1d3a
Update SearchViewModel Dispose function to start from all roots
QilongTang Oct 16, 2019
338291e
Obsolete SearchViewModel initialization on DynamoViewModel
QilongTang Oct 16, 2019
7aca3f3
Dispose InfoBuubleView and workspaceView correctly
QilongTang Oct 16, 2019
b71a0ac
Some Code CleanUp
QilongTang Oct 17, 2019
5f09381
Dispose temp root cates correctly
QilongTang Oct 17, 2019
30134c0
Regressions
QilongTang Oct 17, 2019
259123c
regression none sense :)
QilongTang Oct 18, 2019
4a1bdf4
comments
QilongTang Oct 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/DynamoCoreWpf/ViewModels/Core/HomeWorkspaceViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,9 @@ private bool CanStopPeriodicTimer(object parameter)
return true;
}

/// <summary>
/// Object dispose function
/// </summary>
public override void Dispose()
{
base.Dispose();
Expand All @@ -230,6 +233,7 @@ public override void Dispose()
hwm.EvaluationCompleted -= hwm_EvaluationCompleted;
hwm.SetNodeDeltaState -= hwm_SetNodeDeltaState;
RunSettingsViewModel.PropertyChanged -= RunSettingsViewModel_PropertyChanged;
RunSettingsViewModel.Dispose();
RunSettingsViewModel = null;
DynamoViewModel.Model.ShutdownStarted -= Model_ShutdownStarted;
}
Expand Down
12 changes: 10 additions & 2 deletions src/DynamoCoreWpf/ViewModels/RunSettingsViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ public class RunSettingsViewModel : NotificationObject
#region private members

private bool debug = false;
private readonly HomeWorkspaceViewModel workspaceViewModel;
private HomeWorkspaceViewModel workspaceViewModel;
private readonly DynamoViewModel dynamoViewModel;
private RunTypeItem selectedRunTypeItem;
private SynchronizationContext context;
Expand Down Expand Up @@ -215,7 +215,7 @@ public Visibility RunButtonVisibility

#endregion

#region constructors
#region constructors and dispose function

public RunSettingsViewModel(RunSettings settings, HomeWorkspaceViewModel workspaceViewModel, DynamoViewModel dynamoViewModel)
{
Expand All @@ -236,6 +236,14 @@ public RunSettingsViewModel(RunSettings settings, HomeWorkspaceViewModel workspa
ToggleRunTypeEnabled(RunType.Periodic, false);
}

/// <summary>
/// When switching workspace, this need to be called in HomeworkspaceViewModel dispose function
/// </summary>
internal void Dispose()
{
this.workspaceViewModel = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

#endregion

#region private and internal methods
Expand Down