21

I am trying to implement a high performance thread-safe caching. Here is the code I have implemented. I don't want any on demand computing. Can I use cache.asMap() and retrieve the value safely? Even if the cache is set to have softValues?

  import java.io.IOException;
  import java.util.concurrent.ConcurrentMap;
  import java.util.concurrent.ExecutionException;
  import java.util.concurrent.TimeUnit;
  import java.util.concurrent.TimeoutException;

  import com.google.common.cache.Cache;
  import com.google.common.cache.CacheBuilder;

  public class MemoryCache {

    private static MemoryCache instance;
    private Cache<String, Object> cache;

    private MemoryCache(int concurrencyLevel, int expiration, int size) throws IOException {

        cache = CacheBuilder.newBuilder().concurrencyLevel(concurrencyLevel).maximumSize(size).softValues()
            .expireAfterWrite(expiration, TimeUnit.SECONDS).build();
    }

    static public synchronized MemoryCache getInstance() throws IOException {
        if (instance == null) {
               instance = new MemoryCache(10000, 3600,1000000);
        }
        return instance;
    }

    public Object get(String key) {
        ConcurrentMap<String,Object> map =cache.asMap();
        return map.get(key);
    }

    public void put(String key, Object obj) {
        cache.put(key, obj);
    }
   }
systemboot
  • 860
  • 2
  • 8
  • 22
  • 4
    As a curiosity, do you really expect to have 10000 threads accessing your Cache concurrently? I ask since that's the value you're using as a concurrency level. – pcalcao Jun 20 '12 at 17:44
  • No I've removed that parameter. I am now using the default concurrency level i.e. 4. Thanks for point it out though. – systemboot Jun 21 '12 at 18:00

2 Answers2

32

Guava contributor here:

Yes, that looks just fine, although I'm not sure what the point is of wrapping the cache in another object. (Also, Cache.getIfPresent(key) is fully equivalent to Cache.asMap().get(key).)

Louis Wasserman
  • 191,574
  • 25
  • 345
  • 413
8

If you want high performance, why don't you instanciate the Cache statically instead of using a synchronized getInstance()?

Frank Pavageau
  • 11,477
  • 1
  • 43
  • 53
  • 1
    Agree. Using this idiom would remedy synchronization on getInstance(): https://en.wikipedia.org/wiki/Initialization-on-demand_holder_idiom – bennidi Dec 08 '16 at 15:48
  • If you do that, I am not sure if it will have this problem https://stackoverflow.com/questions/4934913/are-static-variables-shared-between-threads – Surasin Tancharoen Jul 25 '19 at 12:19
  • @SurasinTancharoen not if you instanciate it during the object initialization (either in the `MemoryCache` directly, or using the holder pattern, making it `final`). The question you linked to talks about shared state visibility and unsynchronized reads and writes. – Frank Pavageau Jul 25 '19 at 13:40