Add {100,10,1}-wallet restore benchmarks #2243

Anviking · 2020-10-14T12:14:15Z

Issue Number

ADP-469

Overview

Auxiliary: Make benchmark actually respect passing in --node-db
Auxiliary: Add --running-node SOCKET_PATH for easier development
Benchmark restoring 100, 10 and 1 wallet to 1%
- Some tweaks to helpers to support this
Refactor to make the benchmark declarations more high-level and readable
Make sure the plotting of the results hasn't boken

Comments

Currently we won't a get plot for these new multi-wallet benchmarks, like for the existing ones. Could consider adding in another PR.
Open to disagreement on only restoring 1% (here is the old thread), but I think even 10% might be tricky to pull off in this PR. We'd need much more work for that.

Anviking · 2020-10-14T13:49:21Z

lib/shelley/bench/Restore.hs

@@ -711,7 +769,7 @@ waitForNodeSync tr nw = loop 10

 data BenchmarkLog (n :: NetworkDiscriminant)
    = MsgNodeTipTick BlockHeader SyncProgress
-    | MsgRestorationTick SyncProgress
+    | MsgRestorationTick POSIXTime [SyncProgress]


Why did I add this? Might not be needed yet.

well, below you don't use it in toText, so probably superflouous ;-)

But why not introduce MsgRestorationTick [(POSIXTime,SyncProgress)] to have this time for each sync progress? that would be useful probably

lib/shelley/bench/Restore.hs

Anviking · 2020-10-14T15:09:11Z

lib/shelley/bench/Restore.hs

