-
Notifications
You must be signed in to change notification settings - Fork 30
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
Optimize Single Message Processing in Mailbox #90
Conversation
c9eaa0e
to
988efea
Compare
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.
A few change requests, mostly about naming.
But, I'd also like to know why you prefer ArrayList over Mailbox.
@@ -57,7 +59,11 @@ public static Actor createActor() { | |||
private static final int THREAD_ID_SHIFT = 56; | |||
|
|||
/** Buffer for incoming messages. */ | |||
private Mailbox mailbox = Mailbox.createNewMailbox(16); | |||
private EventualMessage message; |
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.
Hm, perhaps this would be better firstMessage
, to make the semantics clear.
And, I think the comment could be extended to say that we optimize the case that there is one message, and only put everything afterwards into the moreMessages
data structure.
@@ -57,7 +59,11 @@ public static Actor createActor() { | |||
private static final int THREAD_ID_SHIFT = 56; | |||
|
|||
/** Buffer for incoming messages. */ | |||
private Mailbox mailbox = Mailbox.createNewMailbox(16); | |||
private EventualMessage message; | |||
private List<EventualMessage> moreMessages; |
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.
Why do you use an ArrayList here, instead of a Mailbox, any specific reason?
Mailbox was implemented to avoid copying the array of ArrayList when the mailbox becomes to small.
Perhaps a better name would be mailboxExtension
. The code below doesn't read well at the moment, from the perspective of terminology/word choice.
private EventualMessage message; | ||
private List<EventualMessage> moreMessages; | ||
|
||
private long sendTimeStamp; |
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.
better firstMessageTimeStamp
private List<EventualMessage> moreMessages; | ||
|
||
private long sendTimeStamp; | ||
private List<Long> moreSendTimeStamps; |
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.
Better mailboxExtensionTimeStamps
sendTimeStamp = System.currentTimeMillis(); | ||
} | ||
} else { | ||
sendMoreMessages(msg); |
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.
better appendToMailbox(msg)
/** | ||
* Is scheduled on the fork/join pool and executes messages for a specific | ||
* actor. | ||
*/ | ||
private static final class ExecAllMessages implements Runnable { | ||
private final Actor actor; | ||
private Mailbox current; | ||
private EventualMessage current; |
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.
better firstMessage
/** | ||
* Is scheduled on the fork/join pool and executes messages for a specific | ||
* actor. | ||
*/ | ||
private static final class ExecAllMessages implements Runnable { | ||
private final Actor actor; | ||
private Mailbox current; | ||
private EventualMessage current; | ||
private List<EventualMessage> moreCurrent; |
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.
better mailboxExtension
private EventualMessage current; | ||
private List<EventualMessage> moreCurrent; | ||
private long baseMessageId; | ||
private long sendTimeStamp; |
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.
and for the time stamps, same wording as above, please
int i = 0; | ||
if (size > 1) { | ||
for (EventualMessage msg : moreCurrent) { | ||
actor.logMessageBeingExecuted(msg); |
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.
this block looks identical to the handling of the first message. could this be a helper method?
2963bba
to
4191ede
Compare
4191ede
to
416eece
Compare
now uses SPromise like queuing
Fixed Validition.Philosophers resolving promise twice. Fixed RejectedExecutionException when sending messages while shutting down, throw ThreadDeath instead.
416eece
to
74ce078
Compare
Signed-off-by: Stefan Marr <git@stefan-marr.de>
Replaces the current Mailbox implementation with a approach similar to how SPromise handles chained Promises. This brings a performance improvement due to currently low mailbox utilization (many contain only one message).