RedisCache synchronises all get(key, valueLoader) calls

[miensol]

I was examining the performance in our Spring Boot app. Based on profiling data, I noticed that we have a lot of wait times on RedisCache#get(key, valueLoader) method.

The method signature makes it obvious why this is happening:

public synchronized <T> T get(Object key, Callable<T> valueLoader) {

In case of our app, it causes all cache lookups to be executed sequentially. (Admittedly a great protection for Redis server 😅).

In the past this method wasn't synchronized. It changed in this commit.

Documentation of the Cache interface states:

Return the value to which this cache maps the specified key, obtaining that value from valueLoader if necessary. This method provides a simple substitute for the conventional "if cached, return; otherwise create, cache and return" pattern.

If possible, implementations should ensure that the loading operation is synchronized so that the specified valueLoader is only called once in case of concurrent access on the same key.

Technically speaking, the current synchronized implementation adheres to documentation. However, so would version that is not synchronized.

For comparison, here's how the EhCacheCache is implemented:

    public <T> T get(Object key, Callable<T> valueLoader) {
		Element element = lookup(key);
		if (element != null) {
			return (T) element.getObjectValue();
		}
		else {
			this.cache.acquireWriteLockOnKey(key);
			try {
				element = lookup(key);  // one more attempt with the write lock
				if (element != null) {
					return (T) element.getObjectValue();
				}
				else {
					return loadValue(key, valueLoader);
				}
			}
			finally {
				this.cache.releaseWriteLockOnKey(key);
			}
		}
	}

I'm happy to submit a PR that changes the implementation. However, before doing so, I'd love to ask if the synchronized method was implemented in the current way deliberately and there's and edge case which makes changing it harder?
LV link

[mp911de]

I think we should be able to optimize and add synchronization only if the lookup yields null. Feel free to submit a pull request.

[miensol]

Thanks. That's certainly easy.

How do you feel about synchronising (in JVM) when loading values on a per key basis?

[mp911de]

Synchronizing on externally supplied keys is actually prone to create more problems than actually solve. We would need to maintain a decoupled copy and do proper housekeeping to evict keys as we do not have an in-memory reference backed by a cache like EhCache and others do.

[miensol]

If we talk about single JVM locking it doesn't sound that complicated. I may be missing something though.
Wouldn't using ConcurrentReferenceHashMap to store a mutex/lock object for key be enough?

[mp911de]

There are two main issues why this isn't going to work with just a Map:

1. The externally given key is computed from the cache KeyGenerator. It isn't a stable object that would be reused.
2. The consequence is that we need to maintain our own mutex (key wrapper) that is looked up by comparing cache key equality
3. Since no one except the Redis Cache would have a reference to the mutex, a ReferenceHashMap is going to evict mutexes upon GC runs so the net effect is increased memory usage and CPU overhead to maintain mutexes.

We do not want to follow that way. A last note: When using a locking RedisCacheWriter, you get improved locking behavior out of the box.

[miensol]

Thanks for explanation. I understand your concerns, and they are very true in general case. I have no knowledge though if those cases, e.g. using unstable key generator happen in practice nor what a user would expect in such case.

3. Since no one except the Redis Cache would have a reference to the mutex, a ReferenceHashMap is going to evict mutexes upon GC runs so the net effect is increased memory usage and CPU overhead to maintain mutexes.

Yes, AFAIK no one except RedisCache would hold on to a mutex, and thus it wouldn't stay in memory for much longer than needed. If the mutex is used in a lock by some thread, it wouldn't be evicted by GC, would it?

I agree that there will be an increased memory usage. I think though that it will be relatively small, in general case, when compared to sizes of objects saved and received from cache.

I don't have any data other than our project. However, in case of our project without such mechanism, we'll spend time waiting on locks, since all threads that which to compute a value for given key are processed in queue.

Without per key locks, we need to do a workaround on our side.

A last note: When using a locking RedisCacheWriter, you get improved locking behavior out of the box.

Would you mind explaining how this helps one using RedisCache#get(key, valueLoader) ?