Spending loads of time in already optimized class -> Anything wrong?

Viewed 41

I have a simple 2D Point class that I wrote myself. My Points are immutable and I need to create loads of them, for memory efficiency I created a cache to fetch those that are already there.

Total unique points I use during the process around 100_000. I need to fetch loads of them multiple times.

While profiling my app I noticed that most of the time is spent in this class.

I wonder if I did anything tremendously stupid or the time spent really is because I need to create so many points. Can I optimize this class any further? (And yes - I need the concurrent access)

This is the code:

public class Point implements Comparable<Point> {
private static final Map<Integer, Map<Integer, Point>> POINT_CACHE = new ConcurrentHashMap<>();
private static final boolean USE_CACHE = true;

public final int row;
public final int column;


private int hashCache = -1;


public static Point newPoint(int row, int column){
    if(!USE_CACHE) return new Point(row,column);
    return POINT_CACHE.computeIfAbsent(row, k -> new ConcurrentHashMap<>()).computeIfAbsent(column, v -> new Point(row , column));
}

public static Point newPoint(Point point){
    return newPoint(point.row,point.column);
}

protected Point(int row, int column) {
    this.row = row;
    this.column = column;
}

@Override
public boolean equals(Object o) {
    if (this == o) return true;
    if (o == null || getClass() != o.getClass()) return false;
    Point point = (Point) o;
    return row == point.row && column == point.column;
}

@Override
public int hashCode() {
    //Assuming the matrix is less than 65k x 65k, this will return unique hashes
    if (hashCache == -1) hashCache = (row << 16) | column;
    return hashCache;

    //return Objects.hash(row, column);

}

//Getter
}

Profiler Result

1 Answers

Yes, ConcurrentHashMap operations are expensive.

They also use a lot of memory. Even more memory than a regular HashMap.

So unless you get an average cache hit rate over 90%, you are probably going to use more memory in the ConcurrentHashMap objects than you are saving in not creating duplicate Point objects.

The other thing to observe is since your Point objects have int values as their row and column attributes, you could simply encode each distinct point as a long ... and entirely remove the need for the Point objects and the cache.

Related