-
Notifications
You must be signed in to change notification settings - Fork 5
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
feat: Use unbounded channel pattern for mailbox to improve performance #187
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really appreciate the PR. However, I suggest that you add a new mailbox that implements the Mailbox interface.
I am also open if you are suggesting changing the Mailbox interface.
// Mailbox defines the actor mailbox. | ||
// It is responsible for storing messages that are to be processed by the actor and is an | ||
// actor itself. It is also the policy for how messages are enqueued and dequeued from the mailbox. | ||
// Any implementation should be a thread-safe FIFO | ||
type Mailbox interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't change the Mailbox interface. It is there to allow anyone to implement a custom mailbox. For instance, you can add your custom implementation using this interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I would like to know whether you are suggesting changing the mailbox interface.
The interface is there to allow a custom mailbox implementation.
@AlexanderTar Also, once you complete your new mailbox implementation, I suggest running the benchmark code to see whether there is a performance improvement. |
@@ -1,6 +1,8 @@ | |||
module github.com/tochemey/goakt | |||
|
|||
go 1.20 | |||
go 1.21 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I suggest not to change the go version
@AlexanderTar I run your pr locally to check the performance gain and this is the result: The same bench mark run on the main branch: I think there is no much difference comparing the two. I am sure there are places in the code we can improve upon. |
@Tochemey thanks for looking into this so quickly. I admit, I didn't run benchmarks test, so didn't realise the benefits are negligible. I do, however, suggest we change Mailbox interface slightly by introducing |
fba98c1
to
ec52109
Compare
Thanks for this observation. That makes sense. I will take a look at the PR. |
@AlexanderTar I merged the other PR. I am going to close this PR. Feel free to bring out any suggestion that will enhance performance 😉 |
Inspired by https://github.com/golang-design/chann but using thread-safe buffer underneath