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 all commits
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
5 changes: 3 additions & 2 deletions src/DynamoCoreWpf/ViewModels/Core/DynamoViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,7 @@ public bool ShowDebugASTs

public IWatchHandler WatchHandler { get; private set; }

[Obsolete("This Property will be obsoleted in Dynamo 3.0.")]
public SearchViewModel SearchViewModel { get; private set; }

public PackageManagerClientViewModel PackageManagerClientViewModel { get; private set; }
Expand Down Expand Up @@ -535,7 +536,7 @@ protected DynamoViewModel(StartConfiguration startConfiguration)
this.WatchHandler = startConfiguration.WatchHandler;
var pmExtension = model.GetPackageManagerExtension();
this.PackageManagerClientViewModel = new PackageManagerClientViewModel(this, pmExtension.PackageManagerClient);
this.SearchViewModel = new SearchViewModel(this);
this.SearchViewModel = null;

// Start page should not show up during test mode.
this.ShowStartPage = !DynamoModel.IsTestMode;
Expand Down Expand Up @@ -2290,7 +2291,7 @@ public void ImportLibrary(object parameter)
string.Format(detailedMessage, file));
}
}
SearchViewModel.SearchAndUpdateResults();
CurrentSpaceViewModel.InCanvasSearchViewModel.SearchAndUpdateResults();
}
catch(LibraryLoadFailedException ex)
{
Expand Down
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
16 changes: 15 additions & 1 deletion src/DynamoCoreWpf/ViewModels/Core/NodeViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -590,7 +590,10 @@ public NodeViewModel(WorkspaceViewModel workspaceViewModel, NodeModel logic)
++NoteViewModel.StaticZIndex;
}

public virtual void Dispose()
/// <summary>
/// Dispose function
/// </summary>
public override void Dispose()
{
this.NodeModel.PropertyChanged -= logic_PropertyChanged;

Expand All @@ -601,7 +604,18 @@ public virtual void Dispose()
DynamoViewModel.EngineController.AstBuilt -= EngineController_AstBuilt;
}

foreach (var p in InPorts)
{
p.Dispose();
}

foreach (var p in OutPorts)
{
p.Dispose();
}
ErrorBubble.Dispose();
DynamoSelection.Instance.Selection.CollectionChanged -= SelectionOnCollectionChanged;
base.Dispose();
}

public NodeViewModel(WorkspaceViewModel workspaceViewModel, NodeModel logic, Size preferredSize)
Expand Down
72 changes: 31 additions & 41 deletions src/DynamoCoreWpf/ViewModels/Core/WorkspaceViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,29 +251,19 @@ public bool IsCursorForced
set { isCursorForced = value; RaisePropertyChanged("IsCursorForced"); }
}

private CompositeCollection _workspaceElements = new CompositeCollection();
[JsonIgnore]
public CompositeCollection WorkspaceElements { get { return _workspaceElements; } }

ObservableCollection<ConnectorViewModel> _connectors = new ObservableCollection<ConnectorViewModel>();
public CompositeCollection WorkspaceElements { get; } = new CompositeCollection();
QilongTang marked this conversation as resolved.
Show resolved Hide resolved
[JsonIgnore]
public ObservableCollection<ConnectorViewModel> Connectors { get { return _connectors; } }
public ObservableCollection<ConnectorViewModel> Connectors { get; } = new ObservableCollection<ConnectorViewModel>();

ObservableCollection<NodeViewModel> _nodes = new ObservableCollection<NodeViewModel>();
[JsonProperty("NodeViews")]
public ObservableCollection<NodeViewModel> Nodes { get { return _nodes; } }

public ObservableCollection<NodeViewModel> Nodes { get; } = new ObservableCollection<NodeViewModel>();
// Do not serialize notes, they will be converted to annotations during serialization
ObservableCollection<NoteViewModel> _notes = new ObservableCollection<NoteViewModel>();
[JsonIgnore]
public ObservableCollection<NoteViewModel> Notes { get { return _notes; } }

ObservableCollection<InfoBubbleViewModel> _errors = new ObservableCollection<InfoBubbleViewModel>();
public ObservableCollection<NoteViewModel> Notes { get; } = new ObservableCollection<NoteViewModel>();
[JsonIgnore]
public ObservableCollection<InfoBubbleViewModel> Errors { get { return _errors; } }

ObservableCollection<AnnotationViewModel> _annotations = new ObservableCollection<AnnotationViewModel>();
public ObservableCollection<AnnotationViewModel> Annotations { get { return _annotations; } }
public ObservableCollection<InfoBubbleViewModel> Errors { get; } = new ObservableCollection<InfoBubbleViewModel>();
public ObservableCollection<AnnotationViewModel> Annotations { get; } = new ObservableCollection<AnnotationViewModel>();

[JsonIgnore]
public string Name
Expand Down Expand Up @@ -419,19 +409,19 @@ public WorkspaceViewModel(WorkspaceModel model, DynamoViewModel dynamoViewModel)
stateMachine = new StateMachine(this);

var nodesColl = new CollectionContainer { Collection = Nodes };
_workspaceElements.Add(nodesColl);
WorkspaceElements.Add(nodesColl);

var connColl = new CollectionContainer { Collection = Connectors };
_workspaceElements.Add(connColl);
WorkspaceElements.Add(connColl);

var notesColl = new CollectionContainer { Collection = Notes };
_workspaceElements.Add(notesColl);
WorkspaceElements.Add(notesColl);

var errorsColl = new CollectionContainer { Collection = Errors };
_workspaceElements.Add(errorsColl);
WorkspaceElements.Add(errorsColl);

var annotationsColl = new CollectionContainer {Collection = Annotations};
_workspaceElements.Add(annotationsColl);
WorkspaceElements.Add(annotationsColl);

//respond to collection changes on the model by creating new view models
//currently, view models are added for notes and nodes
Expand Down Expand Up @@ -513,7 +503,7 @@ public override void Dispose()
Nodes.Clear();
Notes.Clear();
Connectors.Clear();

InCanvasSearchViewModel.Dispose();
}

internal void ZoomInInternal()
Expand Down Expand Up @@ -621,66 +611,66 @@ void CopyPasteChanged(object sender, EventArgs e)
void Connectors_ConnectorAdded(ConnectorModel c)
{
var viewModel = new ConnectorViewModel(this, c);
if (_connectors.All(x => x.ConnectorModel != c))
_connectors.Add(viewModel);
if (Connectors.All(x => x.ConnectorModel != c))
Connectors.Add(viewModel);
}

void Connectors_ConnectorDeleted(ConnectorModel c)
{
var connector = _connectors.FirstOrDefault(x => x.ConnectorModel == c);
var connector = Connectors.FirstOrDefault(x => x.ConnectorModel == c);
if (connector != null)
{
_connectors.Remove(connector);
Connectors.Remove(connector);
connector.Dispose();
}
}

private void Model_NoteAdded(NoteModel note)
{
var viewModel = new NoteViewModel(this, note);
_notes.Add(viewModel);
Notes.Add(viewModel);
}

private void Model_NoteRemoved(NoteModel note)
{
var matchingNoteViewModel = _notes.First(x => x.Model == note);
_notes.Remove(matchingNoteViewModel);
var matchingNoteViewModel = Notes.First(x => x.Model == note);
Notes.Remove(matchingNoteViewModel);
matchingNoteViewModel.Dispose();
}

private void Model_NotesCleared()
{
foreach (var noteViewModel in _notes)
foreach (var noteViewModel in Notes)
{
noteViewModel.Dispose();
}
_notes.Clear();
Notes.Clear();
}

private void Model_AnnotationAdded(AnnotationModel annotation)
{
var viewModel = new AnnotationViewModel(this, annotation);
_annotations.Add(viewModel);
Annotations.Add(viewModel);
}

private void Model_AnnotationRemoved(AnnotationModel annotation)
{
_annotations.Remove(_annotations.First(x => x.AnnotationModel == annotation));
Annotations.Remove(Annotations.First(x => x.AnnotationModel == annotation));
}

private void Model_AnnotationsCleared()
{
_annotations.Clear();
Annotations.Clear();
}

void Model_NodesCleared()
{
foreach(var nodeViewModel in _nodes)
foreach(var nodeViewModel in Nodes)
{
this.unsubscribeNodeEvents(nodeViewModel);
nodeViewModel.Dispose();
}
_nodes.Clear();
Nodes.Clear();
Errors.Clear();

PostNodeChangeActions();
Expand All @@ -694,9 +684,9 @@ private void unsubscribeNodeEvents(NodeViewModel nodeViewModel)

void Model_NodeRemoved(NodeModel node)
{
NodeViewModel nodeViewModel = _nodes.First(x => x.NodeLogic == node);
NodeViewModel nodeViewModel = Nodes.First(x => x.NodeLogic == node);
Errors.Remove(nodeViewModel.ErrorBubble);
_nodes.Remove(nodeViewModel);
Nodes.Remove(nodeViewModel);
//unsub the events we attached below in NodeAdded.
this.unsubscribeNodeEvents(nodeViewModel);
nodeViewModel.Dispose();
Expand All @@ -709,7 +699,7 @@ void Model_NodeAdded(NodeModel node)
var nodeViewModel = new NodeViewModel(this, node);
nodeViewModel.SnapInputEvent += nodeViewModel_SnapInputEvent;
nodeViewModel.NodeLogic.Modified += OnNodeModified;
_nodes.Add(nodeViewModel);
Nodes.Add(nodeViewModel);
Errors.Add(nodeViewModel.ErrorBubble);
nodeViewModel.UpdateBubbleContent();

Expand Down Expand Up @@ -1222,8 +1212,8 @@ internal void FitViewInternal()
else
{
// no selection, fitview all nodes and notes
var nodes = _nodes.Select(x => x.NodeModel);
var notes = _notes.Select(x => x.Model);
var nodes = Nodes.Select(x => x.NodeModel);
var notes = Notes.Select(x => x.Model);
var models = nodes.Concat<ModelBase>(notes);

if (!models.Any()) return;
Expand Down
4 changes: 4 additions & 0 deletions src/DynamoCoreWpf/ViewModels/Preview/InfoBubbleViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,10 @@ public void OnRequestAction(InfoBubbleEventArgs e)

#region Public Methods

/// <summary>
/// Constructor
/// </summary>
/// <param name="dynamoViewModel"></param>
public InfoBubbleViewModel(DynamoViewModel dynamoViewModel)
{
this.DynamoViewModel = dynamoViewModel;
Expand Down
15 changes: 12 additions & 3 deletions src/DynamoCoreWpf/ViewModels/RunSettingsViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

this is an API break.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

{
#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,15 @@ public RunSettingsViewModel(RunSettings settings, HomeWorkspaceViewModel workspa
ToggleRunTypeEnabled(RunType.Periodic, false);
}

/// <summary>
/// When switching workspace, this need to be called in HomeworkspaceViewModel dispose function
/// </summary>
public override void Dispose()
{
base.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
Loading