views:

489

answers:

3

How can I optimize this code ? I made IPFilter and I need to optimize it.

package com.ipfilter;

import java.util.HashMap;
import java.util.Map;

/**
 *      IPFilter
 * 
 *      Loads given IP addresses to memory, so you can  easily check if ip addres has been blocked
 */

public class IPFilter {
        private Map<Integer, IPFilter> filter = new HashMap<Integer, IPFilter>();

        /**
         * Convert String ip address to Integer array and then calls add ip method
         * @param ip
         * @return
         */
        public void addIP(String ip)
        {
                int[] numbers = convert(ip);
                addIP(numbers, 0);
        }

        /**
         * Convert String ip address to Integer array
         * @param ip
         * @return
         */
        private int[] convert(String ip) {
                String[] strings = ip.split("\\.");
                int[] numbers = new int[strings.length];
                for(int i = 0; i < strings.length; i++)
                {
                        numbers[i] = Integer.parseInt(strings[i]);
                }
                return numbers;
        }

        /**
         * Add ip address to memory
         * @param ip
         * @param level
         */
        private void addIP(int[] ip, int level) {
                if(level < ip.length)
                {
                        if (filter.containsKey(ip[level])) {
                                filter.get(ip[level]).addIP(ip, level + 1);
                        } else {
                                filter.put(ip[level], new IPFilter());
                                filter.get(ip[level]).addIP(ip, level + 1);
                        }
                }
        }

        /**
         * Checks if ip address is in filter
         * @param ip
         * @return
         */
        public boolean isBlocked(String ip)
        {
                return isBlocked(filter, convert(ip), 0);
        }

        /**
         * Check if ip address is blocked
         * @param list
         * @param ip
         * @param level
         * @return
         */
        private boolean isBlocked(Map<Integer, IPFilter> list, int[] ip, int level)
        {
                if(list.containsKey(ip[level]))
                {
                        if(level < ip.length - 1)
                        {
                                return isBlocked(list.get(ip[level]).getList(), ip, level + 1);
                        }
                        else
                        {
                                return true;
                        }
                }
                else
                {
                        return false;
                }
        }       

        /**
         * Getter for list
         * @return
         */
        protected Map<Integer, IPFilter> getList() {
                return filter;
        }
}
+8  A: 

Profile it through some typical use cases and use that data to find out where the performance bottlenecks are. THEN, optimize that code.

Without actually knowing where the performance issue is, you could spend a lot of time and effort saving microseconds.

Ben S
+2  A: 

I'm not sure about what exactly are you trying to optimize. However, you have a couple of containsKey followed by get. A possible optimization is to use get and compare to null. For example, instead of:

 if (filter.containsKey(ip[level])) {
      filter.get(ip[level])
 }

Do the following:

 IPFilter value = filter.get(ip[level]);
 if (value != null) {
      value.addIp(...);
 }

But I think the best tip I can give you is: use a profiler. If you are using Eclipse, check TPTP.

JG
+1  A: 

Not for nothing, but an IP address is a 4 byte integer as it's usually implemented. Why not encode to that, and just let:

int ipSrc = convertIpToInt(String ip); 
if ( ipSrc == ipDest ) { 
  /// 
}

be your comparison.

And for IPv6, you could use use a long.

Then again, what I'd probably do is use java.net.Inet4Address and store them in a Set.

Since you're already using a Map, why not try out a simplistic approach? A halfway smart implementation of Inet4Address.equals() would do an integer comparison, and not a string comparison.

Of course this method breaks down if you want to do wildcarding... :-/

Chris Kaminski
I don't think it breaks down for wildcarding - at least begins_with and ends_with can be handled with bit masking operations in the hashCode computation. Using integer and/or long would be way faster than the original algorithm.
Kevin Day