Squash network inbound queues #13956
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
⏱️ 12h 13m total CI duration on this PR
|
brianolson
requested review from
JoshLind,
bchocho,
zekun000,
sasha8 and
ibalajiarun
as code owners
JoshLind
reviewed
Jul 11, 2024
mempool/src/tests/node.rs
Outdated
|
|
||
| #[cfg(wrong_network_abstraction)] |
There was a problem hiding this comment.
@bchocho, any thoughts on updating or removing all these tests? Should that be done before this PR?
(My feeling is that these should be fixed/removed first, e.g., to ensure we don't accidentally miss something. Feels odd just ignoring them all.)
| data: data.into(), | ||
| res_tx, | ||
| let notification = ReceivedMessage { | ||
| message: NetworkMessage::RpcRequest(RpcRequest { |
There was a problem hiding this comment.
nit: it might be worth exposing a test-only method to create these for tests (e.g., to avoid us having to assign 0's to request_id, priority and rx_at everywhere)?
| @@ -26,6 +26,7 @@ pub enum PeerManagerRequest { | |||
| } | |||
|
|
|||
| /// Notifications sent by PeerManager to upstream actors. | |||
| /// TODO: PeerManagerNotification now only exists in test code and should be deleted; probably use `ReceivedMessage` | |||
There was a problem hiding this comment.
After the mempool-side changes, this can now be removed completely?
JoshLind
reviewed
Jul 15, 2024
| let mut connection = MultiplexMessageSink::new(connection, MAX_FRAME_SIZE); | ||
| for _ in 0..30 { | ||
| // The client should then send the network message. | ||
| connection.send(&send_msg).await.unwrap(); | ||
| } | ||
| info!("client sent"); |
There was a problem hiding this comment.
nit: do we want to commit these, or were these for local debugging?
There was a problem hiding this comment.
test code logging is pretty cheap? it gets thrown away if the test passes, right?
network/framework/src/peer/mod.rs
Outdated
| ); | ||
| match self.upstream_handlers.get(&direct.protocol_id) { | ||
| None => { | ||
| // TODO: better label than "declined"? more like "garbage-in" |
There was a problem hiding this comment.
Makes sense to prioritize following up on the TODOs in a future PR 😄
|
I took a stab at mempool fixing and it wasn't as bad as the full network2 branch with changing both inbound and outbound paths. Merged the fix in here. |
bchocho
reviewed
Jul 18, 2024
bchocho
reviewed
Jul 18, 2024
| @@ -26,6 +26,7 @@ pub enum PeerManagerRequest { | |||
| } | |||
|
|
|||
| /// Notifications sent by PeerManager to upstream actors. | |||
| /// TODO: PeerManagerNotification now only exists in test code and should be deleted; probably use `ReceivedMessage` | |||
There was a problem hiding this comment.
After the mempool-side changes, this can now be removed completely?
network/framework/src/peer/mod.rs
Outdated
| ); | ||
| match self.upstream_handlers.get(&direct.protocol_id) { | ||
| None => { | ||
| // TODO: better label than "declined"? more like "garbage-in" |
| @@ -214,31 +262,52 @@ impl<TMessage> Stream for NetworkEvents<TMessage> { | |||
| } | |||
| } | |||
|
|
|||
| fn unix_micros() -> u64 { | |||
There was a problem hiding this comment.
Any reason not to use time service?
There was a problem hiding this comment.
the usage sites are pretty awkward to plumb that through to
JoshLind
approved these changes
Jul 19, 2024
network/framework/src/counters.rs
Outdated
| @@ -26,6 +26,7 @@ pub const RECEIVED_LABEL: &str = "received"; | |||
| pub const SENT_LABEL: &str = "sent"; | |||
| pub const SUCCEEDED_LABEL: &str = "succeeded"; | |||
| pub const FAILED_LABEL: &str = "failed"; | |||
| pub const UNKNOWN_LABEL: &str = "unk"; | |||
There was a problem hiding this comment.
nit: can we just make this unknown? 😄
| @@ -124,7 +122,7 @@ fn build_test_peer_manager( | |||
| peer_manager, | |||
| peer_manager_request_tx, | |||
| connection_reqs_tx, | |||
| hello_rx, | |||
| // hello_rx, | |||
| } | ||
| }, | ||
| NetworkMessage::RpcResponse(response) => { | ||
| NetworkMessage::RpcResponse(_) => { | ||
| // non-reference cast identical to this match case |
There was a problem hiding this comment.
What's the reason we can't just match this directly?
There was a problem hiding this comment.
the other branches work better with a match over a reference, and *response didn't work.
| } = message; | ||
| let dequeue_at = unix_micros(); | ||
| let dt_micros = dequeue_at - rx_at; | ||
| let dt_seconds = (dt_micros as f64) / 1000000.0; |
There was a problem hiding this comment.
nit: I'd move this to a helper:
let dequeue_delta_secs = calculate_dequeue_delta(rx_at);
...
fn calculate_dequeue_delta(rx_at: u64) -> u64 {
let dequeue_at = unix_micros();
let dt_micros = dequeue_at - rx_at;
(dt_micros as f64) / 1000000.0
}
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
✅ Forge suite
|
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.
Description
Remove several queues of indirection, move received messages directly from the reader thread of a peer to application code.
add inbound queue delay metric
aptos_network_inbound_queue_timeType of Change
Which Components or Systems Does This Change Impact?
How Has This Been Tested?
Local cluster tests. unit tests. forge cluster tests.
Key Areas to Review
Some unit tests in mempool were lost due to relying on layers of the network that are being phased out.
Some parts of the inbound path may still be there and unused, further code cleanup is possible.
Checklist