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

Introduce Label class (MempoolStore Breakdown Part 3) #2201

Merged
merged 16 commits into from Sep 9, 2019

Conversation

@nopara73
Copy link
Collaborator

commented Sep 1, 2019

I decided to break down PR #2188 to reviewable parts. In this PR I fix the FirstSeen property of the SmartTransaction.

@molnard noted in his review of PR #2188 (#2188 (comment)) that CorrectLabel is being called in inconsistent places. I had to introduce a label class to ensure a fully clean solution.

Also fixes #2199

nopara73 added 3 commits Sep 1, 2019
WalletWasabi/Models/Label.cs Outdated Show resolved Hide resolved
WalletWasabi/Models/Label.cs Outdated Show resolved Hide resolved
@lontivero
Copy link
Collaborator

left a comment

LGTM. Should I test it?

@nopara73

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 2, 2019

Should I test it?

Yes please. (Will look at the comments later.)

Update WalletWasabi/Models/Label.cs
Co-Authored-By: Lucas Ontivero <lontivero@users.noreply.github.com>
WalletWasabi.Tests/IntegrationTests/RegTests.cs Outdated Show resolved Hide resolved
WalletWasabi.Tests/IntegrationTests/RegTests.cs Outdated Show resolved Hide resolved
WalletWasabi/Models/Label.cs Outdated Show resolved Hide resolved
WalletWasabi/Models/Label.cs Outdated Show resolved Hide resolved
WalletWasabi/Models/Label.cs Outdated Show resolved Hide resolved
WalletWasabi/Models/Label.cs Outdated Show resolved Hide resolved
WalletWasabi/Models/Label.cs Outdated Show resolved Hide resolved
nopara73 added 3 commits Sep 3, 2019
@nopara73

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 5, 2019

I've done some wallet loading benchmarking for the labels and it seems like label comparision and construction performance doesn't matter:

function:seconds
IEnumerableStringConstructorTimeSpent:0.0361188
MergeTimeSpent:0.0034401
EqualTimeSpentWithXNull:0.0002268
EqualTimeSpentWithYNull:0
EqualTimeSpentWithXYNull:0
EqualTimeSpentWithoutNull:0

This is strange, also what strange is that we did a lot of null comparision and zero non-null comparision.

@nopara73

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 5, 2019

Next, I looked at the GetClusters and spotted a bunch of contains for the SmartCoin lists.

image

I thought the bottleneck is the SmartCoin equality or GetHashCode then, and done the same measurements and it looks like it is not a bottleneck either:

GetHashCodeTimeSpent:0.0010552
EqualTimeSpentWithXNull:0
EqualTimeSpentWithYNull:0.0005534
EqualTimeSpentWithXYNull:0.0069155
EqualTimeSpentWithoutNull:0.0406135
@nopara73

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 5, 2019

Finally I checked how long the GetClusters take and it turns out it's only 0.1780899 seconds.

@nopara73

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 5, 2019

Sum Up

Comparing labels is NOT a performance bottle neck. I thought it is because I remembered David was optimizing the cluster refreshment in the GUI, but at one point we fixed that, the cluster calculation is not a bottleneck anymore either. I removed the performance optimization code: #2201 (comment)

For reference the measurements on wallet load:

RuntimeParams.LoadAsync
0.0042084
Loading txcache
0.4201773
LoadWalletStateAsync.AssertNetworkOrClearBlockState
0.0006977
LoadWalletStateAsync. Parocess BlockStates
188.1793529
LoadWalletStateAsync. Process Leftover Filters
14.1359743
LoadDummyMempoolAsync
0.0005727
RefreshCoinHistories
0.0851518

Where RefreshCoinHistories is about the cluster calculation, takes only 0.08 sec.

@nopara73

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 5, 2019

@molnard @lontivero Ready to be merged?

@nopara73

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 5, 2019

Wait, something is wrong with the tests :o

@nopara73

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 5, 2019

lol, this commit ruined the tests: d854f71

Will check what's wrong later.

@nopara73

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 5, 2019

This makes no sense, it shouldn't be an issue. I may just revert it @molnard @lontivero (unless you want to spend hours figuring out wtf.)

@molnard

This comment has been minimized.

Copy link
Collaborator

commented Sep 5, 2019

CI says: The active test run was aborted. Reason: Process is terminating due to StackOverflowException.

@nopara73

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 5, 2019

@molnard your "readable suggestion" resulted in infinite recursion.

@nopara73

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 5, 2019

Can't use != on labels == overriding, because != label overriding uses ==

@molnard

This comment has been minimized.

Copy link
Collaborator

commented Sep 5, 2019

x != null is using the

public static bool operator !=(Label x, Label y) => !(x == y);

?

@molnard

This comment has been minimized.

Copy link
Collaborator

commented Sep 5, 2019

Maybe you should use only the is operator and the inverse of that. An older example with Object.ReferenceEquals here: https://www.loganfranken.com/blog/698/overriding-equals-in-c-part-3/

@nopara73

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 5, 2019

Sure,but meh, that's ugly. Better be like it was before.

nopara73 added 2 commits Sep 5, 2019
@nopara73

This comment has been minimized.

Copy link
Collaborator Author

commented Sep 5, 2019

Ready for merge as far as I'm concerned.

@molnard
Copy link
Collaborator

left a comment

LGTM

.Select(x => x.Trim())
.Where(x => x != "")
.Distinct(StringComparer.OrdinalIgnoreCase)
.OrderBy(x => x)

This comment has been minimized.

Copy link
@lontivero

lontivero Sep 9, 2019

Collaborator

This is just a comment and not a suggestion to change anything now:
It would be good to consider the user's culture when we order the labels because it looks incorrect with other than English language.

@lontivero
Copy link
Collaborator

left a comment

The logic is okay. I only have a few comments. After that this can be merged.

@nopara73 nopara73 merged commit 27d6edd into zkSNACKs:master Sep 9, 2019

1 of 4 checks passed

Wasabi.Linux in progress
Details
Wasabi.Osx queued
Details
Wasabi.Windows in progress
Details
CodeFactor No issues found.
Details

@nopara73 nopara73 deleted the nopara73:label branch Sep 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.