Serialize calls to the engine api #4651
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Performance Report✔️ no performance regression detected Full benchmark results
|
dapplion
reviewed
Oct 8, 2022
| jobWaitTime: register.histogram({ | ||
| name: "lodestar_engine_http_processor_queue_job_wait_time_seconds", | ||
| help: "Time from job added to the engine http processor queue to starting in seconds", | ||
| buckets: [0.1, 1, 10, 100], |
There was a problem hiding this comment.
Bucket values must be realistic on expected values else not useful at all. If this sort of x10 prev value buckets are still in Lodestar is because we add them a long time ago, or we haven't figured out the right values. The wait time metric is crucial so buckets must be good. See below as example of buckets values specific to a metric given expected values
lodestar/packages/beacon-node/src/metrics/metrics/lodestar.ts
Lines 369 to 370 in 1cba836
There was a problem hiding this comment.
hmmm right, most of the time will be taken up by new payload, and it can vary from 100s of ms to 2-4 seconds (if EL is really slow) and may be on average 300-400ms, so should be bucket for [0.1, 0.5, 2, 5] ?
There was a problem hiding this comment.
Then do for example
[0.05, 0.1, 0.2, 0.3, 0.5, 0.75, 1, 2, 5, 10, 25]
dapplion
reviewed
Oct 8, 2022
| method, | ||
| params: [payloadId], | ||
| methodOpts: getPayloadOpts, | ||
| }) as Promise<EngineApiRpcReturnTypes[typeof method]>); |
There was a problem hiding this comment.
getPayload doesn't need to be sequential right? Don't feel it needs to be queued
There was a problem hiding this comment.
right, can keep it out of the queue, but ideally there shouldn't be anything else in the queue at that moment anyways. still can keep it out.
dapplion
reviewed
Oct 8, 2022
| @@ -75,6 +78,14 @@ const exchageTransitionConfigOpts: ReqOpts = {routeId: "exchangeTransitionConfig | |||
| export class ExecutionEngineHttp implements IExecutionEngine { | |||
| readonly payloadIdCache = new PayloadIdCache(); | |||
| private readonly rpc: IJsonRpcHttpClient; | |||
| private readonly jobQueue: JobItemQueue<[EngineRequest], EngineResponse>; | |||
There was a problem hiding this comment.
Since it's not generic to multiple jobs I would call it rpcFetchQueue or something like that. Also add a comment explaining why this is necessary and reason which methods need it and which do not
dapplion
reviewed
Oct 8, 2022
| @@ -57,6 +58,8 @@ export const defaultExecutionEngineHttpOpts: ExecutionEngineHttpOpts = { | |||
| timeout: 12000, | |||
| }; | |||
|
|
|||
| const QUEUE_MAX_LENGHT = 256; | |||
There was a problem hiding this comment.
Write a JSDoc comment reasoning the choice of this number. What the tradeoff here? A job queue holds items in memory which in this case are full de-serialized blocks. Are there any memory concerns if the queue fills? What's a reasonable length given the usage of this queue? Since blocks are imported strictly sequential the queue length should always be pretty short.
There was a problem hiding this comment.
Will check from the blocks processor queue what limit it has, ideally there could be fcUs corresponding to the last batch of blocks plus new payload
wemeetagain
reviewed
Oct 10, 2022
| * Size for the serializing queue for fcUs and new payloads, the max length could be equal to | ||
| * EPOCHS_PER_BATCH * 2 in case new payloads are also not awaited serially | ||
| */ | ||
| const QUEUE_MAX_LENGHT = EPOCHS_PER_BATCH * SLOTS_PER_EPOCH * 2; |
wemeetagain
approved these changes
Oct 10, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Serialize calls to the engine api
Follow up of #4595
Metrics for engine job processor: