Reduce memory usage of create staked command #4021

newhoggy · 2022-06-09T06:38:56Z

The problem arises we build a very large in-memory JSON value and then write it out as genesis.json. The amount of memory used by the in-memory JSON value can be so large as to use up all the memory.

This PR reduces the severity of the memory usage in two ways.

It introduces a ListMap type, which is almost like Map, but has a [(k, v)] as its internal representation. This avoids the serialisation cost of constructing a map only to convert it back into a list.
Uses Lazy IO so that generated stuffed utxos are created on demand rather than upfront an all in memory.
Introduces a new LazyToJson type class which doesn't have the memory retention problems of aeson library.
For evaluation of fields to WHNF to allow parent object to be GCed which allows large fields that have already be serialised to be collected as well.
Writes delegations to a single delegations.jsonl file which is a newline delimited JSON file. This file is streamed multiple times so that generation of the genesis.json file does not retain memory unnecessarily.

This PR also changes the command to no longer generate payment keys and stake keys. If we want to have the ability optionally output these files, there is additional work to do.

Addresses #3938

cardano-cli/src/Cardano/CLI/Shelley/Run/Genesis/ListMap.hs

newhoggy · 2022-06-13T11:42:34Z

Example run:

$ time dist-newstyle/build/aarch64-osx/ghc-8.10.7/cardano-cli-1.33.0/x/cardano-cli/build/cardano-cli/cardano-cli genesis create-staked --genesis-dir example --supply 10000000000000 --gen-utxo-keys 1 --gen-genesis-keys 0 --supply-delegated 2000000000000000 --gen-pools 2 --gen-stake-delegs 1300000 --testnet-magic 42 --num-stuffed-utxo 8000000
generated genesis with: 0 genesis keys, 1 non-delegating UTxO keys, 2 stake pools, 1300000 delegating UTxO keys, 1300000 delegation map entries,
 genesis create-staked --genesis-dir example --supply 10000000000000  1  0     441.74s user 321.01s system 99% cpu 12:47.91 total

newhoggy · 2022-06-14T05:44:35Z

Slight improvement and uses about 1GB of memory:

$ time dist-newstyle/build/aarch64-osx/ghc-8.10.7/cardano-cli-1.33.0/x/cardano-cli/build/cardano-cli/cardano-cli genesis create-staked --genesis-dir example --supply 10000000000000 --gen-utxo-keys 1 --gen-genesis-keys 0 --supply-delegated 2000000000000000 --gen-pools 2 --gen-stake-delegs 1300000 --testnet-magic 42 --num-stuffed-utxo 8000000
generated genesis with: 0 genesis keys, 1 non-delegating UTxO keys, 2 stake pools, 1300000 delegating UTxO keys, 1300000 delegation map entries,
 genesis create-staked --genesis-dir example --supply 10000000000000  1  0     133.75s user 290.89s system 98% cpu 7:12.31 total

cardano-cli/src/Cardano/CLI/Shelley/Run/Address.hs

cardano-cli/src/Cardano/CLI/Shelley/Run/Genesis.hs

cardano-cli/src/Cardano/CLI/Shelley/Run/StakeAddress.hs

cardano-cli/src/Cardano/CLI/Shelley/Run/Address.hs

Jimbo4350 · 2022-06-14T13:12:32Z

I need to make a second pass

Jimbo4350

Latest review comments courtesy of @dcoutts .

Jimbo4350 · 2022-06-14T15:30:29Z

cardano-cli/src/Cardano/CLI/Shelley/Run/Genesis.hs

+      distribution = [pool | (pool, poolIx) <- zip pools [1 ..], _ <- [1 .. delegsForPool poolIx]]
+
+  -- Distribute M delegates across N pools:
+  delegations <- liftIO $ Lazy.forM distribution $ computeDelegation network


I would say lets just implement out own version of forM and replicateM in the cli and remove the hw-lazy dependency

cardano-node doesn't seem the right place to put them given how generic they are. Can I put them into cardano-base?

Note these sorts of functions need associated tests.

cardano-prelude might be a good place for those functions and tests, unless the overall plan is to stop depending on cardano-prelude (at least that is what we are striking for in cardano-base and cardano-ledger repos)

I've put them in Cardano.CLI.IO.Lazy for now. It can be moved to a better place in a different PR if we find one.

Jimbo4350 · 2022-06-14T15:40:39Z

cardano-cli/src/Cardano/CLI/Shelley/Run/Genesis.hs

+  -- times ensures that any data structures that are created as a result of the read is not
+  -- retained in memory.
+
+  let numDelegations = length delegations


