perf: Add support for multi get operation for database queries #2396

netrome · 2024-10-25T09:12:05Z

Linked Issues

Description

This PR adds support for getting batches of values from the storage through a new StorageBatchInspect trait in fuel-core-storage.

This trait is implemented for StructuredStorage and GenericDatabase through a new get_batch method added to the KeyValueInspect and BluePrintInspect traits. The get_batch method is implemented with the multi-get operation for the RocksDB based storage implementations, and uses a default blanket implementation for in-memory implementations.

Checklist

Breaking changes are clearly marked as such in the PR description and changelog

Before requesting review

I have reviewed the code myself

…n-the-database

Proposals to the #2396

crates/storage/src/codec.rs

xgreenx · 2024-10-31T11:21:27Z

crates/storage/src/kv_store.rs

@@ -41,7 +44,7 @@ pub trait StorageColumn: Copy + core::fmt::Debug {

 /// The definition of the key-value inspection store.
 #[impl_tools::autoimpl(for<T: trait> &T, &mut T, Box<T>)]
-pub trait KeyValueInspect {
+pub trait KeyValueInspect: Send + Sync {


It is very sad that we need to require Send + Sync only because BoxedIter requires Send. Maybe we can define non Send boxed iterator instead and use it(because it seems we don't need Send feature, but maybe I'm wrong)?

Yeah this bound is just to satisfy the BoxedIter requirement. I can see if I can define a non-send one.

Hitting some walls with this implementation. Will try again tomorrow with fresh eyes.

Alright my mistake was trying to change some iterators that we turn into streams, and they need to be Send. Now I have managed to get rid of these trait bounds cbb6efc.

Now we have two boxed iterators: BoxedIter which doesn't require Send, and BoxedIterSend for the cases when you need Send.

crates/fuel-core/src/service/adapters/graphql_api/on_chain.rs

xgreenx · 2024-10-31T11:26:31Z

crates/fuel-core/src/service/adapters/graphql_api/off_chain.rs

+        <Self as StorageBatchInspect<OldTransactions>>::get_batch(self, ids)
+            .map(|result| result.and_then(|opt| opt.ok_or(not_found!(OldTransactions))))
+            .into_boxed()


If you want, you can add the same syntax sugars that we did with storage_as_ref to avoid <Self as StorageInspect<M>> usage.

Oh interesting, that would be nice. I'll look into it.

Turns out this is not as trivial as I had thought, as we're hitting some lifetime issues.

The problem

get_batch returns an iterator bound to the lifetime of the self parameter. I.e. the storage we call it on. This is necessary since we do self.get(...) within the KeyValueInspect::get_batch implementation.

So while we can implement StorageBatchInspect for the StorageRef type as:

impl<'a, S, Type> StorageBatchInspect<Type> for StorageRef<'a, S, Type> where S: StorageBatchInspect<Type>, Type: Mappable, { #[inline(always)] fn get_batch<'b, Iter>( &'b self, _keys: Iter, ) -> impl Iterator<Item = Result<Option<Type::OwnedValue>>> + 'b where Iter: 'b + IntoIterator<Item = &'b Type::Key>, Type::Key: 'b, { None.into_iter() // Note that we'd need access to `self.0` here which requires VM changes, or copying the `StorageRef` type into this crate. But that's a separate, and very manageable issue } }

when we try to use it as

fn old_transactions<'a>( &'a self, ids: BoxedIter<'a, &'a TxId>, ) -> BoxedIter<'a, StorageResult<Transaction>> { self.storage::<OldTransactions>() .get_batch(ids) .map(|result| result.and_then(|opt| opt.ok_or(not_found!(OldTransactions)))) .into_boxed() }

we're hitting this issue

error[E0716]: temporary value dropped while borrowed --> crates/fuel-core/src/service/adapters/graphql_api/off_chain.rs:183:9 | 179 | fn old_transactions<'a>( | -- lifetime `'a` defined here ... 183 | self.storage::<OldTransactions>() | -^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | | | _________creates a temporary value which is freed while still in use | | 184 | | .get_batch(ids) | |___________________________- argument requires that borrow lasts for `'a` ... 187 | } | - temporary value is freed at the end of this statement

Going forward

For this PR I suggest we don't go down this rabbit hole, but I'd be happy to do it as a follow-up if you think it's possible/interesting to explore it. Seems like a quite low prio though.

xgreenx

It would be nice to get some benchmarks(I guess you need to add new one that will use GraphQL) =)

Since this PR add `into_bytes` for the encoder, we can optimize the batch mutate operations as well #2396 (review)

netrome · 2024-10-31T13:39:57Z

It would be nice to get some benchmarks(I guess you need to add new one that will use GraphQL) =)

Sure thing. I'm not sure if it makes sense to use criterion/cargo bench for end-to-end performance testing though as it is more geared towards micro-benchmarking, and we need to do something like the following.

Populate the database with a bunch of coins/transactions/messages etc.
Call the DatabaseMessages::message_batch, DatabaseCoins::coins etc. methods and record the lookup times.

I'll look into our options for this type of workload. Let me know if you have any opinions or thoughts on this.

…n-the-database

netrome · 2024-11-09T06:30:54Z

It would be nice to get some benchmarks(I guess you need to add new one that will use GraphQL) =)

