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

Extend restoration benchmarks to include large random wallets. #2081

Merged
merged 3 commits into from
Aug 26, 2020

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented Aug 26, 2020

Issue Number

#2032

Overview

  • I have defined a new special wallet scheme analogous to the any% scheme used until now, but built on top of the RndState, so that we still get to store and retrieve the address space too.

  • I have removed the 2% case and "replaced" it with a 1% case on large random wallets.

Comments

Tested on the testnet, which works fine. The reason to add this one is to be able to create arbitrarily large random wallets that "make sense", and then measure the time taken for other specific operations such as listing addresses or estimating fees (which I'll do as a separate PR).

@KtorZ KtorZ requested a review from Anviking August 26, 2020 13:55
@KtorZ KtorZ self-assigned this Aug 26, 2020
@KtorZ KtorZ added the RESOLVING ISSUE Mark a PR as resolving issues, for auto-generated CHANGELOG label Aug 26, 2020
-- seed.
--
-- The first argument is expected to be a ratio (between 0 and 1) of addresses
-- we ought to simply recognize as ours. So, giving .5 means that 50% of the
Copy link
Member

Choose a reason for hiding this comment

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

Seems outdated. Should be ratio (between 0% and 100%) and giving @50 means 50% of the I presume?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I had it as an argument initially and switched to a type-level parameter later on.

giving @50 means 50% of the I presume?

Indeed.

-- For benchmarking and testing arbitrary large random wallets.

-- | An "unsound" alternative that can be used for benchmarking and stress
-- testing. It re-uses the same underlying structure as the `RndState` but
Copy link
Member

Choose a reason for hiding this comment

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

same underlying structure

Is there a main difference, say in complexity, between RndAnyState and AnyAddressState?

Copy link
Member

Choose a reason for hiding this comment

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

Would this be true?

RndAnyState, like RndState, grow with the number of addresses used. This is unlike SeqState/AnyAddressState, where which has a bounded gap of user addresses it keeps in memory

If not, something like it, would be cool to understand what makes RndAnyState indicative of performance of random wallets, and why it's not enough to have AnyAddressState.

Copy link
Member

@Anviking Anviking Aug 26, 2020

Choose a reason for hiding this comment

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

Re my earlier comment:

Ah, I'm blind 😅

innerState :: RndState network

"same underlying structure" makes sense then

Copy link
Member Author

Choose a reason for hiding this comment

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

:) ... It still stores the UTxO and transaction history, but the AnyState do not store the address space. BUT, we've observed some major slowness when it comes to storing addresses, so I find it quite important to also include that in the benchmark.

I think that we could have a similar SeqAnyState which also include the seq state, and get rid of the AnyState altogether.

socketFile
np
vData
"1-percent-naked.timelog"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just 1-percent.timelog would be easier


, bench ("restore " <> network <> " 2% ownership")
Copy link
Member

Choose a reason for hiding this comment

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

Removing because it was too slow, I presume?

Maybe it's worth adding 0.1%, or 0.5%, to still have more data points than 0% and 1%.

(Useful, I imagine, if you'd ever want to do some curve fitting on the data)

Copy link
Member Author

Choose a reason for hiding this comment

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

Removing because it was too slow, I presume?

Correct.

Maybe it's worth adding 0.1%, or 0.5%, to still have more data points than 0% and 1%.

Good idea 👍

-- For benchmarking and testing arbitrary large random wallets.

-- | An "unsound" alternative that can be used for benchmarking and stress
-- testing. It re-uses the same underlying structure as the `RndState` but
Copy link
Member

@Anviking Anviking Aug 26, 2020

Choose a reason for hiding this comment

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

Re my earlier comment:

Ah, I'm blind 😅

innerState :: RndState network

"same underlying structure" makes sense then

KtorZ added 3 commits August 26, 2020 16:56
  This wallet is very analogous to the existing any% wallet scheme we designed a while ago, with a subtle difference: it is built __on top of__ the 'RndState' and, as a result, does perform the same database operation and addresses management as the standard random wallets. So the benchmark results obtained from this are much closer to what an actual random wallet of the same size would look like.
  This is actually the implementation we ought to test in these benchmark.
@KtorZ KtorZ force-pushed the KtorZ/2032/extend-restoration-benchmarks branch from f0a4bf9 to 580bd96 Compare August 26, 2020 14:56
@KtorZ
Copy link
Member Author

KtorZ commented Aug 26, 2020

(False, _) | crc32 bytes < p ->
let
(path, gen') = findUnusedPath
(gen inner) (accountIndex inner) (unavailablePaths inner)
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't this extra findUnusedPath call, compared to the inner state implementation, affect the results?

Copy link
Member Author

Choose a reason for hiding this comment

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

A bit yes. We don't benchmark this function independently but I believe that even on large wallets with 100K+ addresses, finding an unused path in a space of 2^31 is quite fast.

Copy link
Member Author

Choose a reason for hiding this comment

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

So I'd expect it to be quite negligible.

@KtorZ
Copy link
Member Author

KtorZ commented Aug 26, 2020

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 26, 2020

Build succeeded

@iohk-bors iohk-bors bot merged commit ef1ed03 into master Aug 26, 2020
@iohk-bors iohk-bors bot deleted the KtorZ/2032/extend-restoration-benchmarks branch August 26, 2020 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RESOLVING ISSUE Mark a PR as resolving issues, for auto-generated CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants