starknet_patricia_storage: spawn a task or not based on config#12174
starknet_patricia_storage: spawn a task or not based on config#12174nimrod-starkware wants to merge 1 commit intonimrod/parallel-reads/config-fieldfrom
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
ff7c5e0 to
87029a0
Compare
a1b79bf to
1729e41
Compare
1729e41 to
b32ebca
Compare
a594575 to
a8d8b53
Compare
b32ebca to
58b2329
Compare
a8d8b53 to
7b94b0e
Compare
58b2329 to
198f552
Compare
7b94b0e to
bcc7298
Compare
198f552 to
48301ce
Compare
e03a849 to
8222312
Compare
ee9db12 to
17fb5a6
Compare
8222312 to
07499bc
Compare
17fb5a6 to
04de417
Compare
07499bc to
8192046
Compare
04de417 to
93549f6
Compare
8192046 to
1e4407b
Compare
93549f6 to
001b426
Compare
1e4407b to
e990000
Compare
001b426 to
67b6567
Compare
67b6567 to
fa080d5
Compare
e990000 to
e7cd356
Compare
e7cd356 to
4a46808
Compare
fa080d5 to
03cf6b6
Compare
03cf6b6 to
fdf98af
Compare
4a46808 to
695d081
Compare
ArielElp
left a comment
There was a problem hiding this comment.
@ArielElp reviewed 4 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on nimrod-starkware).
crates/starknet_patricia_storage/src/rocksdb_storage.rs line 315 at r2 (raw file):
.map(|r| r.map(|opt| opt.map(DbValue)).map_err(|e| e.into())) .collect::<Result<Vec<_>, PatriciaStorageError>>() })
can you factor this out into a function and call it in both cases? Cursor suggested:
fn multi_get(
db: &DB,
keys: impl IntoIterator<Item = impl AsRef<[u8]>>,
) -> Result<Vec<Option<DbValue>>, PatriciaStorageError> {
db.multi_get(keys)
.into_iter()
.map(|r| r.map(|opt| opt.map(DbValue)).map_err(|e| e.into()))
.collect()
}
if self.config.spawn_blocking_reads {
let db = self.db.clone();
let keys: Vec<Vec<u8>> = keys.iter().map(|k| k.0.clone()).collect();
Ok(spawn_blocking(move || multi_get(&db, keys.iter().map(|k| k.as_slice())))
.await??)
} else {
Ok(multi_get(&self.db, keys.iter().map(|k| k.0.as_slice()))?)
}
Code quote:
let db = self.db.clone();
let keys: Vec<Vec<u8>> = keys.iter().map(|k| k.0.clone()).collect();
Ok(spawn_blocking(move || {
let raw_keys = keys.iter().map(|k| k.as_slice());
db.multi_get(raw_keys)
.into_iter()
.map(|r| r.map(|opt| opt.map(DbValue)).map_err(|e| e.into()))
.collect::<Result<Vec<_>, PatriciaStorageError>>()
})
nimrod-starkware
left a comment
There was a problem hiding this comment.
@nimrod-starkware made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on ArielElp).
crates/starknet_patricia_storage/src/rocksdb_storage.rs line 315 at r2 (raw file):
Previously, ArielElp wrote…
can you factor this out into a function and call it in both cases? Cursor suggested:
fn multi_get( db: &DB, keys: impl IntoIterator<Item = impl AsRef<[u8]>>, ) -> Result<Vec<Option<DbValue>>, PatriciaStorageError> { db.multi_get(keys) .into_iter() .map(|r| r.map(|opt| opt.map(DbValue)).map_err(|e| e.into())) .collect() }if self.config.spawn_blocking_reads { let db = self.db.clone(); let keys: Vec<Vec<u8>> = keys.iter().map(|k| k.0.clone()).collect(); Ok(spawn_blocking(move || multi_get(&db, keys.iter().map(|k| k.as_slice()))) .await??) } else { Ok(multi_get(&self.db, keys.iter().map(|k| k.0.as_slice()))?) }
Done in next PR

Note
Medium Risk
Changes the runtime execution model for storage reads (threadpool vs async runtime), which can affect latency/throughput and potential runtime blocking if misconfigured.
Overview
Makes RocksDB read offloading configurable.
RocksDbStoragenow storesRocksDbStorageConfigand conditionally usestokio::spawn_blockingforget/mgetonly whenspawn_blocking_readsis enabled; otherwise reads execute directly.Config defaults and deployment configs are updated to set
committer_config.storage_config.inner_storage_config.spawn_blocking_readstotrue, and the config schema default is changed accordingly.Written by Cursor Bugbot for commit 695d081. This will update automatically on new commits. Configure here.