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

optimize TICKF #3209

Merged
merged 4 commits into from
Dec 16, 2022
Merged

optimize TICKF #3209

merged 4 commits into from
Dec 16, 2022

Conversation

JaredCorduan
Copy link
Contributor

@JaredCorduan JaredCorduan commented Dec 12, 2022

Description

This PR optimizes the TICKF transition, particularly for the problematically slow computations on the epoch boundary. See the ADR in this PR for more context. See also #3034.

This PR consists of four commits: one for each of the two optimizations mentioned in the ADR, one for the ADR, and one to fix a broken link in the PR template.

I did not update the changelog since all the changes are invisible outside of this code base.

replaces #3140 and #3141

Checklist

  • Commit sequence broadly makes sense
  • Commits have useful messages
  • New tests are added if needed and existing tests are updated
  • Any changes are noted in the changelog
  • Code is formatted with ormolu (which can be run with scripts/ormolise.sh
  • Self-reviewed the diff

@JaredCorduan JaredCorduan requested a review from a team as a code owner December 12, 2022 22:25
@JaredCorduan JaredCorduan changed the title Jc/memoize pool distr optimize TICKF Dec 12, 2022
@JaredCorduan JaredCorduan force-pushed the jc/memoize-pool-distr branch 2 times, most recently from ec56a75 to 3f93e3e Compare December 13, 2022 02:47
Copy link
Contributor

@aniketd aniketd left a comment

Choose a reason for hiding this comment

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

I understand this a tad-bit better after reviewing this PR. Thank you! 🙌

Copy link
Collaborator

@lehins lehins left a comment

Choose a reason for hiding this comment

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

This makes sense. Just a few minor suggestions

Jared Corduan added 4 commits December 15, 2022 19:12
Calculating the stake pool distribution in the NEWEPOCH rule is
problematic due to its role in the consensus layer. When a node checks
to see if it is the leader for a slot crossing the epoch boundary, the
node re-computes the distribution every second, and the
calculation takes over a second. As a stop gap measure, we memoize the
computation by creating a thunk for it at the moment when all the data
is available, which happens to be an entire epoch before it is needed.
In particular, we create the thunk inside the `SnapShots` record in the
SNAP rule.
TICKF now only includes the logic needed to compute the same LedgerView
as TICK.
Copy link
Contributor

@aniketd aniketd left a comment

Choose a reason for hiding this comment

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

Awesome stuff! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants