Opened 15 months ago
Last modified 14 months ago
#2647 new enhancement
PeerStata.java: Do not iterate within finishmessages()
Reported by: | jogger | Owned by: | zzz |
---|---|---|---|
Priority: | major | Milestone: | undecided |
Component: | router/transport | Version: | 0.9.43 |
Keywords: | Cc: | ||
Parent Tickets: | Sensitive: | no |
Description
This looks like an important function, but those iterations do next to nothing and consume lots of CPU. The message completion code is redundant because message completion ist done by the ACK logic. Same goes for the push count check, which can never strike because we time out much earlier.
So the remaining code could be moved elsewhere or - if one was to keep the function - it would be sufficient not to iterate, but just to check the first state and possibly fail that out.
Subtickets
Change History (2)
comment:1 Changed 14 months ago by
comment:2 Changed 14 months ago by
I have done the research = code analysis and have sucessfully run without iterations using the following on UDP only routers since more than a month.
public int finishMessages(long now) { // short circuit, unsynchronized if (_outboundMessages.isEmpty()) return _outboundQueue.size(); if (_dead) { dropOutbound(); return 0; } OutboundMessageState state = null; synchronized (_outboundMessages) { Iterator<OutboundMessageState> iter = _outboundMessages.iterator(); if (iter.hasNext()) { // guard against race state = iter.next(); if (state.isExpired(now)) iter.remove(); else state = null; } // end iterating over outbound messages } if (state != null) { if (state.getPushCount() > 1) _retranscount--; OutNetMessage msg = state.getMessage(); if (msg != null) { _transport.failed(state); } else { // it can not have an OutNetMessage if the source is the // final after establishment message if (_log.shouldLog(Log.WARN)) _log.warn("Unable to send a direct message: " + state); } } // no need to update _nextSendTime as we will immediately // either remove() or call allocateSend() return _outboundMessages.size() + _outboundQueue.size(); }
That's an interesting theory. The important thing here is calling _transport.succeeded() and _transport.failed(), but it does appear that those are called elsewhere too. Not clear if we're doing duplicate calls, or all the code in finishMessages() is just a failsafe, or what.
Needs further research.