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

Unsubscribe coin events #1597

Merged
merged 2 commits into from Jun 21, 2019

Conversation

Projects
None yet
3 participants
@lontivero
Copy link
Collaborator

commented Jun 21, 2019

This PR unsubscribe the coins events after they are removed from the CoinListViewModel. Please review.
Related #1594

@lontivero

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 21, 2019

I think this could be behind the leak of NotifyCollectionChangedEventHandler handlers because I saw 160762 instances taking 10MB of only these event handlers. This is weird because the only type exposing this event handler is the Global.Instance.WalletService.Coins collection and clearly we are leaking instances somewhere.

Count      TotalSize  Class Name
160762     10288768   System.Collections.Specialized.NotifyCollectionChangedEventHandler
@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Jun 21, 2019

I'm running my test on this.

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Jun 21, 2019

When I ran it, I didn't experience the memory overflow with this PR, but I experienced that this code block is taking forever, it's like it's stuck. Not sure this is something that'd always happen, trying to investigate it.

var coins = WaitingList
				.Where(x => x.Value <= DateTimeOffset.UtcNow)
				.Select(x => x.Key) // Only if registering coins is already allowed.
				.Where(confirmationPredicate);

			for (int i = 1; i <= maximumInputCountPerPeer; i++) // The smallest number of coins we can register the better it is.
			{
				var linq = coins.GetPermutations(i);
				linq = linq.Where(x => x.Sum(y => y.Amount) >= amountNeededExceptInputFees + (feePerInputs * i)); // If the sum reaches the minimum amount.

				if (i == 1) // If only one coin is to be registered.
				{
					// Prefer the largest one, so more mixing volume is more likely.
					linq = linq.OrderByDescending(x => x.Sum(y => y.Amount));

					// Try to register with the smallest anonymity set, so new unmixed coins come to the mix.
					linq = linq.OrderBy(x => x.Sum(y => y.AnonymitySet));
				}
				else // Else coin merging will happen.
				{
					// Prefer the lowest amount sum, so perfect mix should be more likely.
					linq = linq.OrderBy(x => x.Sum(y => y.Amount));

					// Try to register the largest anonymity set, so red and green coins input merging should be less likely.
					linq = linq.OrderByDescending(x => x.Sum(y => y.AnonymitySet));
				}

				linq = linq.OrderBy(x => x.Count(y => y.Confirmed == false)); // Where the lowest amount of unconfirmed coins there are.

				IEnumerable<SmartCoin> best = linq.FirstOrDefault();

				if (best != default)
				{
					return best.Select(x => x.GetTxoRef()).ToArray();
				}
			}
@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Jun 21, 2019

Then I restarted and tried to run that code block with different combinations, but it always ran within reasonable time :/

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Jun 21, 2019

Aaah, I just realized I was running this test without checking out this PR.

@nopara73 nopara73 requested a review from molnard Jun 21, 2019

@@ -424,6 +424,7 @@ public void OnOpen()
if (toRemove != default)
{
RootList.Remove(toRemove);
toRemove.UnsubscribeEvents();

This comment has been minimized.

Copy link
@nopara73

nopara73 Jun 21, 2019

Collaborator

The Unsubscription is happening elsewhere: .OnItemRemoved(x => x.UnsubscribeEvents())

This comment has been minimized.

Copy link
@nopara73

nopara73 Jun 21, 2019

Collaborator

However if I use OnItemAdded instead of the equivalent method that you're using then interesting things happen, so maybe the OnItemRemoved should be removed and your method left.

This comment has been minimized.

Copy link
@molnard

molnard Jun 28, 2019

Collaborator

As far as I can remember strange things start happening if you use OnItemRemoved. In some cases there the items in the list messed up. Code should stay like this.

@nopara73 nopara73 merged commit 072e04e into zkSNACKs:master Jun 21, 2019

1 of 2 checks passed

Wasabi.Linux queued
Details
CodeFactor No issues found.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.