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

Shelley: avoid expensive equality check of the LedgerView #2512

Merged
merged 1 commit into from
Aug 13, 2020

Conversation

mrBliss
Copy link
Contributor

@mrBliss mrBliss commented Aug 12, 2020

We check whether the LedgerView changed after (re)applying each block, to see
whether we should store a snapshot of the old one. The LedgerView includes the
overlay schedule (a map with 432,000 entries!) and the pool distribution (a map
with an entry for each stakepool). Comparing these two data structures for
equality is very costly. Since the overlay schedule and the pool distribution
only change when transitioning to a new epoch, we can use the epoch number as a
proxy for the equality check of those two data structures.

The other two data structures, i.e., lvProtParams and lvGenDelegs, are small
and can be checked time.

@mrBliss mrBliss added the consensus issues related to ouroboros-consensus label Aug 12, 2020
@mrBliss mrBliss requested a review from nc6 August 12, 2020 19:20
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.

After enabling profiling-detail: all-functions in my cabal.project.local, my profiling reports became much more complete (no more fn_3 (indiv) taking the majority of the time 🙄). I found that this equality was a major time/alloc sink.

I think this, in combination with IntersectMBO/cardano-ledger#1775 will improve validation speed significantly. For example, my profile of db-validator on the first Shelley era went from ~3 min to 20s (that's with PR 1775 applied).

Comment on lines 288 to 289
= SL.lvProtParams oldLedgerView /= SL.lvProtParams newLedgerView ||
SL.lvGenDelegs oldLedgerView /= SL.lvGenDelegs newLedgerView
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nc6 Do these two values ever change within an epoch or also only at the epoch boundary?

If the latter, then we can improve/simplify things even further.

Copy link
Contributor

Choose a reason for hiding this comment

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

Protocol parameters only change at an epoch boundary. Genesis delegates may change within an epoch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, updated. Thanks!

Copy link
Contributor

@nc6 nc6 left a comment

Choose a reason for hiding this comment

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

Could also remove the protocol parameters check.

We check whether the `LedgerView` changed after (re)applying each block, to see
whether we should store a snapshot of the old one. The `LedgerView` includes the
overlay schedule (a map with 432,000 entries!) and the pool distribution (a map
with an entry for each stakepool). Comparing these two data structures for
equality is very costly. Since the overlay schedule and the pool distribution
only change when transitioning to a new epoch, we can use the epoch number as a
proxy for the equality check of those two data structures. The protocol
parameters can also only change at the epoch boundary. The genesis delegates may
change within an epoch.
@mrBliss mrBliss force-pushed the mrBliss/avoid-ledgerview-equality branch from cdbd5e9 to 8e7b5e2 Compare August 13, 2020 07:02
@mrBliss
Copy link
Contributor Author

mrBliss commented Aug 13, 2020

bors merge

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 13, 2020

@iohk-bors iohk-bors bot merged commit 7454eac into master Aug 13, 2020
@iohk-bors iohk-bors bot deleted the mrBliss/avoid-ledgerview-equality branch August 13, 2020 07:31
erikd pushed a commit to IntersectMBO/cardano-node that referenced this pull request Aug 18, 2020
Highlights:
* IntersectMBO/cardano-ledger#1784
* IntersectMBO/cardano-ledger#1785
* IntersectMBO/cardano-ledger#1779
* 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
mrBliss added a commit to IntersectMBO/cardano-node that referenced this pull request Aug 18, 2020
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
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>
@mrBliss
Copy link
Contributor Author

mrBliss commented Aug 21, 2020

Note to myself: when we run into a similar situation again, i.e., having to check the equality of huge data structures, and there isn't a cheap proxy like in this case, there's always: https://hackage.haskell.org/package/containers-0.6.3.1/docs/src/Utils.Containers.Internal.PtrEquality.html#ptrEq 😄

newhoggy pushed a commit to IntersectMBO/cardano-cli that referenced this pull request May 24, 2023
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
Labels
consensus issues related to ouroboros-consensus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants