tonarino / actor

A minimalist actor framework aiming for high performance and simplicity.
MIT License
39 stars 6 forks source link

Support priority channels #22

Closed skywhale closed 3 years ago

skywhale commented 3 years ago

Actors currently have a single ingress channel, and messages are all treated with the same priority. Sometimes we want to send a mix of important and non-important data to an actor, and we have seen that important data is dropped because non-important data is busy and occupies the channel.

We have send_if_not_full() to work around the issue, but it's a bit hacky. I want to introduce a more proper priority concept to the actor channels.

enum Priority {
  High,
  Low,
}

impl<M> Recipient<M> {
  fn send_with_priority(&self, message: N, priority: Priority) { ... }
}
strohel commented 3 years ago

This sounds reasonable. If the number of priorities is fixed, and all would have the same message payload, this should be hard to add by adding another channel on top of control_rx, message_rx.

Possible caveat: From crossbeam::select! macro that we use:

If multiple operations are ready at the same time, a random one among them is selected.

But perhaps this would be already sufficient, if the high-priority channel would be lower-volume.

The lib seems to be well usable even without prio channels, so this is not necessarily Before Publish.

skywhale commented 3 years ago

Thank you @strohel. I agree it's not high-priority.

strohel commented 3 years ago

Possible caveat: From crossbeam::select! macro that we use:

If multiple operations are ready at the same time, a random one among them is selected.

It seems that replacing crossbeam by flume may help with this, see https://github.com/tonarino/actor/issues/11#issuecomment-783729799.

skywhale commented 3 years ago

A relevant comment from @strohel: https://github.com/tonarino/actor/pull/38#issuecomment-786597095

skywhale commented 3 years ago

API-wise, do we want to bind the priority to the message type, or shall we let the sender decide, i.e. bind the priority to the message instance? The sender-decided priority is functionally a superset of message-based priority. That alone doesn't make it a better choice, but I haven't found strong reasons to make the API more opinionated either (thoughts welcome). I've basically pushed that decision to the point when #22 is implemented, though this currently leans on the sender-decided priority side (that was simply a smaller change).

I'm also leaning towards implementing per-instance priority for simplicity. Let me take this issue. I'm curious :)

At a meta level, though, this is a question of whether priority should be decided by senders or by receivers. I think receivers are usually in a better position to decide priority, as it knows all the incoming message types, owns the actual logic of processing them, and the need for priority arises from its possibly limited capacity. A sender may not necessarily know to which actor it's sending a message, due to the Recipient abstraction, so it may need to decide priority in a partial context, not knowing what other messages it is competing against. In that case, priority serves more as "a hint".

strohel commented 3 years ago

We can even have both approaches:

enum Priority { Normal, High }

trait DefaultPriority {
    const PRIORITY: Priority = Priority::Normal;
}

impl Receiver<M> {
    send(&self, message: M) where M: DefaultPriority {
        self.send_with_priority(message, M::PRIORITY)
    }

    send_with_priority(&self, message: M) { ... }
}

I don't know whether that's a good idea, though. It would be nice if bare send(message) would assign Normal priority for all types except the ones that have it explicitly overridden. The example above requires at least impl DefaultPriority for T {} for every T that wants to go into bare send().

skywhale commented 3 years ago

Your comment made me search for specialization in Rust, and I landed here https://github.com/rust-lang/rust/issues/31844. It doesn't look like it'll be stable in the near future :(