[TECH DEBT] Improve ConnectedNetwork - recv_message
#3671
Conversation
| MessageKind::Data(message) => match message { | ||
| DataMessage::SubmitTransaction(transaction, _) => { | ||
| broadcast_event( | ||
| Event { | ||
| view_number: TYPES::Time::new(1), | ||
| event: EventType::ExternalMessageReceived(data), | ||
| }, | ||
| &self.external_event_stream, | ||
| Arc::new(HotShotEvent::TransactionsRecv(vec![transaction])), | ||
| &self.internal_event_stream, |
There was a problem hiding this comment.
I guess this was the whole reason for the batching of messages, to collect transactions. Doesn't seem worth keeping to me
There was a problem hiding this comment.
I think it was also to potentially increase throughput by decreasing context switching, but we were not really making use of it in the way we could have. If we end up having better facilities in later for batching (e.g. a message type that is a vec of messages), I might consider reintroducing some form of batching
| @@ -106,7 +108,7 @@ impl<TYPES: NodeType> CombinedNetworks<TYPES> { | |||
|
|
|||
| Self { | |||
| networks, | |||
| message_cache: Arc::new(RwLock::new(LruCache::new( | |||
| message_cache: Arc::new(PlRwLock::new(LruCache::new( | |||
There was a problem hiding this comment.
Why the switch to parking lot library?
There was a problem hiding this comment.
The original was an async lock, which is slower and we would really only need if we were holding across .await points.
parking_lot is kind of std++ when it comes to RwLocks/Mutexes, it performs better in most cases and is my goto for a sync lock
Difference is probably negligible, but I was already in there
Is part of #3672
Closes #2558
This PR:
1. Changes
recv_msgstorecv_message, allowing us to receive a single message for each call.This simplifies the receiving layer and allows us to remove the
async_sleepthat we were using.2. Improves the message receiving loop
Doesn't do the unfold/pin/stream anymore. Instead, selects between the shutdown stream and our
recv_messagesfuture.3. Improves performance of the
combined_networkloop by:Changing it to use a synchronous lock. It will also now keep going until it receives something deserializable, instead of returning an empty message
This PR does not:
Make any further improvements to
ConnectedNetworkKey places to review:
combined_network.rsnetwork.rsforhandle_message()Testing
Running the examples locally I get a 10x speedup (only tested with
combinedso far) due to the rewrite of the loop and the removal of the sleep. Now, thecombined_networkloop will keep receiving until it receives an uncached message instead of sleeping