Java - holding variables in cache for many threads

Question

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);
    }

}

Yes, more or less. For certain values of "good" and "practice." Though I don't see why you have to duplicate the methods of `Map` by renaming them `set` and `get`. — markspace, Jun 30 '15 at 08:05
It looks to me like you should extend ConcurrentHashMap and make your class a singleton — blagae, Jun 30 '15 at 08:09
Do you intend to have any additional methods to go along with the cache? Otherwise you can just use a CHM directly. Also, depending on just how much performance you want to squeeze out and how the read:write ratio is a copy-on-write strategy might perform even better than CHM. — the8472, Jun 30 '15 at 08:42
I'm not sure why you'd want composition here, unless you want to restrict access to the Map API for your cache. I would extend ConcurrentHashMap because I assume you want a unified cache, and you can easily do that by making your CacheMap a singleton. — blagae, Jun 30 '15 at 09:15
@rails Composition is a better option rather than inheritance. You get the flexibility to swap underlying `cache` implementation from a simple `Map` to a full fledged caching framework such as `EhCache` without breaking client code. Also, you should make `cacheMap` final to ensure visibility. — Chetan Kinger, Jun 30 '15 at 09:46
@ChetanKinger. Thanks. Thats what I thought too. I also turned chacheMap to be final. Please explain 'Ensure visibility'? — Bick, Jun 30 '15 at 09:54
@rails `final` guarantees that `map` would be initialized before the constructor of `CacheMap` returns. See [this](http://stackoverflow.com/questions/24789287/constructors-and-instruction-reordering) question. — Chetan Kinger, Jun 30 '15 at 10:09
Q: What is the value added of your `CacheMap` class as shown? (i.e., Why not just use an instance of `ConcurrentHashMap`?) If you have a real need to hide some of the other `ConcurrentHahsMap` methods, then that would be a valid reason, but I can't think of any other. — Solomon Slow, Jun 30 '15 at 16:07
@blagae, How can you say, "that should be a singleton?". Nothing in the example shows how the class will be used. A singleton prevents an application from using more than one instance. That makes sense when there is some fundamental reason why more than one instance must never exist, but this class is nothing but a Map. There are lots of programs that use more than one map, so why make the class _less_ useful than it could be? Also, any class that depends on the singleton will be harder to test. I would never make a singleton unless I had a _very_ good reason. — Solomon Slow, Jun 30 '15 at 16:15

score 4 · Answer 1 · answered Jun 30 '15 at 08:09

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.

score 0 · Answer 2 · edited May 23 '17 at 12:14

0

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.

edited May 23 '17 at 12:14

Community

1
1

answered Jun 30 '15 at 08:06

Jordi Castilla

26,609
8
70
109

The `volatile` keyword is absolutely unnecessary here as `cacheMap` is assigned only once. It would be better to use `final` instead. – Tagir Valeev Jun 30 '15 at 08:10
how can you say `cacheMap` is assigned only once? – Jordi Castilla Jun 30 '15 at 08:12
In OP code it's `private` field and the whole class implementation is provided where I can see only one assignment, thus I'm pretty sure. – Tagir Valeev Jun 30 '15 at 08:13
But doesnt the ConcurrentHashMap considered threadsafe by itself? – Bick Jun 30 '15 at 09:07
depending the behaviour you expect... AFAIK threadsafe in this case does not mean just one instance shared by all threads that is what I understand you are asking for... – Jordi Castilla Jun 30 '15 at 09:10

score -1 · Answer 3 · answered Jun 30 '15 at 08:12

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! :)

`Collections.synchronizedMap` will also synchronize on reads. If it's accessed by multiple threads this can have a significant performance impact once the involved lock becomes contended. — the8472, Jun 30 '15 at 08:41

Java - holding variables in cache for many threads

3 Answers3