New query GetPoolDistr #3932

newhoggy · 2022-08-01T11:15:40Z

Description

This introduce a new query GetPoolDistr which extracts the stake snapshot. This query is required to improve the CPU and memory efficiency of the query leadership-schedule command in the CLI.

Checklist

Branch
- Commit sequence broadly makes sense
- Commits have useful messages
- New tests are added if needed and existing tests are updated
- If this branch changes Consensus and has any consequences for downstream repositories or end users, said changes must be documented in interface-CHANGELOG.md
- If this branch changes Network and has any consequences for downstream repositories or end users, said changes must be documented in interface-CHANGELOG.md
- If serialization changes, user-facing consequences (e.g. replay from genesis) are confirmed to be intentional.
Pull Request
- Self-reviewed the diff
- Useful pull request description at least containing the following information:
  - What does this PR change?
  - Why these changes were needed?
  - How does this affect downstream repositories and/or end-users?
  - Which ticket does this PR close (if any)? If it does, is it linked?
- Reviewer requested

`cardano-api` changes

There will be a new GetPoolDistr which uses a lot less CPU and memory:

GetPoolDistr
    :: SL.KeyHash 'SL.StakePool (EraCrypto era)
    -> BlockQuery (ShelleyBlock proto era)
                  (SL.PoolDistr (EraCrypto era))

This query takes a set of pool ids so it is possible to query multiple pools at once.

`cardano-cli` changes

A existing command query leadership-schedule will be be functionally unchanged, but use much less CPU and memory.

nfrisby

I'm Requesting Changes, mostly to ensure that we have a good answer to whether calculatePoolDistr is prohibitively expensive.

ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/Query.hs

nfrisby · 2022-08-01T16:31:46Z

ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/Query.hs

@@ -273,6 +278,11 @@ instance ShelleyCompatible proto era => QueryLedger (ShelleyBlock proto era) whe
                , SL._retiring = Map.restrictKeys (SL._retiring dpsPState) poolIds
                }
            Nothing -> dpsPState
+        GetPoolDistr mPoolIds ->
+          let poolDistr = SL.calculatePoolDistr . SL._pstakeSet . SL.esSnapshots $ getEpochState st in


Is this calculatePoolDistr perhaps too expensive?

It might be, but I don't know that we have any clear requirements on what is too expensive. Note that SL._pstakeSet is one of the data structure that we anticipate being moved to disk.

This is for query leadership-schedule, so I think we have a good amount of headroom -- I wouldn't expect SPOs etc to be spamming this command (though it's easy enough to do that accidentally within a script).

We could micro-benchmark the function, and see where we are at, if we are worried.

For a general idea as to what calculatePoolDistr does:

https://github.com/input-output-hk/cardano-ledger/blob/dd4b6e31a05e3c02c6f00f0d53cf20e3b8b74468/eras/shelley/impl/src/Cardano/Ledger/Shelley/Rules/NewEpoch.hs#L204-L220

convert the very large stake distribution into an association list

un-compacting the coins in the process

lookup the stake creds in the delegation map in the process

Map.fromListWith (+) on the big association list

Map.intersectionWith on the condensed map to add in the VRF keys

Is there a way to make this cheaper if we're only interested in a subset of pool ids?

great question! The following might be faster:

Make a new version of calculatePoolDistr with signature

calculatePoolDistr' :: SnapShot crypto -> Set (KeyHash 'Staking crypto) -> PoolDistr crypto

which is identical to calculatePoolDistr except that after looking up the credential in the delegation map (see here), it also looks up the pool ID in the set of pool IDs provided by the user (and discards the result if not found).

I'd look at use cases before optimising too much. I think the two likely cases are:

all pools

a specific pool (and then repeat for multi-pool operators)

The command is likely to be used occasionally rather than frequently, I expect

ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/Query.hs

Comments addressed

newhoggy · 2022-08-10T13:27:02Z

I reverted back to use calculatePoolDistr because some refactoring in cardono-ledger blocks merging the PR otherwise. I would like to merge this version and in a future PR switch to use calculatePoolDistr' #3949

coot · 2022-08-16T10:57:01Z

ouroboros-network/src/Ouroboros/Network/NodeToClient/Version.hs

@@ -35,7 +35,7 @@ data NodeToClientVersion
    | NodeToClientV_13
    -- ^ enabled @CardanoNodeToClientVersion9@, i.e., Babbage
    | NodeToClientV_14
-    -- ^ added @GetPoolState, @GetSnapshots
+    -- ^ added @GetPoolDistr, @GetPoolState, @GetSnapshots


👍 NodeToClientV_14 hasn't been yet released. node-1.35.x comes with NodeToClientV_13 as the latest version.

nfrisby

Marcin's commented that the NTC version was correct. So that's good.

The question of calculatePoolDistr's performance remains unresolved, but I don't think it's a blocker for this PR. We'll need to assess it, though. @newhoggy can you pursue that with some measurements etc after merging this (or before, if that's convenient)?

newhoggy · 2022-08-16T23:18:14Z

Yep. I will evaluate after merging.

There is a follow up PR that improves the performance, but that's blocked on a refactoring PR.

newhoggy · 2022-08-16T23:18:25Z

bors r+

iohk-bors · 2022-08-17T00:31:45Z

Build succeeded:

newhoggy · 2022-08-17T12:44:50Z

I didn't notice much difference in CPU/memory usage of the new version of cardano-node. I think it's because query leadershipship-schedule was already very expensive to start with.

newhoggy marked this pull request as ready for review August 1, 2022 13:23

newhoggy requested review from nfrisby, dnadales and coot as code owners August 1, 2022 13:23

nfrisby previously requested changes Aug 1, 2022

View reviewed changes

newhoggy force-pushed the newhoggy/new-query-GetPoolDistr branch from 9f54955 to c521d49 Compare August 4, 2022 13:38

newhoggy requested a review from bolt12 as a code owner August 4, 2022 13:38

newhoggy force-pushed the newhoggy/new-query-GetPoolDistr branch from c521d49 to f5d6a47 Compare August 5, 2022 08:14

newhoggy requested a review from nfrisby August 5, 2022 08:14

newhoggy commented Aug 5, 2022

View reviewed changes

ouroboros-consensus-shelley/src/Ouroboros/Consensus/Shelley/Ledger/Query.hs Outdated Show resolved Hide resolved

newhoggy requested a review from JaredCorduan August 7, 2022 12:06

newhoggy force-pushed the newhoggy/new-query-GetPoolDistr branch from f5d6a47 to 5baa2fd Compare August 8, 2022 07:38

newhoggy force-pushed the newhoggy/new-query-GetPoolDistr branch 2 times, most recently from 8628e8b to 78fafbc Compare August 9, 2022 08:58

This was referenced Aug 9, 2022

High memory usage when running query stake-snapshot and others. IntersectMBO/cardano-node#4140

Closed

NODE VNEXT SCOPE IntersectMBO/cardano-node#4144

Open

Optimise query leadership schedule command IntersectMBO/cardano-node#4250

Merged

newhoggy force-pushed the newhoggy/new-query-GetPoolDistr branch from 78fafbc to 249bff8 Compare August 10, 2022 13:25

New query GetPoolDistr

16b4712

newhoggy force-pushed the newhoggy/new-query-GetPoolDistr branch from 249bff8 to 16b4712 Compare August 10, 2022 22:57

CarlosLopezDeLara mentioned this pull request Aug 12, 2022

query commands optimization (Performance high memory usage) IntersectMBO/cardano-node#4325

Closed

coot reviewed Aug 16, 2022

View reviewed changes

nfrisby approved these changes Aug 16, 2022

View reviewed changes

iohk-bors bot merged commit 15b56d1 into master Aug 17, 2022

iohk-bors bot deleted the newhoggy/new-query-GetPoolDistr branch August 17, 2022 00:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New query GetPoolDistr #3932

New query GetPoolDistr #3932

newhoggy commented Aug 1, 2022 •

edited

Loading

nfrisby left a comment

nfrisby Aug 1, 2022

JaredCorduan Aug 1, 2022

nfrisby Aug 1, 2022

JaredCorduan Aug 1, 2022

newhoggy Aug 3, 2022

JaredCorduan Aug 3, 2022

kevinhammond Aug 16, 2022

newhoggy commented Aug 10, 2022 •

edited

Loading

coot Aug 16, 2022

nfrisby left a comment

newhoggy commented Aug 16, 2022

newhoggy commented Aug 16, 2022

iohk-bors bot commented Aug 17, 2022

newhoggy commented Aug 17, 2022

New query GetPoolDistr #3932

New query GetPoolDistr #3932

Conversation

newhoggy commented Aug 1, 2022 • edited Loading

Description

Checklist

cardano-api changes

cardano-cli changes

nfrisby 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

newhoggy commented Aug 10, 2022 • edited Loading

Choose a reason for hiding this comment

nfrisby left a comment

Choose a reason for hiding this comment

newhoggy commented Aug 16, 2022

newhoggy commented Aug 16, 2022

iohk-bors bot commented Aug 17, 2022

newhoggy commented Aug 17, 2022

newhoggy commented Aug 1, 2022 •

edited

Loading

`cardano-api` changes

`cardano-cli` changes

newhoggy commented Aug 10, 2022 •

edited

Loading