This is suspicious, calling length could destroy the laziness. We could get numDelegations in this list comprehension: distribution = [pool | (pool, poolIx) <- zip pools [1 ..], _ <- [1 .. delegsForPool poolIx]] if we multiply the number of pools by the number of delegations per pool

Yes, calling length destroys laziness, but I've been in discussion with @kosyrevSerge and he is happy to pay the memory cost in this case.

Note, length is not the only thing that "destroys laziness". Using delegations more than once does the same thing. Previously, delegations was written to a temporary file and the temporarily file was read multiple times to preserve laziness, but @kosyrevSerge didn't want to pay the serialisation cost of that.

The comment above is out of date however and I've deleted it.

Jimbo4350 · 2022-06-14T15:46:54Z

cardano-cli/src/Cardano/CLI/Shelley/Run/Genesis.hs

-    StakeVerificationKey stakeVK <- firstExceptT ShelleyGenesisCmdTextEnvReadFileError
-      . newExceptT
-      $ readFileTextEnvelope (AsVerificationKey AsStakeKey) stakeVKF
+computeDelegation :: ()


So in this function we are generating two keys but all we are interested in are their hashes. We should create a new primitive in the cli that creates non-cryptographically secure key hashes for testing purposes like this.

We can use System.Random for the entropy and hashFromBytes (in cardano-base) to generate dummy key hashes. This should speed things up a lot as generateSigningKey is an expensive function.

A new generateInsecureSigningKey function has been introduced.

Jimbo4350 · 2022-06-14T15:48:17Z

cardano-cli/src/Cardano/CLI/Shelley/Run/Genesis/ListMap.hs

+import qualified Data.List as L
+import qualified Data.Vector as V
+
+newtype ListMap k v = ListMap


We should be able to generically derive the JSON instances for ListMap and it should do the right thing. @newhoggy can you try this?

Note, generically deriving JSON would do one of two things:

deriving newtype would change the format of the JSON file to use [["key1", "value1"], ["key2", "value2"]] instead of {"field1": "value1", "field2": "value2"}.

DeriveGeneric would additionally add { "unListMap": ... } wrapper.

Looks like we're settling on changing the ledger code, so this code will disappear once the ledger has been updated.

I'm keeping this the same for now.

cardano-cli/src/Cardano/CLI/Shelley/Run/Genesis/Types.hs

Some changes addressed

Jimbo4350 · 2022-07-06T20:17:23Z

cabal.project

@@ -330,6 +330,18 @@ source-repository-package
  tag: ee59880f47ab835dbd73bea0847dab7869fc20d8
  --sha256: 1lrzknw765pz2j97nvv9ip3l1mcpf2zr4n56hwlz0rk7wq7ls4cm

+source-repository-package


I still think we should duplicate the functions used from hw-lazy. The node has enough dependencies and we need to be stricter about introducing new ones.

I put the functions into Cardano.CLI.IO.Lazy for the moment.

Jimbo4350 · 2022-07-06T20:20:38Z

cardano-api/src/Cardano/Api/Key.hs

@@ -67,6 +69,16 @@ generateSigningKey keytype = do
    seedSize = deterministicSigningKeySeedSize keytype


+generateInsecureSigningKey :: (Key keyrole, SerialiseAsRawBytes (SigningKey keyrole))


Usually we indent the ::

Jimbo4350 · 2022-07-06T20:21:56Z

cardano-api/src/Cardano/Api/Key.hs

+  -> IO (SigningKey keyrole)
+generateInsecureSigningKey keytype = do
+  g <- Random.getStdGen
+  let (bs, _) = Random.genByteString (fromIntegral (deterministicSigningKeySeedSize keytype)) g


Can we ditch a pair of the parentheses with $?

Jimbo4350 · 2022-07-06T20:22:57Z

cardano-api/src/Cardano/Api/Key.hs

+  let (bs, _) = Random.genByteString (fromIntegral (deterministicSigningKeySeedSize keytype)) g
+  case deserialiseFromRawBytes (AsSigningKey keytype) bs of
+    Just key -> return key
+    Nothing -> error "Unable to generate insecure key"


A better error message would be "generateInsecureSigningKey: Unable to generate insecure key"

Jimbo4350 · 2022-07-06T20:23:21Z

cardano-api/src/Cardano/Api/TxBody.hs

-makeShelleyTransactionBody :: ()
-  => ShelleyBasedEra era
+makeShelleyTransactionBody ::
+     ShelleyBasedEra era


Why not indent the ::?

Moved the ::

