6
\$\begingroup\$

I have been working on a cache that can handle multiple reads at the same time, but the write is blocking.

Is this code overkill by using both a ConcurrentHashMap and a ReentrantReadWriteLock? Could I get away with just the lock?

Are there any other optimizations I could make?

public class QueryCache implements Serializable {

    private final ReentrantReadWriteLock rwl = new ReentrantReadWriteLock();
    private final Lock readLock = rwl.readLock();
    private final Lock writeLock = rwl.writeLock();

    /**
     * Enum representations of different cache operations. You can put a new cache operation here.
     */
    public enum Operation {
        INCREMENT,
        OVERWRITE
    }


    /**
     * String names of the operations that can be performed. You can add more here.
     */
    public static final String INCREMENT = "increment";
    public static final String OVERWRITE = "overwrite";


    /**
     * Contains the key (query) and operations to perform on query
     */
    protected final Map<QueryKey, Map<Operation, BasicDBObject>> cache;


    /**
     * Create a new {@link QueryCache} with the initial capacity {@code size}
     *
     * @param size the initial capacity of the cache
     */
    public QueryCache(final int size) {
        cache = new ConcurrentHashMap<>(size);
    }


    /**
     * Returns an unmodifiable read-only version of the cache.
     * Clears the original cache.
     *
     * @return an unmodifiable read-only version of the cache.
     */
    public Map<QueryKey, Map<Operation, BasicDBObject>> flush() {
        writeLock.lock();
        try {
            Map<QueryKey, Map<Operation, BasicDBObject>> immutableCache = Collections.unmodifiableMap(new HashMap<>(cache));
            cache.clear();
            return immutableCache;
        } finally {
            writeLock.unlock();
        }
    }


    /**
     * Returns the size of the cache
     *
     * @return the size of the cache
     */
    public int size() {
        readLock.lock();
        try {
            return cache.size();
        } finally {
            readLock.unlock();
        }
    }


    /**
     * Clears the cache
     */
    public void clear() {
        writeLock.lock();
        try {
            cache.clear();
        } finally {
            writeLock.unlock();
        }
    }


    /**
     * General insertion call that will pass the request to the inserter with the matching {@code insertionMode} .
     * Will return if a matching insert operation was found.
     *
     * @param insertionMode the name of the insertion to perform
     * @param key           the current query
     * @param fieldName     the name of the field to cache
     * @param fieldValue    the value of the field to cache.
     * @return if a matching insert operation was found.
     */
    public boolean handleInsertion(final String insertionMode, final QueryKey key, final String fieldName, final Object fieldValue) {
        switch (insertionMode) {
            case INCREMENT:
                ensureCacheKey(key);
                insertIncrement(key, fieldName, getFieldValue(fieldValue));
                return true;
            case OVERWRITE:
                ensureCacheKey(key);
                insertOverwrite(key, fieldName, getFieldValue(fieldValue));
                return true;
            default:
                return false;
        }
    }


    protected static Object getFieldValue(final Object field) {
        if (field instanceof JsonNode) {
            JsonNode node = (JsonNode) field;
            if (node.isInt()) return node.asInt();
            if (node.isLong()) return node.asLong();
            if (node.isBoolean()) return node.asBoolean();
            return node.asText();
        }
        return field;
    }


    /**
     * Ensures that there is an empty, non-null value, for each available operation for the provided {@code key}.
     *
     * @param key
     */
    private void ensureCacheKey(final QueryKey key) {
        if (null == cache.get(key)) {
            writeLock.lock();
            try {
                if (null == cache.get(key)) {
                    Map<Operation, BasicDBObject> cacheOperations = new ConcurrentHashMap<>();
                    for (Operation operation : Operation.values()) {
                        cacheOperations.put(operation, new BasicDBObject());
                    }
                    cache.put(key, cacheOperations);
                }

            } finally {
                writeLock.unlock();
            }
        }
    }


