perf(consensus): never drop FCU #8238
Conversation
emhane
added
C-perf
|
wondering if it's tackling the problem at head better, to replace this with a reth/crates/consensus/beacon/src/engine/mod.rs Lines 187 to 188 in 081796b |
eventually, yes, but rn this is basically just fake concurrency of sequential message processing as prep for #7154 |
|
That looks good and should work. I was only thinking maybe we should only add the messages to the queue when we actually can't process them, i.e. when the pruner is running and we skip an FCU. Instead of always caching the messages and popping them from the queue on the next poll that I think slows down the engine message processing a bit. |
|
doesn't work though :/ @shekhirin @mattsse and not sure why, that loop is very complex |
|
think we should rewrite this future implementation to be a stream, so we can better reason about it. rn it's very complex to create a mental model of how this state machine will behave based on varying external conditions. related #6541. |
| // | ||
| // These messages can affect the state of the SyncController and they're also time | ||
| // sensitive, hence they are polled first. | ||
| if let Poll::Ready(Some(msg)) = this.engine_message_stream.poll_next_unpin(cx) { | ||
| while let Some(msg) = this.queued_engine_messages.pop_front() { |
There was a problem hiding this comment.
the engine message processing here only sets a single action to be performed in the blockchain tree. you cannot do it in a while loop because it overwrites the last set action
There was a problem hiding this comment.
didn't work before that commit either :/
There was a problem hiding this comment.
|
closing in favour of #8315 |
Closes #8203.
VecDequewith engine messages to ensure no messages a dropped.