Get rid of SoftReference in RepositoryCache

Now that RepositoryCache have a time based eviction strategy, get rid
of the strategy to evict cache entries if heap memory is running low,
i.e. soft references. Main reason why time based eviction was
implemented was to offer an alternative to the unpredictable soft
references.

Relying on soft references is not working, especially in large heap. The
JVM GC will consider collecting soft references as last resort before
throwing an out of memory error. For example, an application like Gerrit
configured with a 128GB heap, GC will wait until all 128GB is filled
before collecting the soft references so the application will be
suffering long pauses caused by GC for a long time already. In other
words, you will have to restart application because it's unusable before
JVM eviction kicks in.

Keeping the SoftReference in RepositoryCache is causing more harm than
good. If you use the time based eviction (which is the default strategy)
and want to tune JVM to release soft references more aggressively, it
will release repositories from the cache even though they are not
expired which defeats the purpose of the repository cache.

Gerrit uses Lucene library which uses soft references and this is
causing a "memory leak" except if you configure JVM to release soft
references more aggressively which have the nasty side effect of
evicting non expired repositories from the cache.

Change-Id: I9940bd800464c7f007696d0ccde52ea617b2ebce
Signed-off-by: Hugo Arès <hugo.ares@ericsson.com>

2 files changed, 12 insertions, 23 deletions

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/RepositoryCache.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/RepositoryCache.java
index 9a57349f56..543511609d 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/RepositoryCache.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/RepositoryCache.java
@@ -45,8 +45,6 @@ package org.eclipse.jgit.lib;
 
 import java.io.File;
 import java.io.IOException;
-import java.lang.ref.Reference;
-import java.lang.ref.SoftReference;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Iterator;
@@ -202,8 +200,7 @@ public class RepositoryCache {
 			return false;
 		}
 		FileKey key = new FileKey(gitDir, repo.getFS());
-		Reference<Repository> repoRef = cache.cacheMap.get(key);
-		return repoRef != null && repoRef.get() == repo;
+		return cache.cacheMap.get(key) == repo;
 	}
 
 	/** Unregister all repositories from the cache. */
@@ -219,7 +216,7 @@ public class RepositoryCache {
 		cache.configureEviction(repositoryCacheConfig);
 	}
 
-	private final ConcurrentHashMap<Key, Reference<Repository>> cacheMap;
+	private final ConcurrentHashMap<Key, Repository> cacheMap;
 
 	private final Lock[] openLocks;
 
@@ -228,7 +225,7 @@ public class RepositoryCache {
 	private volatile long expireAfter;
 
 	private RepositoryCache() {
-		cacheMap = new ConcurrentHashMap<Key, Reference<Repository>>();
+		cacheMap = new ConcurrentHashMap<Key, Repository>();
 		openLocks = new Lock[4];
 		for (int i = 0; i < openLocks.length; i++) {
 			openLocks[i] = new Lock();
@@ -261,19 +258,15 @@ public class RepositoryCache {
 		}
 	}
 
-	@SuppressWarnings("resource")
 	private Repository openRepository(final Key location,
 			final boolean mustExist) throws IOException {
-		Reference<Repository> ref = cacheMap.get(location);
-		Repository db = ref != null ? ref.get() : null;
+		Repository db = cacheMap.get(location);
 		if (db == null) {
 			synchronized (lockFor(location)) {
-				ref = cacheMap.get(location);
-				db = ref != null ? ref.get() : null;
+				db = cacheMap.get(location);
 				if (db == null) {
 					db = location.open(mustExist);
-					ref = new SoftReference<Repository>(db);
-					cacheMap.put(location, ref);
+					cacheMap.put(location, db);
 				} else {
 					db.incrementOpen();
 				}
@@ -285,16 +278,13 @@ public class RepositoryCache {
 	}
 
 	private void registerRepository(final Key location, final Repository db) {
-		SoftReference<Repository> newRef = new SoftReference<Repository>(db);
-		Reference<Repository> oldRef = cacheMap.put(location, newRef);
-		Repository oldDb = oldRef != null ? oldRef.get() : null;
+		Repository oldDb = cacheMap.put(location, db);
 		if (oldDb != null)
 			oldDb.close();
 	}
 
 	private Repository unregisterRepository(final Key location) {
-		Reference<Repository> oldRef = cacheMap.remove(location);
-		return oldRef != null ? oldRef.get() : null;
+		return cacheMap.remove(location);
 	}
 
 	private boolean isExpired(Repository db) {
@@ -316,8 +306,7 @@ public class RepositoryCache {
 	}
 
 	private void clearAllExpired() {
-		for (Reference<Repository> ref : cacheMap.values()) {
-			Repository db = ref.get();
+		for (Repository db : cacheMap.values()) {
 			if (isExpired(db)) {
 				RepositoryCache.close(db);
 			}
@@ -325,7 +314,7 @@ public class RepositoryCache {
 	}
 
 	private void clearAll() {
-		for (Iterator<Map.Entry<Key, Reference<Repository>>> i = cacheMap
+		for (Iterator<Map.Entry<Key, Repository>> i = cacheMap
 				.entrySet().iterator(); i.hasNext();) {
 			unregisterAndCloseRepository(i.next().getKey());
 		}
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/RepositoryCacheConfig.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/RepositoryCacheConfig.java
index 428dea3e67..28cdaae443 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/RepositoryCacheConfig.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/RepositoryCacheConfig.java
@@ -53,8 +53,8 @@ public class RepositoryCacheConfig {
 
 	/**
 	 * Set cleanupDelayMillis to this value in order to switch off time-based
-	 * cache eviction. The JVM can still expire cache entries when heap memory
-	 * runs low.
+	 * cache eviction. Expired cache entries will only be evicted when
+	 * RepositoryCache.clearExpired or RepositoryCache.clear are called.
 	 */
 	public static final long NO_CLEANUP = 0;