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

Improve startup by eliminating unnecessary migration ranges #282

Merged
merged 8 commits into from
Sep 4, 2024

Conversation

Shaptic
Copy link
Contributor

@Shaptic Shaptic commented Aug 29, 2024

What

Speed up startup time in "already migrated" cases by eliminating unnecessary ledger range traversals.

Specifically, this includes the following changes:

  • Fee stats windows cannot exceed the history retention window, as this doesn't make sense.
  • Migrations that are already applied are not added to the list of "multi-migrations".
  • Fee stats window building conforms to the migration interface to simplify code.
  • LedgerSeqRange has been refactored to always use a value reference.

As a result, if all migrations have occurred, the traversal only occurs over the fee window.

This reduces startup times in a fully-loaded (24hr) database that needs no migrations from ~1m40s to ~1s.

Why

Previously, startup would take a significant amount of time doing nothing because it would loop over a large ledger range and check whether or not a migration was necessary for that ledger. If migrations were already done, it would move on to the next ledger.

In this version, we instead appropriately calculate the ledger range to traverse by only expanding the ledger range if a migration has not already completed.

Known limitations

n/a, tested at length locally

@Shaptic Shaptic requested review from 2opremio, aditya1702 and a team August 29, 2024 19:32
Copy link
Contributor

@aditya1702 aditya1702 left a comment

Choose a reason for hiding this comment

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

Just one nit comment. Otherwise looks good.

cmd/soroban-rpc/internal/daemon/daemon.go Outdated Show resolved Hide resolved
cmd/soroban-rpc/internal/db/migration.go Show resolved Hide resolved
cmd/soroban-rpc/internal/daemon/daemon.go Outdated Show resolved Hide resolved
@Shaptic Shaptic requested a review from tamirms August 29, 2024 22:02
@tamirms
Copy link
Contributor

tamirms commented Aug 30, 2024

@Shaptic the optimization is great! In summary, I have 3 pieces of feedback:

  1. LedgerSeqRange should be an immutable type
  2. There is no need for db.BuildMigrations() to return a LedgerSeqRange because it already has a ApplicableRange() function which can be used to obtain the applicable ledger range
  3. We can implement a Migration instance for the fee stats and then append it to the MultiMigration returned by db.BuildMigrations() which would allow us to remove the special case handling for fee stats in daemon.go

@Shaptic Shaptic force-pushed the better-migration-ranges branch from 441c60c to baced2b Compare September 4, 2024 18:58
Copy link
Contributor

@tamirms tamirms left a comment

Choose a reason for hiding this comment

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

looks great!

@Shaptic Shaptic force-pushed the better-migration-ranges branch from baced2b to 572116a Compare September 4, 2024 19:13
@Shaptic Shaptic enabled auto-merge (squash) September 4, 2024 19:20
@Shaptic Shaptic merged commit e110732 into main Sep 4, 2024
19 checks passed
@Shaptic Shaptic deleted the better-migration-ranges branch September 4, 2024 19:27
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