Skip to content
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

feat: separate BlockExecutor metadata trait methods #6568

Closed
wants to merge 6 commits into from

Conversation

Rjected
Copy link
Member

@Rjected Rjected commented Feb 12, 2024

This adds a trait called BlockExecutorMetadata, which separates the metadata methods from the BlockExecutor trait, so the executor trait is only responsible for execution specific methods.

@Rjected Rjected added C-enhancement New feature or request A-execution Related to the Execution and EVM A-sdk Related to reth's use as a library labels Feb 12, 2024
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to make this part of the trait.

As you pointed out this has nothing to do with execution and we should treat it that way so that the executor impl is responsible for logging the stats when the type state is consumed.

This entire trait seems a bit strange imo, but I don't have a good suggestion for how to improve yet.


/// A [BlockExecutor] that can return metadata like current in-memory changes and internal
/// statistics.
pub trait BlockExecutorMetadata {
/// Internal statistics of execution.
fn stats(&self) -> BlockExecutorStats;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it feels a bit weird to turn this into a trait just for stats reporting.

the BlockExecutor trait seems a bit strange.

looks like we only use this here?

let state = executor.take_output_state();
let write_preparation_duration = time.elapsed();
let time = Instant::now();
// write output
state.write_to_db(provider.tx_ref(), OriginalValuesKnown::Yes)?;
let db_write_duration = time.elapsed();
debug!(
target: "sync::stages::execution",
block_fetch = ?fetch_block_duration,
execution = ?execution_duration,
write_preperation = ?write_preparation_duration,
write = ?db_write_duration,
"Execution time"
);
executor.stats().log_info();

can we make the stats reporting part of the take_output_state?

does this even have to be mutable?

fn take_output_state(&mut self) -> BundleStateWithReceipts;

or can we consume the type and log the stats there?

@Rjected Rjected force-pushed the dan/separate-block-executor-trait branch from c43b522 to 1dcc973 Compare February 13, 2024 17:03
@Rjected Rjected force-pushed the dan/separate-block-executor-trait branch from caaeab5 to 6b7ebd3 Compare February 13, 2024 23:24
@Rjected
Copy link
Member Author

Rjected commented Feb 14, 2024

Going to close this, and roll most of these changes into #6461

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-execution Related to the Execution and EVM A-sdk Related to reth's use as a library C-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants