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

Add restore benchmark #157

Merged
merged 5 commits into from
Apr 9, 2019
Merged

Add restore benchmark #157

merged 5 commits into from
Apr 9, 2019

Conversation

rvl
Copy link
Contributor

@rvl rvl commented Apr 5, 2019

Relates to issue #100

Overview

Adds a benchmark for wallet restore, which require cardano-http-bridge already synced to mainnet and testnet.

Comments

This measures time but memory usage can be measured with +RTS -h.

There is not yet the special IsOurs scheme which will cause many transactions to applied to the wallet.

-> IO ()
-- ^ Consume the entire available chain, applying block to the given
-- wallet, and stop when finished. This function is intended for
-- benchmarking.
Copy link
Member

Choose a reason for hiding this comment

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

This is very slippery slope that has caused us a lot of pain in the previous implementations. Let's not introduce code that is test-specific in our codebase. If something is hard to test, I'd rather review our source implementation than introduce tests work-around..
In the end, we do want to test our production code, and not some testing variation of it.

Here's what I'd suggest:

  • Get rid of watchWallet & processWallet altogether.
  • Introduce a new restoreWallet :: WalletId -> (SlotId, SlotId) -> ExceptT RestoreWalletError IO () which would restore a wallet from a starting slot up to another slot.
  • We can pretty much re-use the existing applyBlocks to do so, and simply, add an extra round of getting the "next" slot from the network layer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the lecture, I agree with not introducing test specific code, and that implementing tests improves the API - however -

  1. The problem with the previous implementation was a complete absence of benchmarks, not test-specific code.
  2. There are no call sites of watchWallet yet and at the moment there is no clear spec of how this function or restoreWallet will be used. I suggest fixing it not now but soon when the caller of restoreWallet is written.
  3. In the interests of small short-lived PRs, this change should be done in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

  1. The problem was, unfortunately, also test-specific code. We had entire modules that were almost only present for testing. And functions with arbitrarily complex signatures to allow passing around custom data and short-circuit surrounding logic :( ...

  2. & 3. True, hence why I think we could remove it. We probably added it too early for "testing" whereas in the end, there was still much work to do before that. However, your suggestion is sensible. I'll get it merge as it is and issue a follow-up PR 👍

GotChunk blocks -> do
action start' blocks
go start'
Sleep -> pure ()
Copy link
Member

Choose a reason for hiding this comment

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

The more we make changes to these interfaces, the more it makes me believe that we are doing something wrong and we should be defining functions in the network layer interface instead. I believe, everything we need could be achieved with a simple change in the network interface:

nextBlocks :: SlotId -> ExceptT e0 m Result

data Result
    = NextBlocks !([Blocks], SlotId) -- ^ Have a result, a batch of next blocks and, the next Id to start from
    | NoBlocks -- ^ There is no blocks available now, so wait.

This way, the chain producer implementation is charge of letting others know about what is the next slot id and, we can get rid of listen, getNextBlocks and drain in favor of an approach that is also probably more resilient to the upcoming changes in the network protocol with Shelley.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure - where do you envision the "listen" process sitting in relation to wallet layer and network layer?

Copy link
Member

Choose a reason for hiding this comment

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

I'll make a proposal in another PR, it will be simpler to discuss around that 👍

@KtorZ
Copy link
Member

KtorZ commented Apr 9, 2019

rebased on top of master + removed unused dependencies in dot cabal.

@KtorZ KtorZ merged commit 00e5f66 into master Apr 9, 2019
@KtorZ KtorZ deleted the rvl/100/bench branch April 9, 2019 14:52
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.

None yet

2 participants