Sure thing. I'm not sure if it makes sense to use criterion/cargo bench for end-to-end performance testing though as it is more geared towards micro-benchmarking, and we need to do something like the following.
1. Populate the database with a bunch of coins/transactions/messages etc.

2. Call the `DatabaseMessages::message_batch`, `DatabaseCoins::coins` etc. methods and record the lookup times.
I'll look into our options for this type of workload. Let me know if you have any opinions or thoughts on this.

I'll aim at defining these workloads as a stand-alone binary, which would allow us to use hyperfine to actually execute the benchmarks and interpret the results.

rafal-ch · 2024-11-14T14:49:39Z

crates/client/src/client/schema/primitives.rs

@@ -177,7 +177,7 @@ impl LowerHex for TxPointer {
    }
 }

-#[derive(cynic::Scalar, Debug, Clone)]
+#[derive(cynic::Scalar, Debug, Clone, PartialEq, Eq)]


Are these changes needed? I can't seem to find a place where these additional derives are necessary.

Right, it doesn't seem like these are needed anymore. I probably added them while iterating on the code, but I can cut them out. Though since this is a public type, I'd argue that we still should proactively add these common traits as per https://rust-lang.github.io/api-guidelines/interoperability.html#types-eagerly-implement-common-traits-c-common-traits

I can still cut them out if you want me to. Let me know what you think.

rafal-ch · 2024-11-14T14:49:58Z

crates/client/src/client/schema/primitives.rs

@@ -194,7 +194,7 @@ impl Deref for HexString {
    }
 }

-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, PartialEq, Eq)]


Same as above.

Same reply :)

crates/fuel-core/src/graphql_api/database.rs

rafal-ch · 2024-11-14T15:33:34Z

crates/storage/src/codec.rs

 }

 /// The trait encodes the type to the bytes and passes it to the `Encoder`,
 /// which stores it and provides a reference to it. That allows gives more
 /// flexibility and more performant encoding, allowing the use of slices and arrays
 /// instead of vectors in some cases. Since the [`Encoder`] returns `Cow<[u8]>`,
 /// it is always possible to take ownership of the serialized value.
-pub trait Encode<T: ?Sized> {
+pub trait Encode<T: ?Sized>: 'static {


I'm not exactly sure why we need the 'static bound here.

Right no this is just a leftover from a previous attempt to fix the lifetime issues with the Encode trait, but in the current solution this is redundant. Thanks for noticing!

Updated in c3b7264

Co-authored-by: Rafał Chabowski <[email protected]>

acerone85 · 2024-11-15T11:09:31Z

crates/storage/src/blueprint.rs

+        storage: &'a S,
+        keys: Iter,
+        column: S::Column,
+    ) -> impl Iterator<Item = crate::Result<Option<M::OwnedValue>>> + 'a


nit:

Suggested change

) -> impl Iterator<Item = crate::Result<Option<M::OwnedValue>>> + 'a

) -> impl IntoIterator<Item = crate::Result<Option<M::OwnedValue>>> + 'a