-            (walletSeq "0.4-percent-seq" $ mkSeqAnyState' @4 networkProxy)
-            benchmarksSeq
+    runBenchmarks
+        [ benchRestoreMultipleWallets 1


Takes 6s to reach 1%.

I suspect the extrapolated value (600s to 100%) is a bit wrong.

Could increase the number of wallets for this test only

Could leave it as is

Could increase the restore-to target for this test only

Could increase restore-to target for all {1,10,100} cases (but 100 would be really slow)

Locally 1 wallet takes 276.4 s, vs 600s extrapolated, so pretty wrong, yes.

I should also note though, that when I compared 100 wallets to 1% extrapolated vs 100 wallets to 10%, they were very similar.

so what's executive summary of the above?

Summary:

Should restore the 1 wallet to 10% for better extrapolation. But we can keep 10 and 100 wallets restoring to 1%, where it doesn't matter much.

There may be some interesting slowdowns (e.g. 10%, 16%), only visible when restoring to 100% (as Piotr also suggested), but I think we can ignore that in this PR.

Based on the data:

To 1%

1 wallets -> 4.015s (extrapolation is 54% higher than the to-10% value ❌)

10 wallets -> 24.62s (extrapolation is 0.9% higher than the to-10% value 🆗 )

100 wallets -> 345.0s (extrapolation is 0.8% higher than the to-10% value 🆗 )

To 10%

1 wallets -> 26.13s (extrapolation is 9.3% lower than the to-100% value 🤔 )

10 wallets -> 244.0s (extrapolation is 14.1% lower than the to-100% value 🤔 )

100 wallets -> 3423s (IIRC)

To 100% (includes the Shelley part of the chain)

1 wallets -> 288s

10 wallets -> 2842s

100 wallets -> 38578s

Anviking · 2020-10-14T15:17:47Z

Triggered Buildkite nightly: https://buildkite.com/input-output-hk/cardano-wallet-nightly/builds/673#cc255463-69ec-4691-80b0-25042a53eced

paweljakubas · 2020-10-14T16:35:02Z

lib/core-integration/src/Cardano/Wallet/BenchShared.hs

-    void $ withNetworkConfiguration args $ \nodeConfig ->
-        withCardanoNode (trMessageText tr) nodeConfig $ \cp ->
-            action tr (networkConfig args) (nodeSocketFile cp)
+    case argUseAlreadyRunningNodeSocketPath args of


paweljakubas · 2020-10-14T16:37:12Z

lib/shelley/bench/Restore.hs

-            (walletRnd "0.4-percent-rnd" $ mkRndAnyState @4)
+            [walletRnd (showPercent permillProxy <> "-percent-rnd")
+                $ mkRndAnyState' permillProxy networkProxy]
+            True -- Write progress to .timelog file


Is this comment correct? The one in line 235 is opposite but the value is also True

Thanks, the value was supposed to be False on line 235.

Could have added a new datatype to better represent this, but maybe I should more fundamentally refactor the bench_restoration function instead 🤔

lib/shelley/bench/Restore.hs

paweljakubas · 2020-10-14T16:48:11Z

lib/shelley/bench/Restore.hs

@@ -9,6 +9,7 @@
 {-# LANGUAGE LambdaCase #-}
 {-# LANGUAGE MultiParamTypeClasses #-}
 {-# LANGUAGE NamedFieldPuns #-}
+{-# LANGUAGE NoMonoLocalBinds #-}


ok, will need to read about it 😅 At this moment do not understand why it is used

So we have the main bench_results function with a bunch of arguments, type parameters and constraints.

I wanted to define more descriptive helpers where I apply most of the boring arguments, e.g.:

let benchRestoreRndWithOwnership permillProxy = bench_restoration networkProxy (trMessageText tr) socketFile np vData [walletRnd (showPercent permillProxy <> "-percent-rnd") $ mkRndAnyState' permillProxy networkProxy] True -- Write progress to .timelog file (unsafeMkPercentage 1) benchmarksRnd

But the per-mille ownership is a type parameter, and Haskell will fix it when inferring the type of the let binding.

benchRestoreRndWithOwnership (Proxy @0) -- compiles benchRestoreRndWithOwnership (Proxy @1) -- Expected type: Proxy 0; Actual type: Proxy 1

Moving it to a where-clause, and explicitly specifying forall (p :: Nat) didn't seem like a good solution because the full type signature is really big and cumbersome, and I wouldn't have access to the locally bound vData, np, networkProxy.

With NoMonoLocalBinds it just works.

Looking here it seems that MonoLocalBinds is off by default, but enabled automatically by GADTs. Didn't know that.

The extension MonoLocalBinds is implied by TypeFamilies and GADTs. You can switch it off again with NoMonoLocalBinds but type inference becomes less predictable if you do so. (Read the papers!)

lib/shelley/bench/Restore.hs

paweljakubas

LGTM

KtorZ · 2020-10-16T14:01:14Z

lib/shelley/bench/Restore.hs

-            benchmarksSeq
+        [ benchRestoreMultipleWallets 1 (unsafeMkPercentage 0.1)
+        , benchRestoreMultipleWallets 10 (unsafeMkPercentage 0.01)
+        , benchRestoreMultipleWallets 100 (unsafeMkPercentage 0.01)


Why not using the same percentage here? That will skew the benchmark a bit, wouldn't it?

Why not using the same percentage here?

To make it run in a few minutes rather than more than an hour.

See #2243 (comment)

That will skew the benchmark a bit, wouldn't it?

Well, only if you interpret in the wrong way.

I mean, why not:

[ benchRestoreMultipleWallets 1 (unsafeMkPercentage 0.01) , benchRestoreMultipleWallets 10 (unsafeMkPercentage 0.01) , benchRestoreMultipleWallets 100 (unsafeMkPercentage 0.01)

The first 0.1 looks like a typo to me ?

Ah — the extrapolation from 0.01 to 0.1 is 54% off because it is so fast (imprecise start and stop times + ramping-up time, I take).

Extrapolating from 0.01 to 0.1 in the for 10 and 100 wallets saves an hour of time, but the accuracy of the extrapolation seem within 1%.

instead of hard-coding to "db"

Anviking · 2020-10-19T10:36:19Z

https://buildkite.com/input-output-hk/cardano-wallet-nightly/builds/680#ea0dfcad-4a8b-4461-99cc-a826b0a4eba1

BenchSeqResults:
  benchName: 1-wallets-to-0.1
  restoreTime: 40.50 s
  readWalletTime: 2.820 ms
  listAddressesTime: 156.5 μs
  listTransactionsTime: 552.8 μs
  estimateFeesTime: 9.631 s
  walletOverview:
     number of addresses: 20
     number of transactions: -0
     = Total value of -0 lovelace across 0 UTxOs

BenchSeqResults:
  benchName: 10-wallets-to-0.01
  restoreTime: 32.13 s
  readWalletTime: 37.23 ms
  listAddressesTime: 1.256 ms
  listTransactionsTime: 1.549 ms
  estimateFeesTime: 12.66 s
  walletOverview:
     number of addresses: 20
     number of transactions: -0
     = Total value of -0 lovelace across 0 UTxOs

BenchSeqResults:
  benchName: 100-wallets-to-0.01
  restoreTime: 551.2 s
  readWalletTime: 7.828 ms
  listAddressesTime: 99.44 μs
  listTransactionsTime: 665.3 μs
  estimateFeesTime: 19.78 s
  walletOverview:
     number of addresses: 20
     number of transactions: -0
     = Total value of -0 lovelace across 0 UTxOs

which silences the verbose tick output to stdout.

Anviking · 2020-10-19T10:55:13Z

🤞
bors r+

2243: Add {100,10,1}-wallet restore benchmarks r=Anviking a=Anviking # Issue Number ADP-469 # Overview  - [x] Auxiliary: Make benchmark actually respect passing in `--node-db` - [x] Auxiliary: Add `--running-node SOCKET_PATH` for easier development - [x] Benchmark restoring 100, 10 and 1 wallet to 1% - [x] Some tweaks to helpers to support this - [x] Refactor to make the benchmark declarations more high-level and readable - [x] Make sure the plotting of the results hasn't boken # Comments - Currently we won't a get plot for these new multi-wallet benchmarks, like for the existing ones. Could consider adding in another PR. - Open to disagreement on only restoring 1% ([here](https://input-output-rnd.slack.com/archives/GBT05825V/p1602674480071800?thread_ts=1602142785.076200&cid=GBT05825V) is the old thread), but I think even 10% might be tricky to pull off in this PR. We'd need much more work for that.   Co-authored-by: Johannes Lund <[email protected]>

iohk-bors · 2020-10-19T12:07:33Z

Build failed:

buildkite/cardano-wallet


builder for '/nix/store/cd3n0bwmg2g515f2h7w3lcrn1369f55g-dot-cabal-hackage.haskell.org-at-2020-10-03T000000Z.drv' failed with exit code 1
--
  | cannot build derivation '/nix/store/p4qz6gfr3z9dn743m64lv6s4dmrr8qar-haskell-language-server-plan-to-nix-pkgs.drv': 1 dependencies couldn't be built

Anviking · 2020-10-19T13:34:30Z

bors r+

iohk-bors · 2020-10-19T15:18:59Z

Build succeeded:

Anviking commented Oct 14, 2020

View reviewed changes

lib/shelley/bench/Restore.hs Outdated Show resolved Hide resolved

Anviking commented Oct 14, 2020

View reviewed changes

lib/shelley/bench/Restore.hs Outdated Show resolved Hide resolved

Anviking force-pushed the anviking/ADP-469/100-wallet-restore branch from 768675c to 03d31a3 Compare October 14, 2020 14:57

Anviking commented Oct 14, 2020

View reviewed changes

Anviking marked this pull request as ready for review October 14, 2020 15:11

Anviking self-assigned this Oct 14, 2020

Anviking requested a review from paweljakubas October 14, 2020 15:17

paweljakubas reviewed Oct 14, 2020

View reviewed changes

lib/shelley/bench/Restore.hs Outdated Show resolved Hide resolved

paweljakubas reviewed Oct 14, 2020

View reviewed changes

lib/shelley/bench/Restore.hs Outdated Show resolved Hide resolved

paweljakubas reviewed Oct 14, 2020

View reviewed changes

Anviking commented Oct 15, 2020

View reviewed changes

lib/shelley/bench/Restore.hs Outdated Show resolved Hide resolved

paweljakubas approved these changes Oct 15, 2020

View reviewed changes

Anviking force-pushed the anviking/ADP-469/100-wallet-restore branch 2 times, most recently from c07e99c to 6839b47 Compare October 15, 2020 15:56

KtorZ reviewed Oct 16, 2020

View reviewed changes

Anviking added 2 commits October 17, 2020 20:35

Actually use argNodeDatabaseDir in bench

2b82952

instead of hard-coding to "db"

Support running bench:restore using already running cardano-node

03d0aac

Anviking force-pushed the anviking/ADP-469/100-wallet-restore branch from dbd4938 to 6c9422b Compare October 17, 2020 18:37

Anviking added 2 commits October 19, 2020 12:51

Add 100-wallet restore benchmarks and refactor

cf900bf

Add --quiet option to restore bench

9b22f9e

which silences the verbose tick output to stdout.

Anviking force-pushed the anviking/ADP-469/100-wallet-restore branch from 6c9422b to 9b22f9e Compare October 19, 2020 10:52

iohk-bors bot merged commit 5c00a79 into master Oct 19, 2020

iohk-bors bot deleted the anviking/ADP-469/100-wallet-restore branch October 19, 2020 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add {100,10,1}-wallet restore benchmarks #2243

Add {100,10,1}-wallet restore benchmarks #2243

Anviking commented Oct 14, 2020 •

edited

Loading

Anviking Oct 14, 2020

paweljakubas Oct 14, 2020

paweljakubas Oct 14, 2020

Anviking Oct 14, 2020

Anviking Oct 14, 2020 •

edited

Loading

paweljakubas Oct 14, 2020

Anviking Oct 15, 2020 •

edited

Loading

paweljakubas Oct 15, 2020

Anviking commented Oct 14, 2020

paweljakubas Oct 14, 2020

paweljakubas Oct 14, 2020

Anviking Oct 15, 2020

paweljakubas Oct 14, 2020

Anviking Oct 15, 2020

paweljakubas Oct 15, 2020

paweljakubas left a comment

KtorZ Oct 16, 2020

Anviking Oct 16, 2020

KtorZ Oct 16, 2020

Anviking Oct 16, 2020 •

edited

Loading

Anviking commented Oct 19, 2020

Anviking commented Oct 19, 2020

iohk-bors bot commented Oct 19, 2020 •

edited by Anviking

Loading

Anviking commented Oct 19, 2020

iohk-bors bot commented Oct 19, 2020

Add {100,10,1}-wallet restore benchmarks #2243

Add {100,10,1}-wallet restore benchmarks #2243

Conversation

Anviking commented Oct 14, 2020 • edited Loading

Issue Number

Overview

Comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anviking Oct 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anviking Oct 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anviking commented Oct 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anviking Oct 16, 2020 • edited Loading

Choose a reason for hiding this comment

Anviking commented Oct 19, 2020

Anviking commented Oct 19, 2020

iohk-bors bot commented Oct 19, 2020 • edited by Anviking Loading

Anviking commented Oct 19, 2020

iohk-bors bot commented Oct 19, 2020

Anviking commented Oct 14, 2020 •

edited

Loading

Anviking Oct 14, 2020 •

edited

Loading

Anviking Oct 15, 2020 •

edited

Loading

Anviking Oct 16, 2020 •

edited

Loading

iohk-bors bot commented Oct 19, 2020 •

edited by Anviking

Loading