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.

Sign up for GitHub

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

Jump to bottom

Make the overlay schedule abstract #1742

Merged
merged 3 commits into from
Aug 17, 2020
Merged

Make the overlay schedule abstract #1742

merged 3 commits into from
Aug 17, 2020

Conversation

mrBliss
Copy link
Contributor

@mrBliss mrBliss commented Jul 31, 2020 •
edited

This will allow us to replace its implementation/representation by something
more compact and efficient.

On mainnet, the epoch size is 432,000, which means we're creating a map of
nearly half a million of entries just to implement a spaced round-robin scheme.

When writing ledger snapshots, we're also writing this map, albeit in a slightly
more compact representation (see compactOverlaySchedule), to disk.

Note that the size of this map will shrink when d goes down.

It should be enough to keep the arguments passed to overlayScheduleHelper in
memory and implement the current API of OverlaySchedule as a function instead
of a mapping. Similarly for serialising an OverlaySchedule, just de/serialise
these arguments.

To test this approach, the function-based implementation can be compared against
the current map-based implementation (e.g., by implementing
overlayScheduleToMap).

@mrBliss
Copy link
Contributor Author

mrBliss commented Jul 31, 2020

This is just a first step, making the way for the actual optimisation. I'm on vacation next week, so I just wanted to open this PR so it's not forgotten about.

It would be good if somebody from the ledger team takes this over.

@nc6 nc6 self-requested a review August 5, 2020 08:57
@mrBliss mrBliss mentioned this pull request Aug 12, 2020
Closed
@mrBliss @nc6
Make the overlay schedule abstract
e5ffba5 
This will allow us to replace its implementation/representation by something
more compact and efficient.

On mainnet, the epoch size is 432,000, which means we're creating a map of
nearly half a million of entries just to implement a spaced round-robin scheme.

When writing ledger snapshots, we're also writing this map, albeit in a slightly
more compact representation (see `compactOverlaySchedule`), to disk.

Note that the size of this map will shrink when `d` goes down.

It should be enough to keep the arguments passed to `overlayScheduleHelper` in
memory and implement the current API of `OverlaySchedule` as a function instead
of a mapping. Similarly for serialising an `OverlaySchedule`, just de/serialise
these arguments.

To test this approach, the function-based implementation can be compared against
the current map-based implementation (e.g., by implementing
`overlayScheduleToMap`).
@nc6 nc6 self-assigned this Aug 17, 2020
@nc6 nc6 marked this pull request as ready for review August 17, 2020 13:26
mrBliss
mrBliss commented Aug 17, 2020
View reviewed changes
Copy link
Contributor Author

@mrBliss mrBliss left a comment

Choose a reason for hiding this comment

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

Nice, good to move it to a separate module 👍

shelley/chain-and-ledger/executable-spec/src/Shelley/Spec/Ledger/OverlaySchedule.hs Outdated
Comment on lines 20 to 21
-- * OBftSlot (for testing)
OBftSlot (..),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not just for testing, it's mentioned in the public API, i.e., lookupInOverlaySchedule.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, my search didn't spot it, then I realised I had a broken search...

