-
Notifications
You must be signed in to change notification settings - Fork 155
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
Delegs: optimise the rewards calculation #1779
Conversation
With this PR, IntersectMBO/ouroboros-network#2512, and #1775, the remaining hotspots in my microbenchmark (validate/reapply the first Shelley epoch) are:
|
Map.fromList | ||
[ (cred, coin) | ||
| (RewardAcnt network' cred, coin) <- Map.toList wdrls_, | ||
network' == network |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the network in wdrls_
actually differ from liftSTS $ asks networkId
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe so. The network stuff is just in here because it's needed to serialise as a reward account. So I think it's fine to completely ignore it with this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can take the network checks out, otherwise this looks great
Map.fromList | ||
[ (cred, coin) | ||
| (RewardAcnt network' cred, coin) <- Map.toList wdrls_, | ||
network' == network |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe so. The network stuff is just in here because it's needed to serialise as a reward account. So I think it's fine to completely ignore it with this change.
From my profiling report of the validation of the first Shelley epoch (excluding the epoch transition): ``` TOT INH IND 35.7 28.1 .4 Control.State.Transition.Extended liftSTS (7553) 13.0 9.8 7.8 Shelley.Spec.Ledger.STS.Delegs delegsTransition.rewards (8136) 4.4 1.2 .6 Shelley.Spec.Ledger.Address >= (8607184) 3.8 .6 .6 Shelley.Spec.Ledger.Address < (8607184) 3.2 - - Shelley.Spec.Ledger.Credential < (8607184) .8 .8 .8 Shelley.Spec.Ledger.Address mkRwdAcnt (8615225) 11.3 9.0 8.6 Shelley.Spec.Ledger.STS.Delegs delegsTransition.rewards' (8136) 2.7 .4 .3 Shelley.Spec.Ledger.Credential >= (8607184) 2.3 - - Shelley.Spec.Ledger.Credential < (8607184) ``` The conversions (`Map.mapKeys`) of the rewards accounts is very costly, and we're doing it twice. Instead, convert the `wdrls`, which is a much smaller map. After this change, `rewards` and `rewards'` are no longer hotspots (0.0 time!).
b815f4d
to
5caedcb
Compare
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
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
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>
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>
From my profiling report of the validation of the first Shelley epoch (excluding the epoch transition):
The conversions (
Map.mapKeys
) of the rewards accounts is very costly, andwe're doing it twice. Instead, convert the
wdrls
, which is a much smaller map.After this change,
rewards
andrewards'
are no longer hotspots (0.0 time!).