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.

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

Further memory leak fixes #1212

Merged
merged 55 commits into from Mar 26, 2019
Merged

Conversation

danwalmsley
Copy link

@danwalmsley danwalmsley commented Mar 7, 2019

Closes #1001
Closes #1094

Removed use of IDisposable in viewmodels, this is not really the correct way to handle this.

Added support in Shell for detecting when a Tab is opened

Initialise and deinitialise coinlist stuff when tab is opened and closed (freeing the memory).

@nopara73
Copy link
Contributor

nopara73 commented Mar 7, 2019

  • memory grows while clikcing on ordering
  • closing and reopening tabs don't seem to grew memory

What else were the issues?

@nopara73 nopara73 requested a review from molnard March 8, 2019 15:16
Copy link
Collaborator

@molnard molnard left a comment

Choose a reason for hiding this comment

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

I tested it on MAC. No memory leaks when reopening tabs. Regarding ordering the memory increase until 600 Mb then it stops. Looks good

@lontivero
Copy link
Collaborator

Just a comment for clarification:

  • memory grows while clikcing on ordering: this was fixed or at least I don't see it happening now.
  • closing and reopening tabs don't seem to grew memory: this still happens but the current behavior is much better than before.

@lontivero
Copy link
Collaborator

I didn't see the @molnard's comment because we both commented at the same time.

600 Mb then it stops.

I can confirm this. In my cases it stops after 780MB or so.

closing and reopening tabs don't seem to grew memory

It does in my Linux. However it stops eating memory and goes back after GC pass.

Looks good to me too.

@molnard
Copy link
Collaborator

molnard commented Mar 19, 2019

The same errors for me on win10x64. One more thing to add: if you send and get the invalid thread exc then go into receive and generate a new receive address you will get:

System.ArgumentException
  HResult=0x80070057
  Message=An item with the same key has already been added. Key: [0, Avalonia.Controls.Generators.ItemContainerInfo]
  Source=System.Collections
  StackTrace:
   at System.Collections.Generic.TreeSet`1.AddIfNotPresent(T item)
   at System.Collections.Generic.SortedDictionary`2.Add(TKey key, TValue value)
   at Avalonia.Controls.Generators.ItemContainerGenerator.Materialize(Int32 index, Object item, IMemberSelector selector)
   at Avalonia.Controls.Presenters.ItemVirtualizerNone.AddContainers(Int32 index, IEnumerable items)
   at Avalonia.Controls.Presenters.ItemVirtualizerNone.ItemsChanged(IEnumerable items, NotifyCollectionChangedEventArgs e)
   at Avalonia.Reactive.LightweightObservableBase`1.PublishNext(T value)
   at Avalonia.Utilities.WeakSubscriptionManager.Subscription`1.OnEvent(Object sender, T eventArgs)
   at System.Collections.ObjectModel.ObservableCollection`1.OnCollectionChanged(NotifyCollectionChangedEventArgs e)
   at System.Collections.ObjectModel.ObservableCollection`1.OnCollectionChanged(NotifyCollectionChangedAction action, Object item, Int32 index)
   at System.Collections.ObjectModel.ObservableCollection`1.InsertItem(Int32 index, T item)
   at WalletWasabi.Gui.Controls.WalletExplorer.ReceiveTabViewModel.<.ctor>b__17_13() in C:\WORK\WalletWasabi\WalletWasabi.Gui\Controls\WalletExplorer\ReceiveTabViewModel.cs:line 71

@danwalmsley
Copy link
Author

@lontivero I took out the sorting headers because they trigger a bug in avalonia which I need to have fixed to stop memory leak.

@danwalmsley
Copy link
Author

Will fix the threading issues

@molnard
Copy link
Collaborator

molnard commented Mar 21, 2019

@lontivero I took out the sorting headers because they trigger a bug in avalonia which I need to have fixed to stop memory leak.

is this the next step?

@danwalmsley
Copy link
Author

@lontivero I took out the sorting headers because they trigger a bug in avalonia which I need to have fixed to stop memory leak.

is this the next step?

That's been worked on in avalonia, did you verify if my fix solved the issue with send tab?

@molnard
Copy link
Collaborator

molnard commented Mar 22, 2019

@lontivero I took out the sorting headers because they trigger a bug in avalonia which I need to have fixed to stop memory leak.
is this the next step?

That's been worked on in avalonia, did you verify if my fix solved the issue with send tab?

yes it is OK now

@danwalmsley danwalmsley marked this pull request as ready for review March 23, 2019 10:30
Copy link
Collaborator

@molnard molnard left a comment

Choose a reason for hiding this comment

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

Looks good Tab open/close memleak gone!

WalletWasabi.Gui/MainWindow.xaml.cs Show resolved Hide resolved
OperationMessage = "Dequeuing coins...Please wait";
var canCancel = this.WhenAnyValue(x => x.IsBusy);
var canOk = this.WhenAnyValue(x => x.IsBusy, (isbusy) => !isbusy);

CancelTokenSource = new CancellationTokenSource().DisposeWith(Disposables);
CancelTokenSource = new CancellationTokenSource();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be disposed!

Copy link
Author

Choose a reason for hiding this comment

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

ack

#pragma warning restore CS4014 // Because this call is not awaited, execution of the current method continues before the call is completed
}).DisposeWith(Disposables);
.Subscribe(async _ => await RewriteTableAsync())
.DisposeWith(_disposables);

this.WhenAnyValue(x => x.SelectedTransaction).Subscribe(transaction =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also defined in the constructor. Remove this or that.

Copy link
Author

Choose a reason for hiding this comment

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

ack

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this still there?

_model = model;
ClipboardNotificationVisible = false;
ClipboardNotificationOpacity = 0;

_confirmed = model.WhenAnyValue(x => x.Confirmed).ToProperty(this, x => x.Confirmed, model.Confirmed).DisposeWith(Disposables);

//TODO: this should be disposed! Memory leak!
Global.UiConfig.WhenAnyValue(x => x.LurkingWifeMode).Subscribe(x =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be disposed! Memory leak!

Copy link
Author

Choose a reason for hiding this comment

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

ack

@molnard
Copy link
Collaborator

molnard commented Mar 25, 2019

@nopara73 it is ready to merge

fixes: #1001

@nopara73 nopara73 merged commit c814b8c into zkSNACKs:master Mar 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Memory leak in view 2nd screen
4 participants