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

[Perf] Speed up the fetching of the latest block height #2515

Merged
merged 3 commits into from
Oct 22, 2024

Conversation

ljedrz
Copy link
Collaborator

@ljedrz ljedrz commented Jul 9, 2024

Another heap-profiling find applicable to the loading of the ledger; by counting the number of heights instead of deserializing them, we can avoid a large number of allocations.

The results at height 330598 are as follows:

  • total allocs -12%
  • temp allocs: -15%
  • ledger loading time: -7%

@ljedrz
Copy link
Collaborator Author

ljedrz commented Jul 9, 2024

CI run link

Signed-off-by: ljedrz <ljedrz@users.noreply.github.com>
@ljedrz ljedrz force-pushed the perf/faster_max_height branch from d1fa0c3 to 71c3806 Compare July 9, 2024 14:54
vicsn
vicsn previously approved these changes Jul 9, 2024
@vicsn
Copy link
Contributor

vicsn commented Jul 9, 2024

Can you point readers to the logic/evidence that the change does not introduce an off-by-one error?

ledger/store/src/block/mod.rs Outdated Show resolved Hide resolved
Signed-off-by: ljedrz <ljedrz@users.noreply.github.com>
@ljedrz
Copy link
Collaborator Author

ljedrz commented Jul 10, 2024

@vicsn done; also, I noticed that originally there actually would have been an issue with a height equal to u32::MAX (at which point there are arguably more pressing issues 😃), which is now fixed.

@ljedrz ljedrz requested a review from vicsn July 10, 2024 07:15
vicsn
vicsn previously approved these changes Jul 10, 2024
Signed-off-by: ljedrz <ljedrz@users.noreply.github.com>
@ljedrz
Copy link
Collaborator Author

ljedrz commented Jul 10, 2024

I just found 2 more instances of max block height lookup (without using heights, which is how I missed them before); I updated the CI branch too.

One of them will further improve ledger loading time.

@ljedrz ljedrz requested a review from vicsn July 10, 2024 10:04
Copy link
Collaborator

@zkxuerb zkxuerb left a comment

Choose a reason for hiding this comment

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

LGTM

@zosorock zosorock merged commit b118ea2 into AleoNet:staging Oct 22, 2024
75 of 83 checks passed
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.

6 participants