-
Notifications
You must be signed in to change notification settings - Fork 59
feat(drive-abci): debug-only per-block phase timing #4573
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
base: v4.2-dev
Are you sure you want to change the base?
Changes from all commits
fe6a596
19f82f4
9826308
2e7fb4d
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -63,6 +63,9 @@ where | |||||
| transaction: &Transaction, | ||||||
| platform_version: &PlatformVersion, | ||||||
| ) -> Result<block_execution_outcome::v0::BlockFinalizationOutcome, Error> { | ||||||
| #[cfg(debug_assertions)] | ||||||
| let mut laps = crate::perf::Laps::new(); | ||||||
|
|
||||||
| let mut validation_result = SimpleValidationResult::<AbciError>::new_with_errors(vec![]); | ||||||
|
|
||||||
| let block_state_info = block_execution_context.block_state_info(); | ||||||
|
|
@@ -94,6 +97,9 @@ where | |||||
| .try_into() | ||||||
| .expect("invalid sha256 length"); | ||||||
|
|
||||||
| #[cfg(debug_assertions)] | ||||||
| laps.lap("fbp_msg_hash"); | ||||||
|
|
||||||
| //// Verification that commit is for our current executed block | ||||||
| // When receiving the finalized block, we need to make sure info matches our current block | ||||||
|
|
||||||
|
|
@@ -136,6 +142,9 @@ where | |||||
| return Ok(validation_result.into()); | ||||||
| } | ||||||
|
|
||||||
| #[cfg(debug_assertions)] | ||||||
| laps.lap("fbp_basic_checks"); | ||||||
|
|
||||||
| // Verify votes extensions | ||||||
| // We don't need to verify votes extension signatures once again after tenderdash | ||||||
| // here, because we will do it bellow broadcasting withdrawal transactions. | ||||||
|
|
@@ -154,6 +163,9 @@ where | |||||
| return Ok(validation_result.into()); | ||||||
| }; | ||||||
|
|
||||||
| #[cfg(debug_assertions)] | ||||||
| laps.lap("fbp_vote_ext"); | ||||||
|
|
||||||
| // Verify commit | ||||||
|
|
||||||
| // In production this will always be true | ||||||
|
|
@@ -188,6 +200,9 @@ where | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| #[cfg(debug_assertions)] | ||||||
| laps.lap("fbp_verify_commit"); | ||||||
|
Collaborator
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. 🟡 Suggestion: Do not sample commit verification when testing disables it With
Suggested change
source: ['claude'] |
||||||
|
|
||||||
| if height == self.config.abci.genesis_height { | ||||||
| self.drive | ||||||
| .set_genesis_time(block_state_info.block_time_ms()); | ||||||
|
|
@@ -205,13 +220,20 @@ where | |||||
|
|
||||||
| to_commit_block_info.core_height = block_header.core_chain_locked_height; | ||||||
|
|
||||||
| if !transaction_to_extension_matches.is_empty() { | ||||||
| #[cfg(debug_assertions)] | ||||||
| laps.lap("fbp_block_info"); | ||||||
|
|
||||||
| let broadcast_withdrawals = !transaction_to_extension_matches.is_empty(); | ||||||
| if broadcast_withdrawals { | ||||||
| self.append_signatures_and_broadcast_withdrawal_transactions( | ||||||
| transaction_to_extension_matches, | ||||||
| platform_version, | ||||||
| )?; | ||||||
| } | ||||||
|
|
||||||
| #[cfg(debug_assertions)] | ||||||
| laps.lap_if(broadcast_withdrawals, "fbp_wd_broadcast"); | ||||||
|
|
||||||
| // Update platform (drive abci) state | ||||||
|
|
||||||
| let extended_block_info = ExtendedBlockInfoV0 { | ||||||
|
|
@@ -225,12 +247,21 @@ where | |||||
| } | ||||||
| .into(); | ||||||
|
|
||||||
| #[cfg(debug_assertions)] | ||||||
| laps.lap("fbp_ext_block_info"); | ||||||
|
|
||||||
| self.update_drive_cache(&block_execution_context, platform_version)?; | ||||||
|
|
||||||
| #[cfg(debug_assertions)] | ||||||
| laps.lap("fbp_drive_cache"); | ||||||
|
|
||||||
| // Check if we should create a checkpoint (must be done before consuming block_execution_context) | ||||||
| let checkpoint_needed = | ||||||
| self.should_checkpoint(&block_execution_context, platform_version)?; | ||||||
|
|
||||||
| #[cfg(debug_assertions)] | ||||||
| laps.lap("fbp_should_checkpoint"); | ||||||
|
|
||||||
| let block_platform_state = block_execution_context.block_platform_state_owned(); | ||||||
|
|
||||||
| self.update_state_cache( | ||||||
|
|
@@ -240,6 +271,9 @@ where | |||||
| platform_version, | ||||||
| )?; | ||||||
|
|
||||||
| #[cfg(debug_assertions)] | ||||||
| laps.lap("fbp_state_cache"); | ||||||
|
|
||||||
| // Gather some metrics | ||||||
| crate::metrics::abci_last_block_time(block_header.time.seconds as u64); | ||||||
| crate::metrics::abci_last_platform_height(height); | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,6 +53,9 @@ where | |
| timer: Option<&HistogramTiming>, | ||
| ) -> Result<ValidationResult<block_execution_outcome::v0::BlockExecutionOutcome, Error>, Error> | ||
| { | ||
| #[cfg(debug_assertions)] | ||
| let mut laps = crate::perf::Laps::new(); | ||
|
|
||
| // Epoch information is always calculated with the last committed platform version | ||
| // even if we are switching to a new version in this block. | ||
| let last_committed_platform_version = platform_state.current_platform_version()?; | ||
|
|
@@ -66,6 +69,9 @@ where | |
| last_committed_platform_version, | ||
| )?; | ||
|
|
||
| #[cfg(debug_assertions)] | ||
| laps.lap("epoch_info"); | ||
|
|
||
| // Cleanup block cache before we execute a new proposal. | ||
| // | ||
| // This has to happen before `perform_events_on_first_block_of_protocol_change` below: | ||
|
|
@@ -74,9 +80,15 @@ where | |
| // them, leaving those reads to fall back to pre-change global cache entries. | ||
| self.clear_drive_block_cache(last_committed_platform_version)?; | ||
|
|
||
| #[cfg(debug_assertions)] | ||
| laps.lap("clear_block_cache"); | ||
|
|
||
| // Create a bock state from previous committed state | ||
| let mut block_platform_state = platform_state.clone(); | ||
|
|
||
| #[cfg(debug_assertions)] | ||
| laps.lap("state_clone"); | ||
|
Collaborator
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. Are you sure you want to measure clone? Please double check your laps - sometimes you measure nothing. |
||
|
|
||
| // Determine a platform version for this block | ||
| let block_platform_version = if epoch_info.is_epoch_change_but_not_genesis() | ||
| && platform_state.next_epoch_protocol_version() | ||
|
|
||
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.
why you not using tracing spans? as a side effect it would make logging more descriptive.