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

Remove loop from elapsed slots calculation #2393

Closed
wants to merge 1 commit into from

Conversation

LucasSte
Copy link

@LucasSte LucasSte commented Aug 1, 2024

Problem

A loop that calculates the number of elapsed slots between two epochs depend on the difference between the start and the end epoch:

let slots_elapsed: u64 = (account_rent_epoch..=self.epoch)

This loop can get quite slow (>20 minutes) if the difference between them is too large.

Summary of Changes

Remove the loop and perform a constant calculation.

I wrote a big comment explaining where I got the expressions from. The math behind it is quite involved, so I included a sanity and a fuzzy test.

@LucasSte LucasSte marked this pull request as ready for review August 1, 2024 19:42
@brooksprumo
Copy link

This loop can get quite slow (>20 minutes) if the difference between them is too large.

What constitutes as "too large"?


This loop can get quite slow (>20 minutes)

Do you know what specifically causes the slowness?

@LucasSte
Copy link
Author

LucasSte commented Aug 1, 2024

This loop can get quite slow (>20 minutes) if the difference between them is too large.

What constitutes as "too large"?

The input my fuzzer generated has zero for the account_rent_epoch and 1334399889604 (this is around 2^40) for the EpochSchedule's epoch. A loop with these conditions causes a timeout.

This loop can get quite slow (>20 minutes)

Do you know what specifically causes the slowness?

Running 2^40 iterations takes a long time.

@LucasSte
Copy link
Author

LucasSte commented Aug 1, 2024

Math on a paper is different from math on a computer, because on a paper nothing overflows. I found out I wasn't handling u64::MAX cases correctly due to saturating_ operations. I have to revise the PR, so I'm marking it as a draft again.

@LucasSte LucasSte marked this pull request as draft August 1, 2024 22:09
@LucasSte
Copy link
Author

LucasSte commented Aug 2, 2024

Closing in favor of #2411.

@LucasSte LucasSte closed this Aug 2, 2024
@LucasSte LucasSte deleted the simplify-loop branch August 22, 2024 16:57
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.

2 participants