From 309d1f0b70ccb1d7137480e3461d58d2adff12de Mon Sep 17 00:00:00 2001 From: joshieDo <93316087+joshieDo@users.noreply.github.com> Date: Mon, 5 Aug 2024 16:15:08 +0200 Subject: [PATCH 1/6] add StaticFileWriters to StaticFileProvider --- .../src/providers/static_file/manager.rs | 29 +++------ .../src/providers/static_file/writer.rs | 63 ++++++++++++++++++- 2 files changed, 69 insertions(+), 23 deletions(-) diff --git a/crates/storage/provider/src/providers/static_file/manager.rs b/crates/storage/provider/src/providers/static_file/manager.rs index ad813d6b81fe..523fdf18fa7a 100644 --- a/crates/storage/provider/src/providers/static_file/manager.rs +++ b/crates/storage/provider/src/providers/static_file/manager.rs @@ -1,13 +1,14 @@ use super::{ - metrics::StaticFileProviderMetrics, LoadedJar, StaticFileJarProvider, StaticFileProviderRW, - StaticFileProviderRWRefMut, BLOCKS_PER_STATIC_FILE, + metrics::StaticFileProviderMetrics, writer::StaticFileWriters, LoadedJar, + StaticFileJarProvider, StaticFileProviderRW, StaticFileProviderRWRefMut, + BLOCKS_PER_STATIC_FILE, }; use crate::{ to_range, BlockHashReader, BlockNumReader, BlockReader, BlockSource, DatabaseProvider, HeaderProvider, ReceiptProvider, RequestsProvider, StageCheckpointReader, StatsReader, TransactionVariant, TransactionsProvider, TransactionsProviderExt, WithdrawalsProvider, }; -use dashmap::{mapref::entry::Entry as DashMapEntry, DashMap}; +use dashmap::DashMap; use parking_lot::RwLock; use reth_chainspec::ChainInfo; use reth_db::{ @@ -114,8 +115,8 @@ pub struct StaticFileProviderInner { /// Whether [`StaticFileJarProvider`] loads filters into memory. If not, `by_hash` queries /// won't be able to be queried directly. load_filters: bool, - /// Maintains a map of `StaticFile` writers for each [`StaticFileSegment`] - writers: DashMap, + /// Maintains a writer set of [`StaticFileSegment`]. + writers: StaticFileWriters, metrics: Option>, /// Access rights of the provider. access: StaticFileAccess, @@ -1055,17 +1056,8 @@ impl StaticFileWriter for StaticFileProvider { } trace!(target: "provider::static_file", ?block, ?segment, "Getting static file writer."); - Ok(match self.writers.entry(segment) { - DashMapEntry::Occupied(entry) => entry.into_ref(), - DashMapEntry::Vacant(entry) => { - let writer = StaticFileProviderRW::new( - segment, - block, - Arc::downgrade(&self.0), - self.metrics.clone(), - )?; - entry.insert(writer) - } + self.writers.get_or_create(segment, || { + StaticFileProviderRW::new(segment, block, Arc::downgrade(&self.0), self.metrics.clone()) }) } @@ -1077,10 +1069,7 @@ impl StaticFileWriter for StaticFileProvider { } fn commit(&self) -> ProviderResult<()> { - for mut writer in self.writers.iter_mut() { - writer.commit()?; - } - Ok(()) + self.writers.commit() } fn ensure_file_consistency(&self, segment: StaticFileSegment) -> ProviderResult<()> { diff --git a/crates/storage/provider/src/providers/static_file/writer.rs b/crates/storage/provider/src/providers/static_file/writer.rs index f63912512e47..c95ab67e7ded 100644 --- a/crates/storage/provider/src/providers/static_file/writer.rs +++ b/crates/storage/provider/src/providers/static_file/writer.rs @@ -2,7 +2,7 @@ use super::{ manager::StaticFileProviderInner, metrics::StaticFileProviderMetrics, StaticFileProvider, }; use crate::providers::static_file::metrics::StaticFileProviderOperation; -use dashmap::mapref::one::RefMut; +use parking_lot::{lock_api::RwLockWriteGuard, RawRwLock, RwLock}; use reth_codecs::Compact; use reth_db_api::models::CompactU256; use reth_nippy_jar::{ConsistencyFailStrategy, NippyJar, NippyJarError, NippyJarWriter}; @@ -20,8 +20,65 @@ use std::{ }; use tracing::debug; -/// Mutable reference to a dashmap element of [`StaticFileProviderRW`]. -pub type StaticFileProviderRWRefMut<'a> = RefMut<'a, StaticFileSegment, StaticFileProviderRW>; +/// Static file writers for every known [`StaticFileSegment`]. +#[derive(Debug, Default)] +pub(crate) struct StaticFileWriters { + headers: RwLock>, + transactions: RwLock>, + receipts: RwLock>, +} + +impl StaticFileWriters { + pub(crate) fn get_or_create( + &self, + segment: StaticFileSegment, + create_fn: impl FnOnce() -> ProviderResult, + ) -> ProviderResult> { + let mut write_guard = match segment { + StaticFileSegment::Headers => self.headers.write(), + StaticFileSegment::Transactions => self.transactions.write(), + StaticFileSegment::Receipts => self.receipts.write(), + }; + + if write_guard.is_none() { + *write_guard = Some(create_fn()?); + } + + Ok(StaticFileProviderRWRefMut(write_guard)) + } + + pub(crate) fn commit(&self) -> ProviderResult<()> { + for writer_lock in [&self.headers, &self.transactions, &self.receipts] { + let mut writer = writer_lock.write(); + if let Some(writer) = writer.as_mut() { + writer.commit()?; + } + } + Ok(()) + } +} + +#[derive(Debug)] +/// Mutable reference to a [`StaticFileProviderRW`] behind a [`RwLockWriteGuard`]. +pub struct StaticFileProviderRWRefMut<'a>( + pub(crate) RwLockWriteGuard<'a, RawRwLock, Option>, +); + +impl<'a> std::ops::DerefMut for StaticFileProviderRWRefMut<'a> { + fn deref_mut(&mut self) -> &mut Self::Target { + // This is always created by [`StaticFileWriters::get_or_create`] + self.0.as_mut().expect("should exist") + } +} + +impl<'a> std::ops::Deref for StaticFileProviderRWRefMut<'a> { + type Target = StaticFileProviderRW; + + fn deref(&self) -> &Self::Target { + // This is always created by [`StaticFileWriters::get_or_create`] + self.0.as_ref().expect("should exist") + } +} #[derive(Debug)] /// Extends `StaticFileProvider` with writing capabilities From 3341c8501ee94d7c975f72f2451e1e1abe96e84f Mon Sep 17 00:00:00 2001 From: joshieDo <93316087+joshieDo@users.noreply.github.com> Date: Mon, 5 Aug 2024 16:55:29 +0200 Subject: [PATCH 2/6] remove receipt writer from hotloop --- crates/storage/provider/src/writer/mod.rs | 26 ++++++++++------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/crates/storage/provider/src/writer/mod.rs b/crates/storage/provider/src/writer/mod.rs index eb4c610e5f03..bfd28ff88acc 100644 --- a/crates/storage/provider/src/writer/mod.rs +++ b/crates/storage/provider/src/writer/mod.rs @@ -157,6 +157,17 @@ where debug!(target: "provider::storage_writer", block_count = %blocks.len(), "Writing blocks and execution data to storage"); + // Only write receipts to static files if there is no receipt pruning configured. + let mut state_writer = if self.database().prune_modes_ref().has_receipts_pruning() { + UnifiedStorageWriter::from_database(self.database()) + } else { + UnifiedStorageWriter::from( + self.database(), + self.static_file() + .get_writer(first_block.number, StaticFileSegment::Receipts)?, + ) + }; + // TODO: remove all the clones and do performant / batched writes for each type of object // instead of a loop over all blocks, // meaning: @@ -175,21 +186,6 @@ where // Write state and changesets to the database. // Must be written after blocks because of the receipt lookup. let execution_outcome = block.execution_outcome().clone(); - - // Only write receipts to static files if there is no receipt pruning configured. - let mut state_writer = if self.database().prune_modes_ref().has_receipts_pruning() { - UnifiedStorageWriter::from_database(self.database()) - } else { - // This should be inside the hotloop, because preferably there should only be one - // mutable reference to a static file writer, since there's a 3 in 100 chance that - // another segment shares the same shard as the `Receipts` one. Which would result - // in a deadlock. - UnifiedStorageWriter::from( - self.database(), - self.static_file() - .get_writer(first_block.number, StaticFileSegment::Receipts)?, - ) - }; state_writer.write_to_storage(execution_outcome, OriginalValuesKnown::No)?; // insert hashes and intermediate merkle nodes From 85fae9640a5dba5e9394a2b53c3eabe0b8988fa2 Mon Sep 17 00:00:00 2001 From: joshieDo <93316087+joshieDo@users.noreply.github.com> Date: Mon, 5 Aug 2024 17:00:09 +0200 Subject: [PATCH 3/6] fmt --- crates/storage/provider/src/writer/mod.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/storage/provider/src/writer/mod.rs b/crates/storage/provider/src/writer/mod.rs index bfd28ff88acc..b3250bef062e 100644 --- a/crates/storage/provider/src/writer/mod.rs +++ b/crates/storage/provider/src/writer/mod.rs @@ -163,8 +163,7 @@ where } else { UnifiedStorageWriter::from( self.database(), - self.static_file() - .get_writer(first_block.number, StaticFileSegment::Receipts)?, + self.static_file().get_writer(first_block.number, StaticFileSegment::Receipts)?, ) }; From dded27b95cd97777b4c1f188845be29d232fd3ce Mon Sep 17 00:00:00 2001 From: joshieDo <93316087+joshieDo@users.noreply.github.com> Date: Mon, 5 Aug 2024 19:42:08 +0200 Subject: [PATCH 4/6] swap docs --- crates/storage/provider/src/providers/static_file/writer.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/storage/provider/src/providers/static_file/writer.rs b/crates/storage/provider/src/providers/static_file/writer.rs index c95ab67e7ded..370aebccde0b 100644 --- a/crates/storage/provider/src/providers/static_file/writer.rs +++ b/crates/storage/provider/src/providers/static_file/writer.rs @@ -58,8 +58,8 @@ impl StaticFileWriters { } } -#[derive(Debug)] /// Mutable reference to a [`StaticFileProviderRW`] behind a [`RwLockWriteGuard`]. +#[derive(Debug)] pub struct StaticFileProviderRWRefMut<'a>( pub(crate) RwLockWriteGuard<'a, RawRwLock, Option>, ); From 2f495626d5fc2229369f6b95a521588ceeb9e82c Mon Sep 17 00:00:00 2001 From: joshieDo <93316087+joshieDo@users.noreply.github.com> Date: Mon, 5 Aug 2024 20:35:50 +0200 Subject: [PATCH 5/6] add warning doc --- crates/storage/provider/src/providers/static_file/writer.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/storage/provider/src/providers/static_file/writer.rs b/crates/storage/provider/src/providers/static_file/writer.rs index 370aebccde0b..bc34f9729199 100644 --- a/crates/storage/provider/src/providers/static_file/writer.rs +++ b/crates/storage/provider/src/providers/static_file/writer.rs @@ -21,6 +21,9 @@ use std::{ use tracing::debug; /// Static file writers for every known [`StaticFileSegment`]. +/// +/// WARNING: Trying to use more than one writer for the same segment type **will result in a +/// deadlock**. #[derive(Debug, Default)] pub(crate) struct StaticFileWriters { headers: RwLock>, From f897b3d628769ebbefcbf7bb43d487a16c61e8e3 Mon Sep 17 00:00:00 2001 From: joshieDo <93316087+joshieDo@users.noreply.github.com> Date: Wed, 7 Aug 2024 12:31:35 +0200 Subject: [PATCH 6/6] add descriptive msg --- crates/storage/provider/src/providers/static_file/writer.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/storage/provider/src/providers/static_file/writer.rs b/crates/storage/provider/src/providers/static_file/writer.rs index bc34f9729199..6df36ab45fdb 100644 --- a/crates/storage/provider/src/providers/static_file/writer.rs +++ b/crates/storage/provider/src/providers/static_file/writer.rs @@ -70,7 +70,7 @@ pub struct StaticFileProviderRWRefMut<'a>( impl<'a> std::ops::DerefMut for StaticFileProviderRWRefMut<'a> { fn deref_mut(&mut self) -> &mut Self::Target { // This is always created by [`StaticFileWriters::get_or_create`] - self.0.as_mut().expect("should exist") + self.0.as_mut().expect("static file writer provider should be init") } } @@ -79,7 +79,7 @@ impl<'a> std::ops::Deref for StaticFileProviderRWRefMut<'a> { fn deref(&self) -> &Self::Target { // This is always created by [`StaticFileWriters::get_or_create`] - self.0.as_ref().expect("should exist") + self.0.as_ref().expect("static file writer provider should be init") } }