Jimbo4350 · 2022-07-06T20:24:46Z

cardano-cli/src/Cardano/CLI/Shelley/Key.hs

@@ -481,3 +483,8 @@ readVerificationKeyOrHashOrTextEnvFile asType verKeyOrHashOrFile =
      eitherVk <- readVerificationKeyOrTextEnvFile asType vkOrFile
      pure (verificationKeyHash <$> eitherVk)
    VerificationKeyHash vkHash -> pure (Right vkHash)
+
+generatePaymentKeys :: Key keyrole => AsType keyrole -> IO (VerificationKey keyrole, SigningKey keyrole)


So these may not necessarily be payment keys because we can vary the keyrole. generateKeyPair might be a better name.

Jimbo4350 · 2022-07-06T20:25:39Z

cardano-api/src/Cardano/Api/KeysShelley.hs

-    newtype VerificationKey StakeKey =
-        StakeVerificationKey (Shelley.VKey Shelley.Staking StandardCrypto)
+    newtype VerificationKey StakeKey = StakeVerificationKey
+      { unStakeVerificationKey :: Shelley.VKey Shelley.Staking StandardCrypto


Why this change?

I use the field accessor here: https://github.com/input-output-hk/cardano-node/pull/4021/files#r915507713

In general, I am always in favour of added an accessor to a newtype. If its there and no one uses it, its not a big deal (especially with a totally un-ambiguous name like this), but when its needed having it already there is a huge win IMO.

Jimbo4350

A few comments but nothing major

Jimbo4350 · 2022-07-06T20:27:01Z

cardano-cli/src/Cardano/CLI/Shelley/Run/Address.hs

    AddressKeyHash vkf mOFp -> runAddressKeyHash vkf mOFp
    AddressBuild paymentVerifier mbStakeVerifier nw mOutFp -> runAddressBuild paymentVerifier mbStakeVerifier nw mOutFp
    AddressBuildMultiSig sFp nId mOutFp -> runAddressBuildScript sFp nId mOutFp
    AddressInfo txt mOFp -> firstExceptT ShelleyAddressCmdAddressInfoError $ runAddressInfo txt mOFp

-runAddressKeyGen :: AddressKeyType
+runAddressKeyGenToFile :: AddressKeyType


indent/alignment?

Moved the ::

Jimbo4350 · 2022-07-06T20:27:31Z

cardano-cli/src/Cardano/CLI/Shelley/Run/Address.hs

+  AddressKeyShelleyExtended -> generateAndWriteKeyFiles AsPaymentExtendedKey  vkf skf
+  AddressKeyByron           -> generateAndWriteKeyFiles AsByronKey            vkf skf
+
+generateAndWriteKeyFiles :: Key keyrole


indent/alignment of ::?

Moved the ::

Jimbo4350 · 2022-07-06T20:27:57Z

cardano-cli/src/Cardano/CLI/Shelley/Run/Address.hs

+generateAndWriteKeyFiles asType vkf skf = do
+  uncurry (writePaymentKeyFiles vkf skf) =<< liftIO (generatePaymentKeys asType)
+
+writePaymentKeyFiles :: Key keyrole


indent/alignment of ::?

Moved the ::

Jimbo4350 · 2022-07-06T21:18:29Z

cardano-cli/src/Cardano/CLI/Shelley/Run/Genesis.hs

+          start genDlgs mNonDlgAmount (length nonDelegAddrs) nonDelegAddrs stakePools stake
+          stDlgAmount numDelegations delegAddrs stuffedUtxoAddrs template
+
+  -- shelleyGenesis contains lazy loaded data, so using lazyToJson to serialise to avoid


We should be able to delete this comment

Jimbo4350 · 2022-07-06T21:19:48Z

cardano-cli/src/Cardano/CLI/Shelley/Run/Genesis.hs

-    StakeVerificationKey stakeVK <- firstExceptT ShelleyGenesisCmdTextEnvReadFileError
-      . newExceptT
-      $ readFileTextEnvelope (AsVerificationKey AsStakeKey) stakeVKF
+computeInsecureDelegation ::


We should add a comment here to indicate that the keys are not cryptographically generated and we don't care because this is for testing purposes. computeInsecureDelegation is a strange name. Why not generateTestDelegation or something to that effect?

Jimbo4350 · 2022-07-06T21:23:09Z

cardano-cli/src/Cardano/CLI/Shelley/Run/Genesis.hs

@@ -1229,4 +1282,3 @@ readAlonzoGenesis fpath = do
  lbs <- handleIOExceptT (ShelleyGenesisCmdGenesisFileError . FileIOError fpath) $ LBS.readFile fpath
  firstExceptT (ShelleyGenesisCmdAesonDecodeError fpath . Text.pack)
    . hoistEither $ Aeson.eitherDecode' lbs
-


Is your editor deleting newlines at the end of the file?

I don't think so.

Jimbo4350 · 2022-07-06T21:23:24Z

cardano-cli/src/Cardano/CLI/Shelley/Run/StakeAddress.hs

-    skeyDesc, vkeyDesc :: TextEnvelopeDescr
-    skeyDesc = "Stake Signing Key"
-    vkeyDesc = "Stake Verification Key"
+runStakeAddressKeyGenToFile ::


Indent/alignment of ::?

Moved the ::

Jimbo4350 · 2022-07-06T21:25:15Z

cardano-cli/src/Cardano/CLI/Shelley/Run/Genesis.hs

+    -- Genesis staking: pools/delegation map & delegated initial UTxO spec:
+    -> [(Ledger.KeyHash 'Ledger.StakePool StandardCrypto, Ledger.PoolParams StandardCrypto)]
+    -> [(Ledger.KeyHash 'Ledger.Staking StandardCrypto, Ledger.KeyHash 'Ledger.StakePool StandardCrypto)]
+    -> Lovelace


Can we add haddocks for these parameters?

I added the haddocks

newhoggy · 2022-07-07T06:35:00Z

cardano-cli/src/Cardano/CLI/Shelley/Run/Genesis.hs

      { dInitialUtxoAddr = shelleyAddressInEra initialUtxoAddr
-      , dDelegStaking = Ledger.hashKey stakeVK
+      , dDelegStaking = Ledger.hashKey (unStakeVerificationKey stakeVK)


Use of unStakeVerificationKey

Comments addressed

lehins

👍

newhoggy · 2022-07-08T12:11:42Z

bors r+

iohk-bors · 2022-07-08T12:40:11Z

Merge conflict.

newhoggy · 2022-07-11T00:37:30Z

bors r+

4021: Reduce memory usage of create staked command r=newhoggy a=newhoggy The problem arises we build a very large in-memory JSON value and then write it out as `genesis.json`. The amount of memory used by the in-memory JSON value can be so large as to use up all the memory. This PR reduces the severity of the memory usage in two ways. 1. It introduces a `ListMap` type, which is almost like `Map`, but has a `[(k, v)]` as its internal representation. This avoids the serialisation cost of constructing a map only to convert it back into a list. 2. Uses Lazy IO so that generated stuffed utxos are created on demand rather than upfront an all in memory. 3. Introduces a new `LazyToJson` type class which doesn't have the memory retention problems of `aeson` library. 4. For evaluation of fields to `WHNF` to allow parent object to be GCed which allows large fields that have already be serialised to be collected as well. 5. Writes delegations to a single `delegations.jsonl` file which is a newline delimited JSON file. This file is streamed multiple times so that generation of the `genesis.json` file does not retain memory unnecessarily. This PR also changes the command to no longer generate payment keys and stake keys. If we want to have the ability optionally output these files, there is additional work to do. Addresses #3938 Co-authored-by: John Ky <john.ky@iohk.io>

iohk-bors · 2022-07-11T01:42:46Z

This PR was included in a batch that successfully built, but then failed to merge into master. It will not be retried.

Additional information:

{"message":"Waiting on code owner review from JaredCorduan, Jimbo4350, dcoutts, and/or erikd.","documentation_url":"https://docs.github.com/articles/about-protected-branches"}

deepfire · 2022-07-11T02:51:55Z

bors r+

iohk-bors · 2022-07-11T03:33:43Z

Build succeeded:

newhoggy changed the title ~~Newhoggy/reduce memory usage of create staked command~~ Reduce memory usage of create staked command Jun 9, 2022

newhoggy force-pushed the newhoggy/reduce-memory-usage-of-create-staked-command branch from 1d7c997 to 60eae8c Compare June 9, 2022 06:39

newhoggy marked this pull request as ready for review June 9, 2022 06:40

newhoggy requested review from dcoutts, Jimbo4350, erikd and JaredCorduan as code owners June 9, 2022 06:40

newhoggy force-pushed the newhoggy/reduce-memory-usage-of-create-staked-command branch 2 times, most recently from f817ee4 to 7a72ec1 Compare June 9, 2022 06:44

newhoggy commented Jun 9, 2022

View reviewed changes

cardano-cli/src/Cardano/CLI/Shelley/Run/Genesis/ListMap.hs Outdated Show resolved Hide resolved

newhoggy force-pushed the newhoggy/reduce-memory-usage-of-create-staked-command branch 6 times, most recently from 661ba47 to 2f68910 Compare June 13, 2022 11:38

newhoggy force-pushed the newhoggy/reduce-memory-usage-of-create-staked-command branch 4 times, most recently from cfca52c to 3c8c31d Compare June 14, 2022 05:21

newhoggy requested a review from deepfire June 14, 2022 07:43

Jimbo4350 requested changes Jun 14, 2022

View reviewed changes

Jimbo4350 previously requested changes Jun 14, 2022

View reviewed changes

newhoggy force-pushed the newhoggy/reduce-memory-usage-of-create-staked-command branch from 3c8c31d to d2abe0b Compare June 14, 2022 21:23

newhoggy commented Jun 15, 2022

View reviewed changes

cardano-cli/src/Cardano/CLI/Shelley/Run/Genesis/Types.hs Outdated Show resolved Hide resolved

newhoggy requested a review from Jimbo4350 June 15, 2022 13:43

Jimbo4350 reviewed Jul 6, 2022

View reviewed changes

Jimbo4350 previously requested changes Jul 6, 2022

View reviewed changes

newhoggy commented Jul 7, 2022

View reviewed changes

newhoggy force-pushed the newhoggy/reduce-memory-usage-of-create-staked-command branch 3 times, most recently from 46db29c to 5be4f73 Compare July 7, 2022 13:16

newhoggy requested review from Jimbo4350 and lehins and removed request for Jimbo4350 July 7, 2022 13:21

lehins approved these changes Jul 7, 2022

View reviewed changes

newhoggy added 2 commits July 11, 2022 10:21

Fix unresolved conflicts in traces_doc_generated.md

f26c734

Reduce memory usage of the create-staked command using lazy IO.

cc24684

newhoggy force-pushed the newhoggy/reduce-memory-usage-of-create-staked-command branch from 5be4f73 to cc24684 Compare July 11, 2022 00:29

erikd approved these changes Jul 11, 2022

View reviewed changes

iohk-bors bot merged commit ca78153 into master Jul 11, 2022

iohk-bors bot deleted the newhoggy/reduce-memory-usage-of-create-staked-command branch July 11, 2022 03:33

CarlosLopezDeLara mentioned this pull request Jun 1, 2023

[BUG] - CLI: genesis create-staked no longer creates stake delegator keys #5286

Closed

This was referenced Nov 30, 2023

Add create-testnet-data command IntersectMBO/cardano-cli#488

Merged

Follow-ups on create-testnet-data v1 IntersectMBO/cardano-cli#506

Closed

smelc mentioned this pull request Dec 8, 2023

create-testnet-data: Make --stake-delegators write credentials to disk. Keep current behavior with new --transient-stake-delegators. IntersectMBO/cardano-cli#512

Merged

3 tasks

smelc mentioned this pull request Jan 3, 2024

[BUG] - mkfiles in babbage creates empty stake-delegator-keys #5586

Open

		@@ -67,6 +69,16 @@ generateSigningKey keytype = do
		seedSize = deterministicSigningKeySeedSize keytype


		generateInsecureSigningKey :: (Key keyrole, SerialiseAsRawBytes (SigningKey keyrole))

Reduce memory usage of create staked command #4021

Reduce memory usage of create staked command #4021

Conversation

newhoggy commented Jun 9, 2022 • edited Loading

newhoggy commented Jun 13, 2022

newhoggy commented Jun 14, 2022

Jimbo4350 commented Jun 14, 2022

Jimbo4350 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

newhoggy Jun 14, 2022 • edited Loading

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

Jimbo4350 Jun 14, 2022 • edited Loading

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

newhoggy Jun 15, 2022 • edited Loading

Choose a reason for hiding this comment

Jimbo4350 Jul 6, 2022 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jimbo4350 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lehins left a comment

Choose a reason for hiding this comment

newhoggy commented Jul 8, 2022

iohk-bors bot commented Jul 8, 2022

newhoggy commented Jul 11, 2022

iohk-bors bot commented Jul 11, 2022

deepfire commented Jul 11, 2022

iohk-bors bot commented Jul 11, 2022

newhoggy commented Jun 9, 2022 •

edited

Loading

Jimbo4350 left a comment •

edited

Loading

newhoggy Jun 14, 2022 •

edited

Loading

Jimbo4350 Jun 14, 2022 •

edited

Loading

newhoggy Jun 15, 2022 •

edited

Loading

Jimbo4350 Jul 6, 2022 •

edited

Loading