Allow subsystems using priority channels #77
Conversation
|
The referenced issue outlines an approach using a generic function, I assume that doesn't compile? Otherwise I'd favor that over the current approach. |
@drahnr Thank you for the review! |
| self. #channel_name .send( | ||
| #support_crate ::make_packet(signals_received, #maybe_boxed_send) | ||
| ).await.map_err(|_| stringify!( #channel_name )) | ||
| if matches!(priority, ChannelsOutPriority::High) { |
There was a problem hiding this comment.
Should you add the same logic for try_send for completeness ?
| @@ -635,9 +635,10 @@ pub(crate) fn impl_feature_gated_items( | |||
| #( | |||
| let (#channel_name_tx, #channel_name_rx) | |||
| = | |||
| #support_crate ::metered::channel::< | |||
| #support_crate ::metered::channel_with_priority::< | |||
There was a problem hiding this comment.
Nit: Should we make this configurable per-subsystem ?
There was a problem hiding this comment.
We can, but I think we'll have little to gain considering the number of subsystems.
There was a problem hiding this comment.
At least from Polkadot perspective, we only need the priority channels for the networking subsystems, so it makes sense to have it configurable per subsystem.
There was a problem hiding this comment.
I'd be in favor of having it per subsystem or for everything. We will poll twice as many channels, in the empty priority channel case, which will be the default for most I presume.
|
Is it expected to ever have the priority defined at runtime? My hunch is no. I'd hence would like to see something like |
|
Note that I consider this solution an OK solution. Since at any point in time a peerview is atomic per peer, we could do an aggregate state and only share a reference update to it. This is out of scope for |
| /// Tries to send a direct priority message to some other `Subsystem`, routed based on message type. | ||
| /// This method is useful for cases where the message queue is bounded and the message is ok | ||
| /// to be dropped if the queue is full. If the queue is full, this method will return an error. | ||
| /// This method is not async and will not block the current task. |
There was a problem hiding this comment.
It doesn't make much sense to drop priority messages.
There was a problem hiding this comment.
Make sense. Does it mean that we should not have the try_priority_send at all?
That sounds like a good idea, making things more generic, but multiple priorities means multiple underlying channels which si what #8 proposes. At this point I am not convinced that having so many priority levels is actually that useful for Polkadot. @drahnr do you have any practical usecase in mind that needs this right now ? If not, I guess we can just be happy with this very simple thing with just 2 priorities. |
I like this approach in principle: moving this sync mechanism out of band. But wouldn't it actually perform worse by having so many (up to 1k) notification futures, compared to what we want to do ? |
Depends on the update frequency, but I think given the context the back of the envelope will proof you're right @sandreim |
I smell a custom queue with aggregatable values, and addressable by keys (here peer Ids) would be what could make this work efficiently, while also limiting the length to the number of peer views. </nerd snipe> |
| ).await; | ||
| } | ||
|
|
||
| async fn priority_send_message(&mut self, msg: OutgoingMessage) |
There was a problem hiding this comment.
If we use the generic with default value, we could avoid this additional public API
There was a problem hiding this comment.
Do you mean we should make priority markers public? Considering, that I couldn't set defaults it's a bit inconvenient to write them every time.
There was a problem hiding this comment.
Hmm, in that case I propose a slighly different API send_message and send_message_with_priority<Priority> which is called by send_message.
| ) | ||
| ), | ||
| ChannelsOutPriority::Normal, | ||
| ).map_err(|err| match err { |
There was a problem hiding this comment.
Same as above, avoid the code duplication of .map_err, here an intermediate inline fn would help
| @@ -81,14 +89,21 @@ pub(crate) fn impl_channels_out_struct(info: &OrchestraInfo) -> Result<proc_macr | |||
| &mut self, | |||
| signals_received: usize, | |||
There was a problem hiding this comment.
I'd like to see a unit test regarding how signals_received is handled and how that effects the priority vs non-priority messages.
There was a problem hiding this comment.
Added signals to tests
Co-authored-by: Bernhard Schuster <[email protected]>
|
@AndreiEres #77 (comment) is really my remaining concern, everthing else LGTM |
Thank you, that's updated here 03fae37 |
Currently subsystems use
MeteredReceiver { priority_channel: None }. In this PR we add priority receiver to itMeteredReceiver { priority_channel: Some(Receiver) }and additional sending interface. It should help us prioritize some messages in subsystems, likePeerViewChange(see paritytech/polkadot-sdk#577).