-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
perf: Add support for multi get operation for database queries #2396
base: master
Are you sure you want to change the base?
Changes from 32 commits
c77045f
e66db04
9eace87
8ba0c9d
76b919b
2333dfb
49994b9
2fb9e88
b2c2632
df63d48
4d5ca7d
36a50bb
6275b36
e1e50fa
f76d210
4feadfd
e141f56
8e3fd0f
39664bb
52eca64
5b0e975
bc955fd
491c452
676a5d4
8b35460
f697707
3d0a71f
5de80b7
d7a30c8
f27bdbb
9ea8894
2c2d222
df33ef2
79e42da
cbb6efc
07f9b94
75fedc7
7ae04bc
c3b7264
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -177,7 +177,7 @@ impl LowerHex for TxPointer { | |
} | ||
} | ||
|
||
#[derive(cynic::Scalar, Debug, Clone)] | ||
#[derive(cynic::Scalar, Debug, Clone, PartialEq, Eq)] | ||
pub struct HexString(pub Bytes); | ||
|
||
impl From<HexString> for Vec<u8> { | ||
|
@@ -194,7 +194,7 @@ impl Deref for HexString { | |
} | ||
} | ||
|
||
#[derive(Debug, Clone)] | ||
#[derive(Debug, Clone, PartialEq, Eq)] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same reply :) |
||
pub struct Bytes(pub Vec<u8>); | ||
|
||
impl FromStr for Bytes { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,8 +12,6 @@ use fuel_core_storage::{ | |
IntoBoxedIter, | ||
IterDirection, | ||
}, | ||
not_found, | ||
tables::Transactions, | ||
transactional::AtomicView, | ||
Error as StorageError, | ||
IsNotFound, | ||
|
@@ -65,6 +63,7 @@ use fuel_core_types::{ | |
use futures::Stream; | ||
use std::{ | ||
borrow::Cow, | ||
collections::BTreeMap, | ||
sync::Arc, | ||
}; | ||
|
||
|
@@ -141,29 +140,57 @@ impl ReadView { | |
pub fn transaction(&self, tx_id: &TxId) -> StorageResult<Transaction> { | ||
let result = self.on_chain.transaction(tx_id); | ||
if result.is_not_found() { | ||
if let Some(tx) = self.off_chain.old_transaction(tx_id)? { | ||
Ok(tx) | ||
} else { | ||
Err(not_found!(Transactions)) | ||
} | ||
self.off_chain.old_transaction(tx_id) | ||
} else { | ||
result | ||
} | ||
} | ||
|
||
pub async fn transactions( | ||
pub async fn transactions(&self, tx_ids: &[TxId]) -> Vec<StorageResult<Transaction>> { | ||
let transactions: Vec<_> = self | ||
.on_chain | ||
.transactions(tx_ids.iter().into_boxed()) | ||
.collect(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suspect you can avoid collecting here since later you iterate on transactions again. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
// Give a chance for other tasks to run. | ||
tokio::task::yield_now().await; | ||
|
||
if transactions.iter().any(|result| result.is_not_found()) { | ||
let on_chain_results = tx_ids.iter().enumerate().zip(transactions).collect(); | ||
|
||
self.extend_with_off_chain_results(on_chain_results).await | ||
} else { | ||
transactions | ||
} | ||
} | ||
|
||
pub async fn extend_with_off_chain_results( | ||
netrome marked this conversation as resolved.
Show resolved
Hide resolved
|
||
&self, | ||
tx_ids: Vec<TxId>, | ||
on_chain_results: BTreeMap<(usize, &TxId), StorageResult<Transaction>>, | ||
) -> Vec<StorageResult<Transaction>> { | ||
// TODO: Use multiget when it's implemented. | ||
// https://github.com/FuelLabs/fuel-core/issues/2344 | ||
let result = tx_ids | ||
let off_chain_indexed_txids: Vec<_> = on_chain_results | ||
.iter() | ||
.map(|tx_id| self.transaction(tx_id)) | ||
.collect::<Vec<_>>(); | ||
// Give a chance to other tasks to run. | ||
.filter_map(|(indexed_tx_id, result)| { | ||
result.is_not_found().then_some(*indexed_tx_id) | ||
}) | ||
.collect(); | ||
|
||
let off_chain_results = off_chain_indexed_txids.iter().copied().zip( | ||
self.off_chain.old_transactions( | ||
off_chain_indexed_txids | ||
.iter() | ||
.map(|(_, tx_id)| *tx_id) | ||
.into_boxed(), | ||
), | ||
); | ||
|
||
let mut results = on_chain_results; | ||
results.extend(off_chain_results); | ||
|
||
// Give a chance for other tasks to run. | ||
tokio::task::yield_now().await; | ||
result | ||
|
||
results.into_values().collect() | ||
} | ||
|
||
pub fn block(&self, height: &BlockHeight) -> StorageResult<CompressedBlock> { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these changes needed? I can't seem to find a place where these additional derives are necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.