patternjavaMinor
Reducing lock contention for a caching utility, or make it totally lockless but threadsafe
Viewed 0 times
contentionthreadsafebutcachingutilityreducingmaketotallyforlockless
Problem
My Java program is a module called
The implementation of this singleton class:
```
public class ConfigProxy {
/ This is our one-n-only instance /
private static ConfigProxy _instance = null;
/ default values for some keys which may not be declared /
private HashMap _defaults = null;
private Jedis jedisConfigConnection;
// hash map for storing the data of config proxy as a cache for each
// String,List and Hash
// to prevent constant hitting to Redis
private ConcurrentHashMap _cacheStrings = new ConcurrentHashMap();
private ConcurrentHashMap> _cacheLists = new ConcurrentHashMap>();
private ConcurrentHashMap> _cacheHashes = new ConcurrentHashMap>();
/*
* Takes host and port as parameters to connect with redis instance
*/
private ConfigProxy(String host, int port) throws ConnectionException {
try {
jedisConfigConnection = new Jedis(host, port);
jedisConfigConnection.ping();
} catch (Exception e) {
throw new ConnectionException("Unable to connect to Config Server");
}
}
public static ConfigProxy getInstance(String host, int port)
throws ConnectionException {
if (_instance == null) _instance = new ConfigProxy(host, port);
return _instance;
}
/**
* Overloaded version which doesn't take any parameters. Inorder for this
* call to succeed, the caller, somewhere should have called
* getInstance(host, port), once. else this throws InitializationException.
*
* @return
* @throws NotInitializedException
*/
public static ConfigProxy getInstance() throws NotInitializedException {
if (_instance == null) throw new NotInitializedException();
return _instance;
}
...
/*
configProxy which fetches configuration entries from a remote node (say from a Redis instance). When the caller calls the get(configKey) method, this module checks in a local cache (a concurrent hashmap) to see if the setting is already cached. If not, it fetches the setting from a Redis instance, caches it and then returns.The implementation of this singleton class:
```
public class ConfigProxy {
/ This is our one-n-only instance /
private static ConfigProxy _instance = null;
/ default values for some keys which may not be declared /
private HashMap _defaults = null;
private Jedis jedisConfigConnection;
// hash map for storing the data of config proxy as a cache for each
// String,List and Hash
// to prevent constant hitting to Redis
private ConcurrentHashMap _cacheStrings = new ConcurrentHashMap();
private ConcurrentHashMap> _cacheLists = new ConcurrentHashMap>();
private ConcurrentHashMap> _cacheHashes = new ConcurrentHashMap>();
/*
* Takes host and port as parameters to connect with redis instance
*/
private ConfigProxy(String host, int port) throws ConnectionException {
try {
jedisConfigConnection = new Jedis(host, port);
jedisConfigConnection.ping();
} catch (Exception e) {
throw new ConnectionException("Unable to connect to Config Server");
}
}
public static ConfigProxy getInstance(String host, int port)
throws ConnectionException {
if (_instance == null) _instance = new ConfigProxy(host, port);
return _instance;
}
/**
* Overloaded version which doesn't take any parameters. Inorder for this
* call to succeed, the caller, somewhere should have called
* getInstance(host, port), once. else this throws InitializationException.
*
* @return
* @throws NotInitializedException
*/
public static ConfigProxy getInstance() throws NotInitializedException {
if (_instance == null) throw new NotInitializedException();
return _instance;
}
...
/*
Solution
-
Definitely do not lock on the
-
Strictly speaking, you could use
-
I'm not entirely convinced that a
-
Random convention stuff - Java doesn't use underscores to indicate instance variables. Always use curly braces. Make everything final that you can.
Definitely do not lock on the
ConfigProxy instance. Somebody else could acquire a lock on it and then you'd be up the creek. Only lock on private instances that you have the only copy of.-
Strictly speaking, you could use
putIfAbsent and get rid of the lock altogether, but then you're potentially seeing multiple Redis calls for the same key. You should check both outside and inside the synchronized block to see if the key is there yet. If two threads both hit that barrier in the else, the first one will put a value in the map .. then the second one will go do it again. The first check keeps you out of the synchronized block, and the second one lets you exit it early.-
I'm not entirely convinced that a
Concurrent map buys you anything here, since you're managing the puts using synchronization. I might be wrong - there may be black magic around gets that I'm unaware of.-
Random convention stuff - Java doesn't use underscores to indicate instance variables. Always use curly braces. Make everything final that you can.
Context
StackExchange Code Review Q#82810, answer score: 2
Revisions (0)
No revisions yet.