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

Optimise db performance of GetLedgerRange #256

Merged
merged 46 commits into from
Sep 5, 2024

Conversation

aditya1702
Copy link
Contributor

@aditya1702 aditya1702 commented Jul 29, 2024

What

Fixes #233

  • Removes the LatestLedger key from meta table. Use GetLedgerRange function since it already contains information about the latest ledger.
  • Make the GetLedgerRange faster by caching latestLedgerSeq and latestLedgerCloseTime. This leads to a single query to get the oldest ledger and its close time.

Why

The latestLedgerSeq key can be removed since we already get that info from ledger range function. The GetLedgerRange is used a lot now so it is important to make it faster by caching the values.

Known limitations

NA

@aditya1702 aditya1702 changed the title Remove LatestLedger key from meta table [WIP] Remove LatestLedger key from meta table Jul 29, 2024
@aditya1702 aditya1702 changed the title [WIP] Remove LatestLedger key from meta table Remove LatestLedger key from meta table Aug 1, 2024
@aditya1702 aditya1702 marked this pull request as ready for review August 1, 2024 17:30
@aditya1702 aditya1702 requested review from tamirms and 2opremio August 1, 2024 17:30
@aditya1702
Copy link
Contributor Author

@2opremio I will include caching the ledger range in another PR since I am testing things with it and this PR can be merged without it.

@aditya1702 aditya1702 requested a review from Shaptic August 1, 2024 17:57
@2opremio
Copy link
Contributor

2opremio commented Aug 1, 2024

Caching the latest ledger is pretty fundamental for performance, so I would rather wait to have caching in main.

@aditya1702
Copy link
Contributor Author

Caching the latest ledger is pretty fundamental for performance, so I would rather wait to have caching in main.

@2opremio The latestLedger is still being cached - https://github.com/stellar/soroban-rpc/pull/256/files#diff-2b645f26f8eb502d1988698186482a5910b37509e606f968a3a60c927c397cfeL150. I was referencing the oldestLedger and the close times that comprise the range.

@2opremio
Copy link
Contributor

2opremio commented Aug 1, 2024

Ah ok, then the PR description and title are outdated. Can you update them?

@2opremio
Copy link
Contributor

2opremio commented Aug 1, 2024

Or do you mean that everything is cached and working except to the oldest ledger and the oldest ledger close time?

@2opremio
Copy link
Contributor

2opremio commented Aug 1, 2024

OK, I've taken a look at the code

@2opremio The latestLedger is still being cached - https://github.com/stellar/soroban-rpc/pull/256/files#diff-2b645f26f8eb502d1988698186482a5910b37509e606f968a3a60c927c397cfeL150. I was referencing the oldestLedger and the close times that comprise the range.

As I recall, this places it into the downstream cache, but doesn't really use the cache (as you can see in the code the code doesn't check the cache for the value it just writes to it). We need to be careful about using GetLedgerRange() as opposed to GetLatestLedger() here because it's more expensive.

One option is to actually read from the cache there but I think there were some subtleties preventing from doing so. Can you take a look?

@aditya1702 aditya1702 changed the title Remove LatestLedger key from meta table Optimise db performance of GetLedgerRange Aug 5, 2024
@aditya1702 aditya1702 requested a review from tamirms August 14, 2024 15:06
@2opremio
Copy link
Contributor

I am missing a migration to remove the entry from the meta table.

…-range-cache

# Conflicts:
#	cmd/soroban-rpc/internal/db/db.go
#	cmd/soroban-rpc/internal/ingest/mock_db_test.go
#	cmd/soroban-rpc/internal/ingest/service_test.go
@aditya1702 aditya1702 requested a review from 2opremio August 27, 2024 18:35
@aditya1702 aditya1702 requested a review from tamirms August 28, 2024 21:42
@aditya1702 aditya1702 requested a review from tamirms August 29, 2024 11:36
@aditya1702 aditya1702 merged commit 918c978 into stellar:main Sep 5, 2024
19 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.

db: Remove latestledger key from meta table
3 participants