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

Fix high CPU load #1533

Conversation

Projects
None yet
3 participants
@lontivero
Copy link
Collaborator

commented Jun 4, 2019

Closes #1425 - This PR change the cleaning up process from m*O(n) to m*O(1)

image

Show resolved Hide resolved WalletWasabi/Services/MemPoolService.cs Outdated
@lontivero

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 4, 2019

Okay, I've changed the enumerator for an array just in case one day we change the ConcurrentHashSet by a different collection.

nopara73 added some commits Jun 5, 2019

@nopara73

This comment has been minimized.

Copy link
Collaborator

commented Jun 5, 2019

@lontivero I added a few modifications can you review it? I'll go another iteration of review later.

Also this issue came out on this branch: #1541

I'm not sure if it's related, but it'll make me review it more carefully after you review my changes.


var toRemove = TransactionHashes.Where(x => !allMempoolHashesHashSet.Contains(x.ToString().Substring(0, compactness)));
var toRemove = TransactionHashes.Where(x => !allMempoolHashes.Contains(x.ToString().Substring(0, compactness)));

This comment has been minimized.

Copy link
@lontivero

lontivero Jun 5, 2019

Author Collaborator

I needed to debug it in order to see what ISet type it returns and it returns a HashSet. That would means that the Contains() method that is called is really the HashSet<string>.Contains() and not the ICollection<string>.Contains as the intellisense says. The only problem imo is that it is not evident and is based on the fact that the json deserializer will return the concrete HashSet. I would let the .ToHashSet.

Anyway, I will run this and measure it as before just to be sure it doesn't revert the improvement achieved.

@lontivero

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 5, 2019

I've reviewed it and also tested it. It works okay (there is not increase in the CPU usage while cleaning the mempool).

@nopara73 nopara73 merged commit 2738e45 into zkSNACKs:master Jun 6, 2019

3 of 4 checks passed

Wasabi.Windows in progress
Details
CodeFactor No issues found.
Details
Wasabi.Linux #20190606.1 succeeded
Details
Wasabi.Osx #20190606.1 succeeded
Details

@lontivero lontivero deleted the lontivero:Features/Improve-Mempool-CleanUp-Performance branch Jun 15, 2019

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.