-
Notifications
You must be signed in to change notification settings - Fork 280
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
Block publishing performance #8181
Conversation
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.
LGTM
Just some nits
It would be nice to have some tests to make sure the trackers (production and publishing) are called as expected... Could be added later
private final int latePublishingEventThreshold; | ||
private final Function<UInt64, UInt64> slotTimeCalculator; | ||
|
||
public BlockProductionAndPublishingPerformanceFactory( |
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.
I'd rename it to BlockPerformanceFactory
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.
mmm.. BlockPerformanceFactory
seems a bit generic to me. We have the block import performance, so need something more specific...
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.
And there is a BlockPerformance
class too. Well we could keep it then...
@@ -221,7 +231,7 @@ public MetricsConfigBuilder blockPerformanceEnabled(final boolean blockPerforman | |||
return this; | |||
} | |||
|
|||
public MetricsConfigBuilder blockProductionPerformanceEnabled( | |||
public MetricsConfigBuilder blockProductionAndPublishingPerformanceEnabled( | |||
final boolean blockProductionPerformanceEnabled) { |
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.
Should be renamed to blockProductionAndPublishingPerformanceEnabled
too
@@ -428,10 +428,14 @@ public UInt64 computeEpochAtSlot(final UInt64 slot) { | |||
return atSlot(slot).miscHelpers().computeEpochAtSlot(slot); | |||
} | |||
|
|||
public UInt64 computeTimeAtSlot(BeaconState state, UInt64 slot) { | |||
public UInt64 computeTimeAtSlot(final BeaconState state, final UInt64 slot) { | |||
return atSlot(slot).miscHelpers().computeTimeAtSlot(state.getGenesisTime(), slot); |
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.
Could call the new wrapper computeTimeAtSlot(state.getGenesisTime(), slot)
@@ -116,7 +116,7 @@ public class ValidatorApiHandler implements ValidatorApiChannel { | |||
*/ | |||
private static final int DUTY_EPOCH_TOLERANCE = 1; | |||
|
|||
private final BlockProductionPerformanceFactory blockProductionPerformanceFactory; | |||
private final BlockProductionAndPublishingPerformanceFactory blockProductionPerformanceFactory; |
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.
Should be renamed accordingly
BlockProductionPerformance
too)BlockPublishingPerformance
to cover the last step of block production flowexamples:
Slow Block Production *** Slot: 1397723 start 10ms, preparation_on_tick +0ms, preparation_apply_deferred_attestations +0ms, preparation_process_head +200ms, retrieve_state +1ms, beacon_block_prepared +293ms, local_get_payload +23ms, builder_get_header +169ms, builder_bid_validated +4ms, beacon_block_created +1ms, state_transition +161ms, state_hashing +14ms, complete +0ms total: 866ms
Slow Block Publishing *** Slot: 1397723 start 882ms, builder_get_payload +2300ms, blob_sidecars_prepared +10ms, block_and_blob_sidecars_publishing_initiated +0ms, block_import_completed +1ms, complete +0ms total: 2311ms
fixes #5808
Documentation
doc-change-required
label to this PR if updates are required.Changelog