CODEWITHSUNDEEP
javamultithreadingthread-safetyjava-8
Bick
Bick

Reputation: 18521

Java - holding variables in cache for many threads

I want to have a class that holds variables in cache for many threads.

Is it a good practice to hold it like this in a ConcurrentHashMap?

public class CacheMap {

    private static Map<Object, Object> cacheMap = new ConcurrentHashMap<>();

    public static void set(Object key, Object value) {
        cacheMap.put(key, value);
    }

    public static Object get(String key) {
        return cacheMap.get(key);
    }

}

Upvotes: 2

Views: 3139

Answers (3)

mrt
mrt

Reputation: 79

In terms of concurrency your solution will work fine. The ConcurrentHashMap is thread safe so it could be used by many threads.

I just recommend you to be careful with memory leaks, because your cache could grow without limit if you don't define a procedure to clean it.

Also you can define your own cache. For instance, I like to use LRU (Least Recently Use) queues for my cache objects. Using a LRU queue you will be sure that the cache won't grow without limit and that the elements that will be removed from the cache will be those that were used more time ago.

You can implement a LRU queue, for example a possible implementation could be:

public class MyLRUHashMap<K, V> extends LinkedHashMap<K, V> {

    private static final long serialVersionUID = -3216599441788189122L;

    private int maxCapacity = 1000;

    protected MyLRUHashMap(int capacity) {
        super(capacity);
        maxCapacity = capacity;
    }

    protected MyLRUHashMap(int capacity, boolean accessOrder) {
        super(capacity, 0.75f, accessOrder);
        maxCapacity = capacity;
    }

    @Override
    protected boolean removeEldestEntry(Map.Entry<K, V> eldest) {
        return (this.size() >= maxCapacity);
    }
}

In order to create a thread safe variable of "MyHashMap" you have to instantiate it in this way:

Map<Object, Object> MyCacheMap = Collections.synchronizedMap(new MyLRUHashMap(capacity, accessOrder));

Hope it helps! :)

Upvotes: -1

Tagir Valeev
Tagir Valeev

Reputation: 100149

In current implementation it's possible that you will have the same value computed twice. I suppose the usage pattern currently looks like this:

Object value = cache.get(key);
if(value == null) {
    value = computeValue();
    cache.set(key, value);
}
// use value here

Having such code it's possible that you will compute the same value several times if several threads simultaneously ask for the same value. Moreover, they will be using the different resulting object which may lead to unnecessary memory waste if the value is long-lived object.

The better solution is to use the computeIfAbsent method of ConcurrentHashMap:

Object value = map.computeIfAbsent(key, k -> computeValue());

This way it's guaranteed that it will be computed only once for each key. Thus instead of having two get and set methods in your cache implementation I'd suggest to have single computeIfAbsent method.

Also there are minor issues in your code like different types from key in get and set methods and lack of generic parameters.

Upvotes: 4

Jordi Castilla
Jordi Castilla

Reputation: 26961

I would recommend to use volatile in the Map, but the rest of the pattern seems ok.

From this answer:

Volatile variable: If two Threads(suppose t1 and t2) are accessing the same object and updating a variable which is declared as volatile then it means t1 and t2 can make their own local cache of the Object except the variable which is declared as a volatile . So the volatile variable will have only one main copy which will be updated by different threads and update made by one thread to the volatile variable will immediately reflect to the other Thread.

private volatile Map<Object, Object> cacheMap = new ConcurrentHashMap<>();

Also check this article for further info.

Upvotes: 0

Related Questions