Opened 7 years ago

Closed 7 years ago

#707 closed enhancement (fixed)

easy tweaks to reduce NTCP reader thread cpu usage

Reported by: Zlatin Balevsky Owned by: zzz
Priority: minor Milestone: 0.9.3
Component: router/transport Version: 0.9.1
Keywords: cpu Cc: zab@…, killyourtv@…
Parent Tickets: Sensitive: no

Description

Yourkit traces show that:


19% of the cpu time spend by the NTCP reader thread is used to parse an InetAddress from a String in NTCPAddress.isPubliclyRoutable . I looked through the RouterAddress class and it should be very easy to construct that object once and cache it.


6% is spent

public void setMessage(I2NPMessage msg) {
        _message = msg;
        if (msg != null) {
            _messageType = msg.getClass().getSimpleName(); // <-- here
            _messageTypeId = msg.getType();
            _messageId = msg.getUniqueId();
            _messageSize = _message.getMessageSize();
        }
    }

and that variable is only used in logging.


Note these percentages are valid only for the NTCP read thread. I'm focusing on that because it has direct impact on total throughput as described in #697

Subtickets

Change History (8)

comment:1 Changed 7 years ago by Zlatin Balevsky

Cc: zab@… added

comment:2 Changed 7 years ago by killyourtv

Cc: killyourtv@… added

comment:3 Changed 7 years ago by Zlatin Balevsky

Running dev router with a patch for this, but I'm not making it public to give killyourtv chance to come up with something on his own…

comment:4 Changed 7 years ago by zzz

Good ideas. Both also affect SSU, and the lookup caching would help in other lookup places (GeoIP, Blocklist, and ProfileOrganizer? are the main ones).

As you say, getSimpleName() is straightforward, although I'm surprised it's so expensive.

re: getHostByName(), we could do one of:

1) cache in RouterAddress? as you suggest
2) static LRU cache (LinkedHashMap?) in TransportImpl?
3) 1) combined with 2) as a 2nd-level cache, moved probably to net.i2p.util.Addresses

If we do 1) we can add a getPort() to go along with getIP() there

comment:5 Changed 7 years ago by zzz

Here's my stab at option 1)

#
# old_revision [fc53d15a3a2c93a520b003d4010db112d645c87d]
#
# patch "core/java/src/net/i2p/data/RouterAddress.java"
#  from [74ce0101cb5c6fe4fb5b0c723d7f48445cb3b162]
#    to [b9dc9fbc04bc303df8df34b72a0ac68a9dc4ce2d]
#
============================================================
--- core/java/src/net/i2p/data/RouterAddress.java	74ce0101cb5c6fe4fb5b0c723d7f48445cb3b162
+++ core/java/src/net/i2p/data/RouterAddress.java	b9dc9fbc04bc303df8df34b72a0ac68a9dc4ce2d
@@ -12,6 +12,8 @@ import java.io.OutputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
+import java.net.InetAddress;
+import java.net.UnknownHostException;
 import java.util.Collections;
 import java.util.Date;
 import java.util.Map;
@@ -36,7 +38,13 @@ public class RouterAddress extends DataS
     private Date _expiration;
     private String _transportStyle;
     private final Properties _options;
+    // cached values
+    private byte[] _ip;
+    private int _port;
 
