From 83235432e7fd789261cad7729bf3febfc168cd6f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20Ar=C3=A8s?= Date: Fri, 4 Sep 2015 15:32:28 -0400 Subject: [PATCH 1/3] Fix repository cache never closing repository MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Repository has a usage counter that is initialized to 1 at instantiation and this counter is decremented when Repository.close method is called. There is also a Repository.incrementOpen method that RepositoryCache uses to increment the usage count when it's returning a repository that is already opened. The problem was that RepositoryCache was incrementing the usage count for repositories that it just opened or registered. The usage count was 2 when it should have been 1. Incrementing usage count is now only be done for repository that are served from the cache. This bug is causing slow memory increase of our Gerrit server until the server become slow. Even if the RepositoryCache is using SoftReference, it seems that the JVM is not garbage collecting the repositories because it's not yet on the edge of being out of memory. To test this change, I replicated all repositories(11k) from Gerrit master to one slave. The Gerrit master used memory after this test was 10GB without this change and 3.5GB with. Change-Id: I86c7b36174e384f106b51fe92f306018fd1dbdf0 Signed-off-by: Hugo Arès --- .../eclipse/jgit/lib/RepositoryCacheTest.java | 21 +++++++++++++++++++ .../src/org/eclipse/jgit/lib/Repository.java | 3 ++- .../org/eclipse/jgit/lib/RepositoryCache.java | 6 ++++-- 3 files changed, 27 insertions(+), 3 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RepositoryCacheTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RepositoryCacheTest.java index 6c6292558..8f30fd082 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RepositoryCacheTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RepositoryCacheTest.java @@ -173,4 +173,25 @@ public void testUnregister() { assertEquals(0, RepositoryCache.getRegisteredKeys().size()); } + @Test + public void testRepositoryUsageCount() throws Exception { + FileKey loc = FileKey.exact(db.getDirectory(), db.getFS()); + Repository d2 = RepositoryCache.open(loc); + assertEquals(1, d2.useCnt.get()); + RepositoryCache.open(FileKey.exact(loc.getFile(), db.getFS())); + assertEquals(2, d2.useCnt.get()); + d2.close(); + assertEquals(1, d2.useCnt.get()); + d2.close(); + assertEquals(0, d2.useCnt.get()); + } + + @Test + public void testRepositoryUsageCountWithRegisteredRepository() { + assertEquals(1, ((Repository) db).useCnt.get()); + RepositoryCache.register(db); + assertEquals(1, ((Repository) db).useCnt.get()); + db.close(); + assertEquals(0, ((Repository) db).useCnt.get()); + } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java index f8266133a..5546b790e 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java @@ -110,7 +110,8 @@ public static ListenerList getGlobalListenerList() { return globalListeners; } - private final AtomicInteger useCnt = new AtomicInteger(1); + /** Use counter */ + final AtomicInteger useCnt = new AtomicInteger(1); /** Metadata directory holding the repository's critical files. */ private final File gitDir; 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 23cc264c1..58771857b 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/RepositoryCache.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/RepositoryCache.java @@ -196,15 +196,17 @@ private Repository openRepository(final Key location, db = location.open(mustExist); ref = new SoftReference(db); cacheMap.put(location, ref); + } else { + db.incrementOpen(); } } + } else { + db.incrementOpen(); } - db.incrementOpen(); return db; } private void registerRepository(final Key location, final Repository db) { - db.incrementOpen(); SoftReference newRef = new SoftReference(db); Reference oldRef = cacheMap.put(location, newRef); Repository oldDb = oldRef != null ? oldRef.get() : null; From 53ea86cd7be3859ff741ea2b9760396ffabf5fe0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20Ar=C3=A8s?= Date: Tue, 8 Sep 2015 09:21:31 -0400 Subject: [PATCH 2/3] Fix RefDirectory not closing resources MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When repositories are opened using the RepositoryCache, they are kept in memory and when the repository usage counter reaches 0, the Repository.close method is called which then calls close method on its reference and object databases. The problem is that RefDirectory.close method was a no-op and the reference database was kept in memory. This problem is only happening when opening a repository using the RepositoryCache because it never evicts repositories, it's just calling the close method. Change-Id: Iacb961de8e8b1f5b37824bf0d1a4caf4c6f1233f Signed-off-by: Hugo Arès --- .../eclipse/jgit/internal/storage/file/RefDirectory.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java index b8c2fb4a5..6f3166a68 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java @@ -203,10 +203,10 @@ public void create() throws IOException { @Override public void close() { - // We have no resources to close. + clearReferences(); } - void rescan() { + private void clearReferences() { looseRefs.set(RefList. emptyList()); packedRefs.set(PackedRefList.NO_PACKED_REFS); } @@ -214,7 +214,7 @@ void rescan() { @Override public void refresh() { super.refresh(); - rescan(); + clearReferences(); } @Override From 7d2b3b9e25a430241a2d277e0cb222ad40cd0c2c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20Ar=C3=A8s?= Date: Fri, 18 Sep 2015 14:05:23 -0400 Subject: [PATCH 3/3] Remove repository from cache when it's closed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RepositoryCache has 2 methods to remove a repository from the cache but they are never called when a repository is closed. Users of the cache were expected to call one of those 2 methods but how could they have called them at proper time without having visibility of the repository usage count. Ideally, I would have reworked the RepositoryCache to wrap any repository it opens in a class that would be responsible to unregister them from the cache when it's really closed, i.e. when usage counter reaches 0. The problem preventing the wrapping solution is the RepositoryCache.register method that allows to register an already opened repository in the cache. Such repositories cannot be wrapped because callers are still holding a reference on the unwrapped repository. Document that RepositoryCache.close method is removing the repository from the cache as well as closing it and rework RepositoryCache.unregister method to only remove the repository from the cache. Use the latter to unregister repository when Repository.doClose is getting executed. Change-Id: Ia364816e4da8d7b6cfa72f10758ca31aa8a1f9db Signed-off-by: Hugo Arès Signed-off-by: Matthias Sohn --- .../eclipse/jgit/lib/RepositoryCacheTest.java | 14 ++++++ .../src/org/eclipse/jgit/lib/Repository.java | 1 + .../org/eclipse/jgit/lib/RepositoryCache.java | 43 +++++++++++++++---- 3 files changed, 49 insertions(+), 9 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RepositoryCacheTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RepositoryCacheTest.java index 8f30fd082..a1cec2d91 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RepositoryCacheTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/RepositoryCacheTest.java @@ -194,4 +194,18 @@ public void testRepositoryUsageCountWithRegisteredRepository() { db.close(); assertEquals(0, ((Repository) db).useCnt.get()); } + + public void testRepositoryUnregisteringWhenClosing() throws Exception { + FileKey loc = FileKey.exact(db.getDirectory(), db.getFS()); + Repository d2 = RepositoryCache.open(loc); + assertEquals(1, d2.useCnt.get()); + assertThat(RepositoryCache.getRegisteredKeys(), + hasItem(FileKey.exact(db.getDirectory(), db.getFS()))); + assertEquals(1, RepositoryCache.getRegisteredKeys().size()); + + d2.close(); + + assertEquals(0, d2.useCnt.get()); + assertEquals(0, RepositoryCache.getRegisteredKeys().size()); + } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java index 5546b790e..ba0dea39f 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/Repository.java @@ -865,6 +865,7 @@ public void incrementOpen() { public void close() { if (useCnt.decrementAndGet() == 0) { doClose(); + RepositoryCache.unregister(this); } } 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 58771857b..22b5fcd11 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/RepositoryCache.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/RepositoryCache.java @@ -130,10 +130,10 @@ public static void register(final Repository db) { } /** - * Remove a repository from the cache. + * Close and remove a repository from the cache. *

- * Removes a repository from the cache, if it is still registered here, - * permitting it to close. + * Removes a repository from the cache, if it is still registered here, and + * close it. * * @param db * repository to unregister. @@ -141,15 +141,35 @@ public static void register(final Repository db) { public static void close(final Repository db) { if (db.getDirectory() != null) { FileKey key = FileKey.exact(db.getDirectory(), db.getFS()); - cache.unregisterRepository(key); + cache.unregisterAndCloseRepository(key); } } /** * Remove a repository from the cache. *

- * Removes a repository from the cache, if it is still registered here, - * permitting it to close. + * Removes a repository from the cache, if it is still registered here. This + * method will not close the repository, only remove it from the cache. See + * {@link RepositoryCache#close(Repository)} to remove and close the + * repository. + * + * @param db + * repository to unregister. + * @since 4.3 + */ + public static void unregister(final Repository db) { + if (db.getDirectory() != null) { + unregister(FileKey.exact(db.getDirectory(), db.getFS())); + } + } + + /** + * Remove a repository from the cache. + *

+ * Removes a repository from the cache, if it is still registered here. This + * method will not close the repository, only remove it from the cache. See + * {@link RepositoryCache#close(Repository)} to remove and close the + * repository. * * @param location * location of the repository to remove. @@ -214,11 +234,16 @@ private void registerRepository(final Key location, final Repository db) { oldDb.close(); } - private void unregisterRepository(final Key location) { + private Repository unregisterRepository(final Key location) { Reference oldRef = cacheMap.remove(location); - Repository oldDb = oldRef != null ? oldRef.get() : null; - if (oldDb != null) + return oldRef != null ? oldRef.get() : null; + } + + private void unregisterAndCloseRepository(final Key location) { + Repository oldDb = unregisterRepository(location); + if (oldDb != null) { oldDb.close(); + } } private Collection getKeys() {