    /**
     * Increments any existing increment query in the {@code incrementCache} or inserts it.
     *
     * @param key        the current query
     * @param fieldName  the name of the field to cache
     * @param fieldValue the value of the field to cache
     */
    private void insertIncrement(final QueryKey key, final String fieldName, final Object fieldValue) {

        Long value;
        if (fieldValue instanceof Integer) {
            value = ((Integer) fieldValue).longValue();
        } else {
            value = (Long) fieldValue;
        }

        cache.get(key).get(Operation.INCREMENT).putIfAbsent(fieldName, new AtomicLong());
        ((AtomicLong) cache.get(key).get(Operation.INCREMENT).get(fieldName)).addAndGet(value);
    }


    /**
     * Overwrites any existing overwrite query in the {@code overwriteCache} or inserts it.
     *
     * @param key        the current query
     * @param fieldName  the name of the field to cache
     * @param fieldValue the value of the field to cache
     */
    private void insertOverwrite(final QueryKey key, final String fieldName, final Object fieldValue) {
        writeLock.lock();
        try {
            cache.get(key).get(Operation.OVERWRITE).put(fieldName, fieldValue);
        } finally {
            writeLock.unlock();
        }
    }
}
\$\endgroup\$
4
\$\begingroup\$

Concurrent code is very subtle. Even if your code works without locks, removing them could lead to bugs down the road if someone changes the code.

That said, you could remove the locks if you change how ensureCacheKey works. For example, you could remove the locking in that method and call cache.putIfAbsent instead of cache.put. This approach is attractive because the cache.get() check eliminates most unnecessary cacheOperations creations and the few that slip through should be very cheap. Alternately you could have a lock specific to that method (or with more effort, a separate lock for each particular query key).

The usages of the read lock are unnecessary because ConcurrentHashMap supports reads concurrent with writes and because read operations only perform one map operation. The only other place where the cache is modified is when it is cleared. As you may define your concurrency semantics to order chronologically overlapping clear and put operations as you please, and because each write operation only performs one map write, these operations don't need to share a lock.

Some additional comments:

  1. In flush(), you are clearing the cache and thus clearing immutableCache. That probably isn't what you want. I suggest replacing cache.clear() with cache = new ... and making the cache variable volatile. In this case make sure that any methods that perform multiple operations on a cache read the cache into a local variable and do all operations on the local variable so they are operating entirely on the same cache.

  2. If overwrites and increments never occur on the same keys, your code might be cleaner if you had separate maps for inserts and overwrites.

  3. Instead of having strings "increment" and "overwrite" defined as separate top-level variables, make them fields of Operation or lowercase the Operation instances and use Operation.xxx.name().

  4. Use !cache.containsKey(x) instead of cache.get(x) == null.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the excellent feedback. I have added most of the feedback to the code. Though you can not use comment #3, as it is not a compile time expression. \$\endgroup\$ – maffo Mar 13 '16 at 18:16
  • \$\begingroup\$ @poixen For #3, use Operations, not strings, and switch on the Operation. \$\endgroup\$ – Solomonoff's Secret Mar 13 '16 at 23:44
  • 1
    \$\begingroup\$ BasicDBObject (if it is MongoDb's) is a subclass of LinkedHashMap and therefore not thread safe. I don't think you can forego the locks. \$\endgroup\$ – bowmore Mar 14 '16 at 6:50
  • \$\begingroup\$ @bowmore doesn't making the cache volatile solve this issue? The map that the DBO is contained in is also concurrent, so the writes should be locking? \$\endgroup\$ – maffo Mar 14 '16 at 7:15
  • 1
    \$\begingroup\$ in insertOverWrite you do : cache.get(key).get(Operation.OVERWRITE).put(fieldName, fieldValue);; without the write lock, that put is not synchronized. The containing ConcurrentHashMaps don't help you here, as you're outside their get() methods. Volatile doesn't help you either. \$\endgroup\$ – bowmore Mar 14 '16 at 7:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.