Opened 10 months ago
Last modified 6 months ago
#2701 open enhancement
remove NTCP bottlenecks (some)
Reported by: | jogger | Owned by: | zzz |
---|---|---|---|
Priority: | minor | Milestone: | undecided |
Component: | router/transport | Version: | 0.9.46 |
Keywords: | Cc: | Zlatin Balevsky | |
Parent Tickets: | Sensitive: | no |
Description
While profiling NTCP I found some areas eating lots of CPU. Following are easy to fix:
- Syncing on a method that retrieves just one field does nothing. Many occurrences in the transport, NTCPConnection.isEstablished() eats most.
- _conlock also blocks a lot. Suggest using the double check idiom in NTCPTransport.outboundMessageReady()
con = _conByIdent.get(ih); if (con == null) { synchronized (_conLock) { con = _conByIdent.get(ih); if (con == null) {
Subtickets
Attachments (1)
Change History (6)
comment:1 Changed 10 months ago by
Cc: | Zlatin Balevsky added |
---|---|
Status: | new → open |
comment:2 Changed 10 months ago by
NTCPConnection.isEstablished() eats most.
as far as I can see that delegates to either OutboundNTCP2State.isComplete or EstablishBase.isComplete()
The one for outbound does not have any logic that requires a full monitor, so it can be replaced with volatile easily.
The one in EstablishBase does have some logic which is easier to do with a monitor, but with a little creativity could be replaced with an AtomicReference which would be much cheaper to read.
comment:3 Changed 10 months ago by
followup and an alternative proposal at http://zzz.i2p/topics/2860 , posted there during an extended downtime for trac
comment:4 Changed 9 months ago by
re: 2nd bullet, I've been testing it for a month, no issues. As recommended by OP, it double-checks that the con is NOT in the map, so there won't be dups. There's no lock that the con IS in the map, so in rare cases we could end up transmitting on a con that is closed and about to be removed. But that can happen anyway.. see locking in removeCon().
So here's the diff I've been testing for a month. Looking for comments on it, and also on the diff linked in the above comment for the 1st bullet from OP.
# # old_revision [39905af4247c2d16678a63ca36025c13a1cd3237] # # patch "router/java/src/net/i2p/router/transport/ntcp/NTCPTransport.java" # from [a6961bb8f3db3fac9a3fb38a627ab039d494fc8c] # to [ce5e8f9b2ade2214aa9d069f5eab39a1b7e922fb] # ============================================================ --- router/java/src/net/i2p/router/transport/ntcp/NTCPTransport.java a6961bb8f3db3fac9a3fb38a627ab039d494fc8c +++ router/java/src/net/i2p/router/transport/ntcp/NTCPTransport.java ce5e8f9b2ade2214aa9d069f5eab39a1b7e922fb @@ -359,35 +359,38 @@ public class NTCPTransport extends Trans RouterInfo target = msg.getTarget(); RouterIdentity ident = target.getIdentity(); Hash ih = ident.calculateHash(); - NTCPConnection con = null; int newVersion = 0; boolean fail = false; - synchronized (_conLock) { - con = _conByIdent.get(ih); - if (con == null) { - RouterAddress addr = getTargetAddress(target); - if (addr != null) { - newVersion = getNTCPVersion(addr); - if (newVersion != 0) { - try { - con = new NTCPConnection(_context, this, ident, addr, newVersion); - establishing(con); - //if (_log.shouldLog(Log.DEBUG)) - // _log.debug("Send on a new con: " + con + " at " + addr + " for " + ih); - // Note that outbound conns go in the map BEFORE establishment - _conByIdent.put(ih, con); - } catch (DataFormatException dfe) { - if (_log.shouldWarn()) - _log.warn("bad address? " + target, dfe); + // _conLock contention, check outside lock first + NTCPConnection con = _conByIdent.get(ih); + if (con == null) { + synchronized (_conLock) { + con = _conByIdent.get(ih); + if (con == null) { + RouterAddress addr = getTargetAddress(target); + if (addr != null) { + newVersion = getNTCPVersion(addr); + if (newVersion != 0) { + try { + con = new NTCPConnection(_context, this, ident, addr, newVersion); + establishing(con); + //if (_log.shouldLog(Log.DEBUG)) + // _log.debug("Send on a new con: " + con + " at " + addr + " for " + ih); + // Note that outbound conns go in the map BEFORE establishment + _conByIdent.put(ih, con); + } catch (DataFormatException dfe) { + if (_log.shouldWarn()) + _log.warn("bad address? " + target, dfe); + fail = true; + } + } else { fail = true; } } else { + // race, RI changed out from under us + // call afterSend below outside of conLock fail = true; } - } else { - // race, RI changed out from under us - // call afterSend below outside of conLock - fail = true; } } }
comment:5 Changed 6 months ago by
Change referenced in comment 3 above in 87f95291ec636f603939c9fd2f5220045a049fa7 to be 0.9.46-8
I'll look at 2nd bullet in OP. CC'ing zab for advice in first bullet