mrBliss
mrBliss commented Aug 17, 2020
View reviewed changes
shelley/chain-and-ledger/executable-spec/src/Shelley/Spec/Ledger/STS/Bbody.hs Outdated
@@ -3,6 +3,7 @@
{-# LANGUAGE FlexibleContexts #-}
{-# LANGUAGE FlexibleInstances #-}
{-# LANGUAGE MultiParamTypeClasses #-}
{-# LANGUAGE RankNTypes #-}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably not needed.

shelley/chain-and-ledger/executable-spec/src/Shelley/Spec/Ledger/OverlaySchedule.hs
Comment on lines +85 to +84
emptyOverlaySchedule :: OverlaySchedule crypto
emptyOverlaySchedule = OverlaySchedule Map.empty
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we intend this API to be qualified when imported, we could have shorter names like empty, lookup, isEmpty, new/create/..., toMap, compact, decompact, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, considered that but couldn't be bothered to change things around yet...

JaredCorduan
JaredCorduan approved these changes Aug 17, 2020
View reviewed changes
Copy link
Contributor

@JaredCorduan JaredCorduan left a comment

Choose a reason for hiding this comment

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

LGTM

nc6
nc6 approved these changes Aug 17, 2020
View reviewed changes
shelley/chain-and-ledger/executable-spec/src/Shelley/Spec/Ledger/API/Wallet.hs Outdated
@@ -137,7 +138,7 @@ getLeaderSchedule globals ss cds poolHash key = Set.filter isLeader epochSlots
where
isLeader slotNo =
let y = VRF.evalCertified () (mkSeed seedL slotNo epochNonce) key
in Map.notMember slotNo overlaySched
in isNothing (lookupInOverlaySchedule slotNo overlaySched)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe better to use not $ isOverlaySlot now

shelley/chain-and-ledger/executable-spec/src/Shelley/Spec/Ledger/OverlaySchedule.hs Outdated
Comment on lines 20 to 21
-- * OBftSlot (for testing)
OBftSlot (..),
Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, my search didn't spot it, then I realised I had a broken search...

shelley/chain-and-ledger/executable-spec/src/Shelley/Spec/Ledger/STS/Bbody.hs Outdated
@@ -3,6 +3,7 @@
{-# LANGUAGE FlexibleContexts #-}
{-# LANGUAGE FlexibleInstances #-}
{-# LANGUAGE MultiParamTypeClasses #-}
{-# LANGUAGE RankNTypes #-}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed

...ain-and-ledger/shelley-spec-ledger-test/test/Test/Shelley/Spec/Ledger/Examples/Federation.hs
@@ -129,7 +126,12 @@ coreNodeKeysBySchedule ::
AllIssuerKeys c 'GenesisDelegate
coreNodeKeysBySchedule = coreNodeKeysForSlot . fullOSched
where
fullOSched pp = Map.unions $ [overlayScheduleFor e pp | e <- [0 .. 10]]
Copy link
Contributor

Choose a reason for hiding this comment

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

We can probably do this a better way.

@nc6
Move 'OverlaySchedule' into its own module.
08a2bca 
'LedgerState' is already a giant grab-bag of functionality. This is a
good excuse to extract some of it.
@nc6
Avoid going through OverlaySlots.
1a5f10e 
This PR now collects the `OverlaySchedule` directly in `BBodyEnv`,
removing the need for overlaySlots and hopefully avoiding an expensive
call to `Map.keysSet`. It should be functionally equivalent to #1775
whilst allowing us to optimise further in making the overlay schedule
non-explicit.
@nc6 nc6 merged commit 736c929 into master Aug 17, 2020
@nc6 nc6 deleted the mrBliss/overlay-schedule branch August 17, 2020 14:21
@mrBliss mrBliss mentioned this pull request Aug 17, 2020
Merged
mrBliss added a commit to IntersectMBO/cardano-node that referenced this pull request Aug 18, 2020
@mrBliss
Update dependencies
511c40d 
Highlights:
* IntersectMBO/cardano-ledger#1784
* IntersectMBO/cardano-ledger#1785
* IntersectMBO/cardano-ledger#1779
* IntersectMBO/cardano-ledger#1785
* IntersectMBO/cardano-ledger#1742
* IntersectMBO/ouroboros-network#2512

* Remove unused source-repository-package cardano-sl-x509 (this package
  required the `ip < 1.5` constraint which conflicted with the
  `primitive >= 0.7.1.0` lower bound in `cardano-ledger-specs`).
* Remove unused http-client dependency from stack.yaml
* Remove duplicate from cabal.project
iohk-bors bot added a commit to IntersectMBO/cardano-node that referenced this pull request Aug 18, 2020
@iohk-bors @mrBliss @disassembler
Merge #1696
dddb08b 
1696: Update dependencies r=mrBliss a=mrBliss

Highlights:
* IntersectMBO/cardano-ledger#1784
* IntersectMBO/cardano-ledger#1785
* IntersectMBO/cardano-ledger#1779
* IntersectMBO/cardano-ledger#1785
* IntersectMBO/cardano-ledger#1742
* IntersectMBO/ouroboros-network#2512

Co-authored-by: Thomas Winant <thomas@well-typed.com>
Co-authored-by: Samuel Leathers <samuel.leathers@iohk.io>
newhoggy pushed a commit to IntersectMBO/cardano-cli that referenced this pull request May 24, 2023
@iohk-bors @mrBliss @disassembler
Merge #1696
ae9aecf 
1696: Update dependencies r=mrBliss a=mrBliss

Highlights:
* IntersectMBO/cardano-ledger#1784
* IntersectMBO/cardano-ledger#1785
* IntersectMBO/cardano-ledger#1779
* IntersectMBO/cardano-ledger#1785
* IntersectMBO/cardano-ledger#1742
* IntersectMBO/ouroboros-network#2512

Co-authored-by: Thomas Winant <thomas@well-typed.com>
Co-authored-by: Samuel Leathers <samuel.leathers@iohk.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nc6 nc6 nc6 approved these changes

@JaredCorduan JaredCorduan JaredCorduan approved these changes

@uroboros uroboros Awaiting requested review from uroboros

Assignees

@nc6 nc6

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mrBliss @JaredCorduan @nc6