-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Re-consider separate, ordered mailbox #94
Comments
In theory, yes, but how do you propose to handle overflows? This was the main issue I ran into thinking about this option |
Reset it every time the queue is empty perhaps? Am I too naive in saying that we could also just not handle them? The "problem" would be a slight out of order handling of same-priority messages. From experience, actors receive messages from various addresses concurrently, so the exact order in which they will be handled is unclear anyway so this wouldn't even be noticeable. But if we accept this behaviour in an edge case, we might also just accept it all the time :D |
Expanding more on this: Usage of In other words, I would argue that someone using We may want to debate the name of that function again because it doesn't communicate that right away, only conceptually. |
If we would implement #92 (comment), then this issue would go away because the |
@Restioson What is your latest position on this? I am gonna try and summarize our current knowledge: Separate handling of messages with default priority has the advantage that they will be delivered to the actor in the order they have been sent even if the user uses I see two problems with this feature:
From a user perspective, the feature is incomplete (only applies to default priority) and from a maintainer perspective, it adds a fair bit of complexity to the codebase which is why I'd like to remove it. |
Friendly ping @Restioson :) |
I propose to merge |
Hi, sorry for getting to this so late. I am back from holiday now :) I think the main thing around having a unified mailbox would be that we would need to find some way to implement the two features (priority & no priority) in one fell swoop while still retaining message ordering, as there are some existing usecases which use message ordering. We could try the VecDeque solution and see what the performance/memory usage impact would be versus the current solution. I do anticipate it would use more memory and allocate more, needing one new VecDeque per priority that the user is using. Maybe there are other priority queue structures that also preserve insertion order? |
As I tried to argue above, message ordering is guaranteed if you await the response of the message. If you say Can you explain what ordering you want to retain? Also, I think we shouldn't look at it as no priority vs priority. Priority 0 is just another priority and not no priority :) |
Here are meeting "notes" from an out-of-band meeting: https://hackmd.io/@AeK4eNTZTGGJXAT4cxS_Zw/r1MvCrHCi. The conclusion of the meeting is:
Some naming suggestions that came up during the meeting:
An idea that just came to my mind:
What may not be intuitive is that people might think "what executor / thread is this going to be spawned on?" when in reality it isn't actually "spawned" as such but just queued, albeit in a non-FIFO queue. |
With #85, we are introducing 3 mailboxes: ordered, prioritized and broadcast.
Ordered exists because
BinaryHeaps
don't preserve insertion order and thus, messages with the same priority (like 0 as the default) could be processed in a different order, assuming they are used withsplit_receiver
and the processing thus happens async.I am claiming that this is way too much of a detail to expose to the user (which has to eventually learn that there are 3 mailboxes) so my suggestion would be to:
a) Use a data structure that preserves insertion order
b) Accept the re-ordering of messages
I am not data structure expert but couldn't we for example also use an atomic counter to tag each incoming message and sort the heap based on priority and message counter? That should preserve the sending order even among messages with the same priority.
The text was updated successfully, but these errors were encountered: