[jboss-jira] [JBoss JIRA] Commented: (JGRP-1043) UNICAST: high contention

[ https://jira.jboss.org/jira/browse/JGRP-1043?page=com.atlassian.jira.plug... ] Brian Stansberry commented on JGRP-1043: ---------------------------------------- Another related contention point I see when profiling is for AckReceiverWindow.msgs. Calls the lead to acquiring that lock come from UNICAST.handleDataReceived(). Looking at that I see a couple possible improvements: 1) if(win.smallerThanNextToRemove(seqno)) sendAck(msg.getSrc(), seqno); The win.smallerThanNextToRemove(seqno) call has to acquire the AckReceiverWindow.msgs lock. My interpretation of this is that it's purpose is to send an ack to a message retransmission, where the original message has already been received. Why would this be necessary? A) ACK of original message didn't arrive in time to cancel retranmission B) ACK of original message got lost. If B) occurred, the retransmission will go on forever, so to prevent that we have this code to ack the retransmission. Now, why would win.smallerThanNextToRemove(seqno) return true? a) It's a retransmission, i.e. the case described above. In that case "added" from the earlier "boolean added=win.add(seqno, msg);" will be false. So, checking for !added would handle this. b) Another thread is removing messages from the window and between when the thread adding the message called win.add(seqno, msg) and later called if(win.smallerThanNextToRemove(seqno)), the "removing thread" already removed it. In that case the "removing thread" will ack the message and the adding thread also acking it is redundant. Bottom line, I think the above can be changed to the following, eliminating contention on AckReceiverWindow.msgs: if(!added) { // Must be a retransmission of a msg we already acked. Ack it to ensure retransmission task // is cancelled, in case our ack of original message got dropped sendAck(msg.getSrc(), seqno); } 2) if(!added && !win.hasMessagesToRemove()) { // no ack if we didn't add the msg (e.g. duplicate) return; } I get the feeling this code is leftover cruft, as the "// no ack if we didn't add the msg (e.g. duplicate)" comment doesn't fit the surrounding logic that determines whether there's an ack. The effect of this block is just to cause the thread to return and not drop into the section where it checks if it should become the thread that removes messages. That check is now quite cheap, so why not just remove this? This one is no big deal in the normal case, since added will usually be true so the win.hasMessagesToRemove() call won't be made. But in the overload case where there are starting to be a lot of retransmissions, the win.hasMessagesToRemove() call will start being made, adding to the contention on the lock. -- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: https://jira.jboss.org/jira/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira