soroban-rpc: getLedgerEntries: query multiple ledger entries at once #896

2opremio · 2023-08-24T10:39:47Z

This improves the performance of the getLedgerEntries method and also paves the way to querying multiple entries from libpreflight (ahead of the upcoming ledger expiration changes)

2opremio · 2023-08-24T10:44:05Z

BTW, we should probably limit the number of entries requested at once.

@tsachiherman did you get to add a limit to the size of JSON requests? We should also do that.

2opremio · 2023-08-24T11:45:51Z

Uhmm .. this PR actually reduces the performance of preflighting. I should look into it before merging.

Before:

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         	    6476	    178607 ns/op
BenchmarkGetPreflight/DB_storage
BenchmarkGetPreflight/DB_storage-12                	    4984	    237336 ns/op
BenchmarkGetPreflight/DB_storage,_restarting
BenchmarkGetPreflight/DB_storage,_restarting-12    	    5192	    231442 ns/op
BenchmarkGetPreflight/DB_storage,_no_cache
BenchmarkGetPreflight/DB_storage,_no_cache-12      	    2744	    428038 ns/op
PASS

After:

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         	    5682	    213653 ns/op
BenchmarkGetPreflight/DB_storage
BenchmarkGetPreflight/DB_storage-12                	    2412	    485964 ns/op
BenchmarkGetPreflight/DB_storage,_restarting
BenchmarkGetPreflight/DB_storage,_restarting-12    	    2457	    459799 ns/op
BenchmarkGetPreflight/DB_storage,_no_cache
BenchmarkGetPreflight/DB_storage,_no_cache-12      	    1750	    678824 ns/op
PASS

2opremio · 2023-08-24T12:30:05Z

I added a small optimization leading to better times but we are still doing worse than before.

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         	    6856	    175734 ns/op
BenchmarkGetPreflight/DB_storage
BenchmarkGetPreflight/DB_storage-12                	    3008	    367869 ns/op
BenchmarkGetPreflight/DB_storage,_restarting
BenchmarkGetPreflight/DB_storage,_restarting-12    	    3010	    366539 ns/op
BenchmarkGetPreflight/DB_storage,_no_cache
BenchmarkGetPreflight/DB_storage,_no_cache-12      	    2016	    592172 ns/op
PASS

This improves performance of the getLedgerEntries and also paves the way to querying multiple entries from libpreflight.

2opremio · 2023-08-24T13:29:38Z

After #897 the numbers are much closer!

Before this PR:

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         	    6387	    170649 ns/op
BenchmarkGetPreflight/DB_storage
BenchmarkGetPreflight/DB_storage-12                	    4916	    229133 ns/op
BenchmarkGetPreflight/DB_storage,_restarting
BenchmarkGetPreflight/DB_storage,_restarting-12    	    4990	    226570 ns/op
BenchmarkGetPreflight/DB_storage,_no_cache
BenchmarkGetPreflight/DB_storage,_no_cache-12      	    2793	    424232 ns/op
PASS

After this PR:

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         	    6685	    173606 ns/op
BenchmarkGetPreflight/DB_storage
BenchmarkGetPreflight/DB_storage-12                	    4840	    242125 ns/op
BenchmarkGetPreflight/DB_storage,_restarting
BenchmarkGetPreflight/DB_storage,_restarting-12    	    4810	    242487 ns/op
BenchmarkGetPreflight/DB_storage,_no_cache
BenchmarkGetPreflight/DB_storage,_no_cache-12      	    2492	    476540 ns/op
PASS

2opremio · 2023-08-24T13:31:54Z

I think I am happy with this.

cmd/soroban-rpc/internal/db/ledgerentry.go

cmd/soroban-rpc/internal/preflight/pool.go

tsachiherman

looks great, please take a note of @tamirms 's comment.

cmd/soroban-rpc/internal/db/ledgerentry.go

cmd/soroban-rpc/internal/methods/get_ledger_entries.go

cmd/soroban-rpc/internal/preflight/pool.go

tsachiherman · 2023-08-24T15:10:16Z

BTW, we should probably limit the number of entries requested at once.

@tsachiherman did you get to add a limit to the size of JSON requests? We should also do that.

I'm not sure where that would be. But we should definitely add a limit to the number of keys that can be queried in a single getLedgerEntries call.

Adding a limit to the json size request is a bit harder to implement; we might be able to limit the http request size by adding a middleware.

2opremio · 2023-08-24T15:50:22Z

BTW, we should probably limit the number of entries requested at once.

@tsachiherman did you get to add a limit to the size of JSON requests? We should also do that.

@tsachiherman did you note this comments?

tsachiherman · 2023-08-24T16:00:57Z

BTW, we should probably limit the number of entries requested at once.
@tsachiherman did you get to add a limit to the size of JSON requests? We should also do that.

@tsachiherman did you note this comments?

I agree that we should limit the number of entries requested.

As for the other one, I think that it's doable via a middleware - but it's more work .. with less obvious gain. ( unless it's easy to configure )

2opremio · 2023-08-24T16:03:20Z

Limiting the size of http requests is a pretty common practice.

Should we limit the number of entries queried to, say, 200?

2opremio · 2023-08-24T16:09:35Z

@tsachiherman see https://pkg.go.dev/net/http#MaxBytesReader

cmd/soroban-rpc/internal/db/ledgerentry.go

2opremio requested review from paulbellamy and stellarsaur August 24, 2023 10:39

2opremio force-pushed the really-do-get-multiple-ledger-entries branch from 7891fd6 to a4818b6 Compare August 24, 2023 10:42

2opremio requested a review from tsachiherman August 24, 2023 10:44

2opremio force-pushed the really-do-get-multiple-ledger-entries branch from fa9e62a to 7ea058e Compare August 24, 2023 12:55

2opremio added 3 commits August 24, 2023 15:22

soroban-rpc: getLedgerEntries: query multiple ledger entries at once

8fae9e7

This improves performance of the getLedgerEntries and also paves the way to querying multiple entries from libpreflight.

Add GetLedgerEntry benchmark and optimize it slightly

a3fb086

Move getLatestLedgerSequence inside the loop now that it's cached

2e9b9de

2opremio force-pushed the really-do-get-multiple-ledger-entries branch from 7ea058e to 2e9b9de Compare August 24, 2023 13:23

tamirms reviewed Aug 24, 2023

View reviewed changes

cmd/soroban-rpc/internal/db/ledgerentry.go Outdated Show resolved Hide resolved

tamirms reviewed Aug 24, 2023

View reviewed changes

cmd/soroban-rpc/internal/db/ledgerentry.go Show resolved Hide resolved

paulbellamy approved these changes Aug 24, 2023

View reviewed changes

cmd/soroban-rpc/internal/db/ledgerentry.go Outdated Show resolved Hide resolved

tamirms reviewed Aug 24, 2023

View reviewed changes

cmd/soroban-rpc/internal/db/ledgerentry.go Show resolved Hide resolved

tamirms reviewed Aug 24, 2023

View reviewed changes

cmd/soroban-rpc/internal/preflight/pool.go Outdated Show resolved Hide resolved

tsachiherman reviewed Aug 24, 2023

View reviewed changes

tsachiherman assigned 2opremio Aug 24, 2023

tsachiherman added this to the Soroban Testnet Release milestone Aug 24, 2023

Address review feedback

6cf7543

Shaptic reviewed Aug 24, 2023

View reviewed changes

cmd/soroban-rpc/internal/db/ledgerentry.go Show resolved Hide resolved

Merge branch 'main' into really-do-get-multiple-ledger-entries

e0c6039

2opremio added 3 commits August 25, 2023 10:26

Address review feedback

d77c241

Add review feedback

2b632bd

Merge branch 'main' into really-do-get-multiple-ledger-entries

3214170

2opremio enabled auto-merge (squash) August 25, 2023 08:30

2opremio merged commit fa3dd8d into stellar:main Aug 25, 2023

2opremio deleted the really-do-get-multiple-ledger-entries branch August 25, 2023 09:05

dmore mentioned this pull request Sep 29, 2023

[Snyk] Upgrade @stellar/freighter-api from 1.5.1 to 1.6.0 dmore/soroban-tools#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

soroban-rpc: getLedgerEntries: query multiple ledger entries at once #896

soroban-rpc: getLedgerEntries: query multiple ledger entries at once #896

2opremio commented Aug 24, 2023 •

edited

Loading

2opremio commented Aug 24, 2023

2opremio commented Aug 24, 2023 •

edited

Loading

2opremio commented Aug 24, 2023 •

edited

Loading

2opremio commented Aug 24, 2023 •

edited

Loading

2opremio commented Aug 24, 2023

tsachiherman left a comment

tsachiherman commented Aug 24, 2023

2opremio commented Aug 24, 2023

tsachiherman commented Aug 24, 2023

2opremio commented Aug 24, 2023 •

edited

Loading

2opremio commented Aug 24, 2023

soroban-rpc: getLedgerEntries: query multiple ledger entries at once #896

soroban-rpc: getLedgerEntries: query multiple ledger entries at once #896

Conversation

2opremio commented Aug 24, 2023 • edited Loading

2opremio commented Aug 24, 2023

2opremio commented Aug 24, 2023 • edited Loading

2opremio commented Aug 24, 2023 • edited Loading

2opremio commented Aug 24, 2023 • edited Loading

2opremio commented Aug 24, 2023

tsachiherman left a comment

Choose a reason for hiding this comment

tsachiherman commented Aug 24, 2023

2opremio commented Aug 24, 2023

tsachiherman commented Aug 24, 2023

2opremio commented Aug 24, 2023 • edited Loading

2opremio commented Aug 24, 2023

2opremio commented Aug 24, 2023 •

edited

Loading

2opremio commented Aug 24, 2023 •

edited

Loading

2opremio commented Aug 24, 2023 •

edited

Loading

2opremio commented Aug 24, 2023 •

edited

Loading

2opremio commented Aug 24, 2023 •

edited

Loading