+    public static final String PROP_HOST = "host";
+    public static final String PROP_PORT = "port";
+
     public RouterAddress() {
         _cost = -1;
         _options = new OrderedProperties();
@@ -141,6 +149,52 @@ public class RouterAddress extends DataS
     }
     
     /**
+     *  Caching version of InetAddress.getByName(getOption("host")).getAddress(), which is slow.
+     *  Caches numeric host names only.
+     *  Will resolve but not cache resolution of DNS host names.
+     *
+     *  @return IP or null
+     *  @since 0.9.2
+     */
+    public byte[] getIP() {
+        if (_ip != null)
+            return _ip;
+        byte[] rv = null;
+        String host = _options.getProperty(PROP_HOST);
+        if (host != null) {
+            try {
+                rv = InetAddress.getByName(host).getAddress();
+                if (host.replaceAll("[0-9\\.]", "").length() == 0 ||
+                    host.replaceAll("[0-9a-fA-F:]", "").length() == 0) {
+                    _ip = rv;
+                }
+            } catch (UnknownHostException uhe) {}
+        }
+        return rv;
+    }
+    
+    /**
+     *  Caching version of Integer.parseInt(getOption("port"))
+     *  Caches valid ports 1-65535 only.
+     *
+     *  @return port or 0 if invalid
+     *  @since 0.9.2
+     */
+    public int getPort() {
+        if (_port != 0)
+            return _port;
+        String port = _options.getProperty(PROP_PORT);
+        if (port != null) {
+            try {
+                int rv = Integer.parseInt(port);
+                if (rv > 0 && rv <= 65535)
+                    _port = rv;
+            } catch (NumberFormatException nfe) {}
+        }
+        return _port;
+    }
+
+    /**
      *  @throws IllegalStateException if was already read in
      */
     public void readBytes(InputStream in) throws DataFormatException, IOException {

comment:6 Changed 7 years ago by zzz

And here's my stab at option 2) (includes some cleanup of the other static cache in there)

#
# old_revision [fc53d15a3a2c93a520b003d4010db112d645c87d]
#
# patch "router/java/src/net/i2p/router/transport/TransportImpl.java"
#  from [c5df1d3c80b8caf2cb84fc3f489b9509fdfe4a75]
#    to [6b0f50a4f8d227ccd2b5b2365d33c2e3b9aa2226]
#
============================================================
--- router/java/src/net/i2p/router/transport/TransportImpl.java	c5df1d3c80b8caf2cb84fc3f489b9509fdfe4a75
+++ router/java/src/net/i2p/router/transport/TransportImpl.java	6b0f50a4f8d227ccd2b5b2365d33c2e3b9aa2226
@@ -10,11 +10,14 @@ import java.io.Writer;
 
 import java.io.IOException;
 import java.io.Writer;
+import java.net.InetAddress;
+import java.net.UnknownHostException;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.Date;
 import java.util.HashMap;
 import java.util.Iterator;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
@@ -22,6 +25,7 @@ import java.util.concurrent.ConcurrentHa
 import java.util.Vector;
 import java.util.concurrent.ConcurrentHashMap;
 
+import net.i2p.data.DataHelper;
 import net.i2p.data.Hash;
 import net.i2p.data.RouterAddress;
 import net.i2p.data.RouterIdentity;
@@ -53,8 +57,22 @@ public abstract class TransportImpl impl
     private final Map<Hash, Long>  _unreachableEntries;
     private final Set<Hash> _wasUnreachableEntries;
     /** global router ident -> IP */
-    private static final Map<Hash, byte[]> _IPMap = new ConcurrentHashMap(128);
+    private static final Map<Hash, byte[]> _IPMap;
+    /** textual IP to bytes, because InetAddress.getByName() is slow */
+    private static final Map<String, byte[]> _IPAddress;
 
+    static {
+        long maxMemory = Runtime.getRuntime().maxMemory();
+        if (maxMemory == Long.MAX_VALUE)
+            maxMemory = 96*1024*1024l;
+        long min = 512;
+        long max = 8192;
+        // 1024 nominal for 128 MB
+        int size = (int) Math.max(min, Math.min(max, 1 + (maxMemory / (128*1024))));
+        _IPMap = new LHM(size);
+        _IPAddress = new LHM(size);
+    }
+
     /**
      * Initialize the new transport
      *
@@ -585,14 +603,64 @@ public abstract class TransportImpl impl
     }
 
     public void setIP(Hash peer, byte[] ip) {
-        _IPMap.put(peer, ip);
-        _context.commSystem().queueLookup(ip);
+        byte[] old;
+        synchronized (_IPMap) {
+            old = _IPMap.put(peer, ip);
+        }
+        if (!DataHelper.eq(old, ip))
+            _context.commSystem().queueLookup(ip);
     }
 
     public static byte[] getIP(Hash peer) {
-        return _IPMap.get(peer);
+        synchronized (_IPMap) {
+            return _IPMap.get(peer);
+        }
     }
 
+    /**
+     *  Caching version of InetAddress.getByName().getAddress(), which is slow.
+     *  Caches numeric host names only.
+     *  Will resolve but not cache DNS host names.
+     *
+     *  @param host if null returns null
+     *  @return IP or null
+     *  @since 0.9.2
+     */
+    public static byte[] getIP(String host) {
+        if (host == null)
+            return null;
+        byte[] rv;
+        synchronized (_IPAddress) {
+            rv = _IPAddress.get(host);
+        }
+        if (rv != null)
+            return rv;
+        try {
+            rv = InetAddress.getByName(host).getAddress();
+        } catch (UnknownHostException uhe) {
+            return null;
+        }
+        if (host.replaceAll("[0-9\\.]", "").length() == 0 ||
+            host.replaceAll("[0-9a-fA-F:]", "").length() == 0) {
+            synchronized (_IPAddress) {
+                _IPAddress.put(host, rv);
+            }
+        }
+        return rv;
+    }
+
+    /**
+     *  @since 0.9.2
+     */
+    static void clearCaches() {
+        synchronized(_IPMap) {
+            _IPMap.clear();
+        }
+        synchronized(_IPAddress) {
+            _IPAddress.clear();
+        }
+    }
+
     /** @param addr non-null */
     public static boolean isPubliclyRoutable(byte addr[]) {
         if (addr.length == 4) {
@@ -615,4 +683,18 @@ public abstract class TransportImpl impl
             return false;
         }
     }
+
+    private static class LHM<K, V> extends LinkedHashMap<K, V> {
+        private final int _max;
+
+        public LHM(int max) {
+            super(max, 0.75f, true);
+            _max = max;
+        }
+
+        @Override
+        protected boolean removeEldestEntry(Map.Entry<K, V> eldest) {
+            return size() > _max;
+        }
+    }
 }

comment:7 Changed 7 years ago by zzz

Milestone: 0.9.3
Status: newaccepted

OutNetMessage? fixed in 0.9.1-23-rc, thanks to dg, kytv, and zab for their help.

For InetAddress? I'm leaning toward option 3), to be implemented in the i2p.i2p.zzz.test branch and propped for 0.9.3.

comment:8 Changed 7 years ago by zzz

Resolution: fixed
Status: acceptedclosed

Option 3) propped as 0.9.2-1.

Note: See TracTickets for help on using tickets.