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

soroban-rpc: Add writethrough cache for Config ledger entries #837

Merged
merged 8 commits into from
Aug 7, 2023

Conversation

2opremio
Copy link
Contributor

@2opremio 2opremio commented Aug 3, 2023

After #692 I noticed that the DB access to config ledger entries is a big part of the preflight computation.

This PR adds a writethrough cache for config ledger entries.

Before the cache:

goos: darwin
goarch: arm64
pkg: github.com/stellar/soroban-tools/cmd/soroban-rpc/internal/preflight
BenchmarkGetPreflight
BenchmarkGetPreflight/In-memory_storage
BenchmarkGetPreflight/In-memory_storage-12         	    6112	    193728 ns/op
BenchmarkGetPreflight/DB_storage
BenchmarkGetPreflight/DB_storage-12                	    2766	    430002 ns/op
PASS

After adding the cache:

goos: darwin
goarch: arm64
pkg: github.com/stellar/soroban-tools/cmd/soroban-rpc/internal/preflight
BenchmarkGetPreflight
BenchmarkGetPreflight/In-memory_storage
BenchmarkGetPreflight/In-memory_storage-12         	    6169	    192616 ns/op
BenchmarkGetPreflight/DB_storage
BenchmarkGetPreflight/DB_storage-12                	    3717	    324078 ns/op
PASS

I am not sure it's worth the extra complexity though, considering it brings us down from 0.43 ms to 0.32 ms per (simple) preflight.

@2opremio 2opremio requested a review from tamirms August 3, 2023 22:07
@tamirms
Copy link
Contributor

tamirms commented Aug 4, 2023

@2opremio doesn't the cache keep growing indefinitely? I would be worried about the memory consumption because I think the cache would just keep growing as long as soroban rpc is up. The other concern is that the cache is copied whenever a new read transaction is created. So if the cache is already large lots of concurrent requests could spike memory usage even more.

@2opremio
Copy link
Contributor Author

2opremio commented Aug 4, 2023

doesn't the cache keep growing indefinitely?

It doesn't, because we only store the config ledger entries

The other concern is that the cache is copied whenever a new read transaction is created

Valid concern, but the number of possible ledger entries is small (~15)

@2opremio
Copy link
Contributor Author

2opremio commented Aug 4, 2023

@tamirms I feel likeI didn't supply enough context. We only cache config ledger entries. The preflight computation needs config parameters from the network and it fetches them (around 10 ledger entries) every time it runs.

The number of config ledger entries is limited (around 15 of them, see https://github.com/stellar/stellar-xdr/blob/next/Stellar-contract-config-setting.x#L200-L217 ) so we don't have to worry about eviction.

@2opremio
Copy link
Contributor Author

2opremio commented Aug 4, 2023

@tamirms PTAL

@2opremio 2opremio merged commit 8eacf0c into stellar:main Aug 7, 2023
@2opremio 2opremio deleted the config-entries-cache branch August 7, 2023 09:28
kalepail pushed a commit to kalepail/soroban-tools that referenced this pull request Aug 10, 2023
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