Opened 2 months ago

Last modified 5 weeks 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)

2701.patch (25.6 KB) - added by Zlatin Balevsky 2 months ago.
Replace _stateLock with an AtomicReference?

Download all attachments as: .zip

Change History (5)

comment:1 Changed 2 months ago by zzz

Cc: Zlatin Balevsky added
Status: newopen

I'll look at 2nd bullet in OP. CC'ing zab for advice in first bullet

comment:2 Changed 2 months ago by Zlatin Balevsky

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.

Changed 2 months ago by Zlatin Balevsky

Attachment: 2701.patch added

Replace _stateLock with an AtomicReference?

comment:3 Changed 2 months ago by zzz

followup and an alternative proposal at http://zzz.i2p/topics/2860 , posted there during an extended downtime for trac

comment:4 Changed 5 weeks ago by zzz

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;
                     }
                 }
             }
Note: See TracTickets for help on using tickets.