Iterators implement IntoIterator, so if you change the signature to return a IntoIterator then StorageBatchInspect becomes slightly more flexible, as implementations can return either an iterator or a collection.

As a general principle, I like interfaces to be very general in what they accept but prefer them to be specific in what they return. (see Robusness principle). That's why I typically accept IntoIterator in parameters, because as you say then the caller doesn't have to care about doing `.into_iter()´ explicitly if they have a collection or anything.

Therefore, for return values it's better in my opinion to return the Iterator directly since it is more specific. Otherwise, all callers are basically forced to call .into_iter() before they can do anything with the returned iterator, which is less ergonomic. Any implementer of the trait can (and should) always call .into_iter() if they are about to return a collection. Again, not doing so would just force this burden upon the user of the function which is less than ideal without providing any benefit whatsoever.

Let me know if you disagree and I'll be happy to further discuss this.

acerone85 · 2024-11-15T11:40:03Z

crates/fuel-core/src/graphql_api/database.rs

+        let transactions: Vec<_> = self
+            .on_chain
+            .transactions(tx_ids.iter().into_boxed())
+            .collect();


I suspect you can avoid collecting here since later you iterate on transactions again.

Unforutnately I need to both iterate over the transactions and zip them with the on_chain_results - so we need to iterate twice over the tersults. We could potentially do something fancy with Itertools::tee, but I think this solution is simpler and easier on the eyes.

netrome · 2024-11-15T21:42:10Z

Got some initial benchmark results in #2433, and it doesn't look good for the multi-get implementation. So far it's either slower or the same for all workloads I've tried. I've done the following:

Measure end to end query times for getting transactions by owner over GraphQL, database populated by submitting a bunch of transactions (100k or 1M in most tests). with the same owner.
Measure transaction lookup times using a ReadView directly reading a database pre-populated with 1M transactions (also tried different parameters here).
Measure coin lookup times using a ReadView directly reading a database pre-populated with 1M coins. Tried with different parameters, and also with concurrent reads. Experimented with enabling/disabling caching in rocksdb.

I'm all ears if anyone has any suggestions for workloads to try, but until we can find evidence that this change actually is an improvement in any way we should put this PR on hold.

netrome linked an issue Oct 25, 2024 that may be closed by this pull request

Add support for multi-get operation in the Database #2344

Open

netrome changed the base branch from release/v0.40.0 to master October 25, 2024 09:12

netrome force-pushed the 2344-add-support-for-multi-get-operation-in-the-database branch from f9e0289 to 2532b06 Compare October 25, 2024 12:31

netrome self-assigned this Oct 29, 2024

netrome changed the title ~~2344 add support for multi get operation in the database~~ feat: Add support for multi get operation for database queries Oct 29, 2024

netrome force-pushed the 2344-add-support-for-multi-get-operation-in-the-database branch 3 times, most recently from 97f786f to 5405e6c Compare October 29, 2024 09:40

netrome marked this pull request as ready for review October 29, 2024 21:22

netrome requested review from xgreenx, Dentosal and MitchTurner as code owners October 29, 2024 21:22

netrome force-pushed the 2344-add-support-for-multi-get-operation-in-the-database branch from 2920ae8 to cff7b47 Compare October 29, 2024 21:23

netrome requested a review from a team October 29, 2024 21:23

netrome force-pushed the 2344-add-support-for-multi-get-operation-in-the-database branch from cff7b47 to b23e4be Compare October 30, 2024 09:24

netrome added 15 commits October 30, 2024 20:10

feat: Multi-get with boxed iterators

c77045f

feat: Multi get on bĺueprint

e66db04

feat: get_multi on RocksDB

9eace87

feat: Use local fuel-vm

8ba0c9d

feat: Implementation for structured storage

76b919b

feat: Use multi-get

2333dfb

feat: Messages impl

49994b9

feat: Don't rely on modified StorageInspect

2fb9e88

wip: Introduce specific StorageBatchInspect trait

b2c2632

wip: Use specific trait

df63d48

feat: Use boxed iterator

4d5ca7d

feat: Use multi-get when getting full block

36a50bb

feat: Use multi-get when getting coins

6275b36

feat: Use multi-get when getting transactions

e1e50fa

refactor: Rename multi_get -> get_batch

f76d210

netrome added 3 commits October 31, 2024 09:42

fix: Whitespace

d7a30c8

feat: Take Cow as keys in KeyValueInspect::get_batch

f27bdbb

Merge branch 'master' into 2344-add-support-for-multi-get-operation-i…

9ea8894

…n-the-database

netrome requested review from xgreenx and a team October 31, 2024 09:57

xgreenx mentioned this pull request Oct 31, 2024

Proposals to multi get PR #2419

Merged

Proposals to multi get PR (#2419)

2c2d222

Proposals to the #2396

xgreenx reviewed Oct 31, 2024

View reviewed changes

xgreenx mentioned this pull request Oct 31, 2024

Another proposals to multi get PR #2420

Merged

xgreenx reviewed Oct 31, 2024

View reviewed changes

netrome and others added 2 commits October 31, 2024 13:03

feat: Simplify DatabaseCoins port

df33ef2

Another proposals to multi get PR (#2420)

79e42da

Since this PR add `into_bytes` for the encoder, we can optimize the batch mutate operations as well #2396 (review)

netrome force-pushed the 2344-add-support-for-multi-get-operation-in-the-database branch from 32993e0 to 3a3d967 Compare November 1, 2024 09:02

feat: Don't require Send in BoxedIter

cbb6efc

netrome force-pushed the 2344-add-support-for-multi-get-operation-in-the-database branch from 3a3d967 to cbb6efc Compare November 1, 2024 09:05

Merge branch 'master' into 2344-add-support-for-multi-get-operation-i…

07f9b94

…n-the-database

netrome requested review from a team and xgreenx November 1, 2024 09:10

fix: Cargo fmt

75fedc7

netrome changed the title ~~feat: Add support for multi get operation for database queries~~ perf: Add support for multi get operation for database queries Nov 1, 2024

netrome mentioned this pull request Nov 1, 2024

chore: Benchmark multi-get implementation #2422

Open

rafal-ch reviewed Nov 14, 2024

View reviewed changes

netrome and others added 2 commits November 15, 2024 10:25

Update crates/fuel-core/src/graphql_api/database.rs

7ae04bc

Co-authored-by: Rafał Chabowski <[email protected]>

fix: Remove redundant 'static bound

c3b7264

acerone85 reviewed Nov 15, 2024

View reviewed changes

netrome force-pushed the 2344-add-support-for-multi-get-operation-in-the-database branch from 36386af to c3b7264 Compare November 15, 2024 20:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: Add support for multi get operation for database queries #2396

perf: Add support for multi get operation for database queries #2396

netrome commented Oct 25, 2024 •

edited

Loading

xgreenx Oct 31, 2024

netrome Oct 31, 2024

netrome Oct 31, 2024

netrome Nov 1, 2024

xgreenx Oct 31, 2024

netrome Oct 31, 2024

netrome Nov 1, 2024

xgreenx left a comment

netrome commented Oct 31, 2024

netrome commented Nov 9, 2024

rafal-ch Nov 14, 2024

netrome Nov 15, 2024

rafal-ch Nov 14, 2024

netrome Nov 15, 2024

rafal-ch Nov 14, 2024

netrome Nov 15, 2024

netrome Nov 15, 2024

acerone85 Nov 15, 2024 •

edited

Loading

netrome Nov 15, 2024

acerone85 Nov 15, 2024

netrome Nov 15, 2024

netrome commented Nov 15, 2024

	) -> impl Iterator<Item = crate::Result<Option<M::OwnedValue>>> + 'a
	) -> impl IntoIterator<Item = crate::Result<Option<M::OwnedValue>>> + 'a

perf: Add support for multi get operation for database queries #2396

Are you sure you want to change the base?

perf: Add support for multi get operation for database queries #2396

Conversation

netrome commented Oct 25, 2024 • edited Loading

Linked Issues

Description

Checklist

Before requesting review

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

The problem

Going forward

xgreenx left a comment

Choose a reason for hiding this comment

netrome commented Oct 31, 2024

netrome commented Nov 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acerone85 Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netrome commented Nov 15, 2024

netrome commented Oct 25, 2024 •

edited

Loading

acerone85 Nov 15, 2024 •

edited

Loading