From 2b9dd32a828577bc1026ad53ec4f735a9b219022 Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Fri, 3 Jan 2020 00:52:37 +0100 Subject: [PATCH 1/6] Add config constants for WindowCache configuration options Change-Id: Icc5265f87ae58aa1e667ed1827075c4a30f75c32 Signed-off-by: Matthias Sohn --- org.eclipse.jgit/.settings/.api_filters | 38 +++++++++++++++++++ .../org/eclipse/jgit/lib/ConfigConstants.java | 24 ++++++++++++ .../jgit/storage/file/WindowCacheConfig.java | 32 ++++++++++------ 3 files changed, 82 insertions(+), 12 deletions(-) diff --git a/org.eclipse.jgit/.settings/.api_filters b/org.eclipse.jgit/.settings/.api_filters index 9ee9ff7b6..14c22e937 100644 --- a/org.eclipse.jgit/.settings/.api_filters +++ b/org.eclipse.jgit/.settings/.api_filters @@ -49,6 +49,30 @@ + + + + + + + + + + + + + + + + + + + + + + + + @@ -84,6 +108,20 @@ + + + + + + + + + + + + + + diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java index 43776e081..9d292f642 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java @@ -200,6 +200,30 @@ public final class ConfigConstants { /** The "streamFileThreshold" key */ public static final String CONFIG_KEY_STREAM_FILE_TRESHOLD = "streamFileThreshold"; + /** + * The "packedGitMmap" key + * @since 5.1.13 + */ + public static final String CONFIG_KEY_PACKED_GIT_MMAP = "packedgitmmap"; + + /** + * The "packedGitWindowSize" key + * @since 5.1.13 + */ + public static final String CONFIG_KEY_PACKED_GIT_WINDOWSIZE = "packedgitwindowsize"; + + /** + * The "packedGitLimit" key + * @since 5.1.13 + */ + public static final String CONFIG_KEY_PACKED_GIT_LIMIT = "packedgitlimit"; + + /** + * The "packedGitOpenFiles" key + * @since 5.1.13 + */ + public static final String CONFIG_KEY_PACKED_GIT_OPENFILES = "packedgitopenfiles"; + /** The "remote" key */ public static final String CONFIG_KEY_REMOTE = "remote"; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/WindowCacheConfig.java b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/WindowCacheConfig.java index c2e6a4200..c04b677df 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/WindowCacheConfig.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/WindowCacheConfig.java @@ -43,6 +43,14 @@ package org.eclipse.jgit.storage.file; +import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_CORE_SECTION; +import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_DELTA_BASE_CACHE_LIMIT; +import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_PACKED_GIT_LIMIT; +import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_PACKED_GIT_MMAP; +import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_PACKED_GIT_OPENFILES; +import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_PACKED_GIT_WINDOWSIZE; +import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_STREAM_FILE_TRESHOLD; + import org.eclipse.jgit.internal.storage.file.WindowCache; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.storage.pack.PackConfig; @@ -227,20 +235,20 @@ public void setStreamFileThreshold(int newLimit) { * @since 3.0 */ public WindowCacheConfig fromConfig(Config rc) { - setPackedGitOpenFiles(rc.getInt( - "core", null, "packedgitopenfiles", getPackedGitOpenFiles())); //$NON-NLS-1$ //$NON-NLS-2$ - setPackedGitLimit(rc.getLong( - "core", null, "packedgitlimit", getPackedGitLimit())); //$NON-NLS-1$ //$NON-NLS-2$ - setPackedGitWindowSize(rc.getInt( - "core", null, "packedgitwindowsize", getPackedGitWindowSize())); //$NON-NLS-1$ //$NON-NLS-2$ - setPackedGitMMAP(rc.getBoolean( - "core", null, "packedgitmmap", isPackedGitMMAP())); //$NON-NLS-1$ //$NON-NLS-2$ - setDeltaBaseCacheLimit(rc.getInt( - "core", null, "deltabasecachelimit", getDeltaBaseCacheLimit())); //$NON-NLS-1$ //$NON-NLS-2$ + setPackedGitOpenFiles(rc.getInt(CONFIG_CORE_SECTION, null, + CONFIG_KEY_PACKED_GIT_OPENFILES, getPackedGitOpenFiles())); + setPackedGitLimit(rc.getLong(CONFIG_CORE_SECTION, null, + CONFIG_KEY_PACKED_GIT_LIMIT, getPackedGitLimit())); + setPackedGitWindowSize(rc.getInt(CONFIG_CORE_SECTION, null, + CONFIG_KEY_PACKED_GIT_WINDOWSIZE, getPackedGitWindowSize())); + setPackedGitMMAP(rc.getBoolean(CONFIG_CORE_SECTION, null, + CONFIG_KEY_PACKED_GIT_MMAP, isPackedGitMMAP())); + setDeltaBaseCacheLimit(rc.getInt(CONFIG_CORE_SECTION, null, + CONFIG_KEY_DELTA_BASE_CACHE_LIMIT, getDeltaBaseCacheLimit())); long maxMem = Runtime.getRuntime().maxMemory(); - long sft = rc.getLong( - "core", null, "streamfilethreshold", getStreamFileThreshold()); //$NON-NLS-1$ //$NON-NLS-2$ + long sft = rc.getLong(CONFIG_CORE_SECTION, null, + CONFIG_KEY_STREAM_FILE_TRESHOLD, getStreamFileThreshold()); sft = Math.min(sft, maxMem / 4); // don't use more than 1/4 of the heap sft = Math.min(sft, Integer.MAX_VALUE); // cannot exceed array length setStreamFileThreshold((int) sft); From 6185db3d776f1064af1972b4ba2175a917c35ab3 Mon Sep 17 00:00:00 2001 From: Patrick Hiesel Date: Thu, 9 Jan 2020 10:23:12 +0100 Subject: [PATCH 2/6] Replace usage of ArrayIndexOutOfBoundsException in treewalk Using exceptions during normal operations - for example with the desire of expanding an array in the failure case - can have a severe performance impact. When exceptions are instantiated, a stack trace is collected. Generating stack trace can be expensive. Compared to that, checking an array for length - even if done many times - is cheap since this is a check that can run in just a handful of CPU cycles. Change-Id: Ifaf10623f6a876c9faecfa44654c9296315adfcb Signed-off-by: Patrick Hiesel Signed-off-by: Matthias Sohn --- .../eclipse/jgit/treewalk/AbstractTreeIterator.java | 6 ++---- .../eclipse/jgit/treewalk/CanonicalTreeParser.java | 11 +++++------ 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/AbstractTreeIterator.java b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/AbstractTreeIterator.java index 335abe1b5..a3c93cf87 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/AbstractTreeIterator.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/AbstractTreeIterator.java @@ -239,12 +239,10 @@ protected AbstractTreeIterator(AbstractTreeIterator p) { path = p.path; pathOffset = p.pathLen + 1; - try { - path[pathOffset - 1] = '/'; - } catch (ArrayIndexOutOfBoundsException e) { + if (pathOffset > path.length) { growPath(p.pathLen); - path[pathOffset - 1] = '/'; } + path[pathOffset - 1] = '/'; } /** diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/CanonicalTreeParser.java b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/CanonicalTreeParser.java index 019968875..b2d8fc3aa 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/CanonicalTreeParser.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/treewalk/CanonicalTreeParser.java @@ -387,14 +387,13 @@ private void parseEntry() { tmp = pathOffset; for (;; tmp++) { c = raw[ptr++]; - if (c == 0) + if (c == 0) { break; - try { - path[tmp] = c; - } catch (ArrayIndexOutOfBoundsException e) { - growPath(tmp); - path[tmp] = c; } + if (tmp >= path.length) { + growPath(tmp); + } + path[tmp] = c; } pathLen = tmp; nextPtr = ptr + OBJECT_ID_LENGTH; From 709f83d489b777c21ad5bbeeb3e8b1232b3f0ee5 Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Fri, 13 Dec 2019 01:18:12 +0100 Subject: [PATCH 3/6] WindowCache: add option to use strong refs to reference ByteWindows Java GC evicts all SoftReferences when the used heap size comes close to the maximum heap size. This means peaks in heap memory consumption can flush the complete WindowCache which was observed to have negative impact on performance of upload-pack in Gerrit. Hence add a boolean option core.packedGitUseStrongRefs to allow using strong references to reference packfile pages cached in the WindowCache. If this option is set to true Java gc can no longer flush the WindowCache to free memory if the used heap comes close to the maximum heap size. On the other hand this provides more predictable performance. Bug: 553573 Change-Id: I9de406293087ab0fa61130c8e0829775762ece8d Signed-off-by: Matthias Sohn --- .../storage/file/WindowCacheGetTest.java | 68 ++-- org.eclipse.jgit/.settings/.api_filters | 6 + .../internal/storage/file/WindowCache.java | 337 +++++++++++++++--- .../org/eclipse/jgit/lib/ConfigConstants.java | 6 + .../jgit/storage/file/WindowCacheConfig.java | 32 ++ 5 files changed, 379 insertions(+), 70 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/WindowCacheGetTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/WindowCacheGetTest.java index f1a18b096..a173532db 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/WindowCacheGetTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/WindowCacheGetTest.java @@ -53,6 +53,8 @@ import java.io.IOException; import java.io.InputStreamReader; import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; import java.util.List; import org.eclipse.jgit.errors.CorruptObjectException; @@ -66,9 +68,25 @@ import org.eclipse.jgit.util.MutableInteger; import org.junit.Before; import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameters; +@RunWith(Parameterized.class) public class WindowCacheGetTest extends SampleDataRepositoryTestCase { private List toLoad; + private WindowCacheConfig cfg; + private boolean useStrongRefs; + + @Parameters(name = "useStrongRefs={0}") + public static Collection data() { + return Arrays + .asList(new Object[][] { { Boolean.TRUE }, { Boolean.FALSE } }); + } + + public WindowCacheGetTest(Boolean useStrongRef) { + this.useStrongRefs = useStrongRef.booleanValue(); + } @Override @Before @@ -93,11 +111,12 @@ public void setUp() throws Exception { } } assertEquals(96, toLoad.size()); + cfg = new WindowCacheConfig(); + cfg.setPackedGitUseStrongRefs(useStrongRefs); } @Test public void testCache_Defaults() throws IOException { - WindowCacheConfig cfg = new WindowCacheConfig(); cfg.install(); doCacheTests(); checkLimits(cfg); @@ -122,7 +141,6 @@ public void testCache_Defaults() throws IOException { @Test public void testCache_TooFewFiles() throws IOException { - final WindowCacheConfig cfg = new WindowCacheConfig(); cfg.setPackedGitOpenFiles(2); cfg.install(); doCacheTests(); @@ -131,7 +149,6 @@ public void testCache_TooFewFiles() throws IOException { @Test public void testCache_TooSmallLimit() throws IOException { - final WindowCacheConfig cfg = new WindowCacheConfig(); cfg.setPackedGitWindowSize(4096); cfg.setPackedGitLimit(4096); cfg.install(); @@ -142,26 +159,31 @@ public void testCache_TooSmallLimit() throws IOException { private static void checkLimits(WindowCacheConfig cfg) { final WindowCache cache = WindowCache.getInstance(); WindowCacheStats s = cache.getStats(); - assertTrue(0 < s.getAverageLoadTime()); - assertTrue(0 < s.getOpenByteCount()); - assertTrue(0 < s.getOpenByteCount()); - assertTrue(0.0 < s.getAverageLoadTime()); - assertTrue(0 <= s.getEvictionCount()); - assertTrue(0 < s.getHitCount()); - assertTrue(0 < s.getHitRatio()); - assertTrue(1 > s.getHitRatio()); - assertTrue(0 < s.getLoadCount()); - assertTrue(0 <= s.getLoadFailureCount()); - assertTrue(0.0 <= s.getLoadFailureRatio()); - assertTrue(1 > s.getLoadFailureRatio()); - assertTrue(0 < s.getLoadSuccessCount()); - assertTrue(s.getOpenByteCount() <= cfg.getPackedGitLimit()); - assertTrue(s.getOpenFileCount() <= cfg.getPackedGitOpenFiles()); - assertTrue(0 <= s.getMissCount()); - assertTrue(0 <= s.getMissRatio()); - assertTrue(1 > s.getMissRatio()); - assertTrue(0 < s.getRequestCount()); - assertTrue(0 < s.getTotalLoadTime()); + assertTrue("average load time should be > 0", + 0 < s.getAverageLoadTime()); + assertTrue("open byte count should be > 0", 0 < s.getOpenByteCount()); + assertTrue("eviction count should be >= 0", 0 <= s.getEvictionCount()); + assertTrue("hit count should be > 0", 0 < s.getHitCount()); + assertTrue("hit ratio should be > 0", 0 < s.getHitRatio()); + assertTrue("hit ratio should be < 1", 1 > s.getHitRatio()); + assertTrue("load count should be > 0", 0 < s.getLoadCount()); + assertTrue("load failure count should be >= 0", + 0 <= s.getLoadFailureCount()); + assertTrue("load failure ratio should be >= 0", + 0.0 <= s.getLoadFailureRatio()); + assertTrue("load failure ratio should be < 1", + 1 > s.getLoadFailureRatio()); + assertTrue("load success count should be > 0", + 0 < s.getLoadSuccessCount()); + assertTrue("open byte count should be <= core.packedGitLimit", + s.getOpenByteCount() <= cfg.getPackedGitLimit()); + assertTrue("open file count should be <= core.packedGitOpenFiles", + s.getOpenFileCount() <= cfg.getPackedGitOpenFiles()); + assertTrue("miss success count should be >= 0", 0 <= s.getMissCount()); + assertTrue("miss ratio should be > 0", 0 <= s.getMissRatio()); + assertTrue("miss ratio should be < 1", 1 > s.getMissRatio()); + assertTrue("request count should be > 0", 0 < s.getRequestCount()); + assertTrue("total load time should be > 0", 0 < s.getTotalLoadTime()); } private void doCacheTests() throws IOException { diff --git a/org.eclipse.jgit/.settings/.api_filters b/org.eclipse.jgit/.settings/.api_filters index 14c22e937..90341ad9d 100644 --- a/org.eclipse.jgit/.settings/.api_filters +++ b/org.eclipse.jgit/.settings/.api_filters @@ -67,6 +67,12 @@ + + + + + + diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/WindowCache.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/WindowCache.java index 797507dd1..e8ab73538 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/WindowCache.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/WindowCache.java @@ -48,6 +48,7 @@ import java.lang.ref.ReferenceQueue; import java.lang.ref.SoftReference; import java.util.Random; +import java.util.concurrent.ConcurrentLinkedQueue; import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReferenceArray; import java.util.concurrent.atomic.LongAdder; @@ -85,9 +86,16 @@ * comprised of roughly 10% of the cache, and evicting the oldest accessed entry * within that window. *

- * Entities created by the cache are held under SoftReferences, permitting the + * Entities created by the cache are held under SoftReferences if option + * {@code core.packedGitUseStrongRefs} is set to {@code false} in the git config + * (this is the default) or by calling + * {@link WindowCacheConfig#setPackedGitUseStrongRefs(boolean)}, permitting the * Java runtime's garbage collector to evict entries when heap memory gets low. * Most JREs implement a loose least recently used algorithm for this eviction. + * When this option is set to {@code true} strong references are used which + * means that Java gc cannot evict the WindowCache to reclaim memory. On the + * other hand this provides more predictable performance since the cache isn't + * flushed when used heap comes close to the maximum heap size. *

* The internal hash table does not expand at runtime, instead it is fixed in * size at cache creation time. The internal lock table used to gate load @@ -104,19 +112,19 @@ * for a given (PackFile,position) tuple. *

  • For every load() invocation there is exactly one * {@link #createRef(PackFile, long, ByteWindow)} invocation to wrap a - * SoftReference around the cached entity.
    • + * SoftReference or a StrongReference around the cached entity. *
  • For every Reference created by createRef() there will be - * exactly one call to {@link #clear(Ref)} to cleanup any resources associated + * exactly one call to {@link #clear(PageRef)} to cleanup any resources associated * with the (now expired) cached entity.
    • * *

    * Therefore, it is safe to perform resource accounting increments during the * {@link #load(PackFile, long)} or * {@link #createRef(PackFile, long, ByteWindow)} methods, and matching - * decrements during {@link #clear(Ref)}. Implementors may need to override + * decrements during {@link #clear(PageRef)}. Implementors may need to override * {@link #createRef(PackFile, long, ByteWindow)} in order to embed additional * accounting information into an implementation specific - * {@link org.eclipse.jgit.internal.storage.file.WindowCache.Ref} subclass, as + * {@link org.eclipse.jgit.internal.storage.file.WindowCache.PageRef} subclass, as * the cached entity may have already been evicted by the JRE's garbage * collector. *

    @@ -390,8 +398,8 @@ static final void purge(PackFile pack) { cache.removeAll(pack); } - /** ReferenceQueue to cleanup released and garbage collected windows. */ - private final ReferenceQueue queue; + /** cleanup released and/or garbage collected windows. */ + private final CleanupQueue queue; /** Number of entries in {@link #table}. */ private final int tableSize; @@ -425,6 +433,8 @@ static final void purge(PackFile pack) { private final StatsRecorderImpl mbean; + private boolean useStrongRefs; + private WindowCache(WindowCacheConfig cfg) { tableSize = tableSize(cfg); final int lockCount = lockCount(cfg); @@ -433,7 +443,6 @@ private WindowCache(WindowCacheConfig cfg) { if (lockCount < 1) throw new IllegalArgumentException(JGitText.get().lockCountMustBeGreaterOrEqual1); - queue = new ReferenceQueue<>(); clock = new AtomicLong(1); table = new AtomicReferenceArray<>(tableSize); locks = new Lock[lockCount]; @@ -455,6 +464,9 @@ else if (eb < 4) mmap = cfg.isPackedGitMMAP(); windowSizeShift = bits(cfg.getPackedGitWindowSize()); windowSize = 1 << windowSizeShift; + useStrongRefs = cfg.isPackedGitUseStrongRefs(); + queue = useStrongRefs ? new StrongCleanupQueue(this) + : new SoftCleanupQueue(this); mbean = new StatsRecorderImpl(); statsRecorder = mbean; @@ -503,16 +515,18 @@ private ByteWindow load(PackFile pack, long offset) throws IOException { } } - private Ref createRef(PackFile p, long o, ByteWindow v) { - final Ref ref = new Ref(p, o, v, queue); - statsRecorder.recordOpenBytes(ref.size); + private PageRef createRef(PackFile p, long o, ByteWindow v) { + final PageRef ref = useStrongRefs + ? new StrongRef(p, o, v, queue) + : new SoftRef(p, o, v, (SoftCleanupQueue) queue); + statsRecorder.recordOpenBytes(ref.getSize()); return ref; } - private void clear(Ref ref) { - statsRecorder.recordOpenBytes(-ref.size); + private void clear(PageRef ref) { + statsRecorder.recordOpenBytes(-ref.getSize()); statsRecorder.recordEvictions(1); - close(ref.pack); + close(ref.getPack()); } private void close(PackFile pack) { @@ -577,7 +591,7 @@ private ByteWindow getOrLoad(PackFile pack, long position) } v = load(pack, position); - final Ref ref = createRef(pack, position, v); + final PageRef ref = createRef(pack, position, v); hit(ref); for (;;) { final Entry n = new Entry(clean(e2), ref); @@ -601,8 +615,8 @@ private ByteWindow getOrLoad(PackFile pack, long position) private ByteWindow scan(Entry n, PackFile pack, long position) { for (; n != null; n = n.next) { - final Ref r = n.ref; - if (r.pack == pack && r.position == position) { + final PageRef r = n.ref; + if (r.getPack() == pack && r.getPosition() == position) { final ByteWindow v = r.get(); if (v != null) { hit(r); @@ -615,7 +629,7 @@ private ByteWindow scan(Entry n, PackFile pack, long position) { return null; } - private void hit(Ref r) { + private void hit(PageRef r) { // We don't need to be 100% accurate here. Its sufficient that at least // one thread performs the increment. Any other concurrent access at // exactly the same time can simply use the same clock value. @@ -625,7 +639,7 @@ private void hit(Ref r) { // final long c = clock.get(); clock.compareAndSet(c, c + 1); - r.lastAccess = c; + r.setLastAccess(c); } private void evict() { @@ -639,7 +653,8 @@ private void evict() { for (Entry e = table.get(ptr); e != null; e = e.next) { if (e.dead) continue; - if (old == null || e.ref.lastAccess < old.ref.lastAccess) { + if (old == null || e.ref.getLastAccess() < old.ref + .getLastAccess()) { old = e; slot = ptr; } @@ -659,7 +674,7 @@ private void evict() { *

    * This is a last-ditch effort to clear out the cache, such as before it * gets replaced by another cache that is configured differently. This - * method tries to force every cached entry through {@link #clear(Ref)} to + * method tries to force every cached entry through {@link #clear(PageRef)} to * ensure that resources are correctly accounted for and cleaned up by the * subclass. A concurrent reader loading entries while this method is * running may cause resource accounting failures. @@ -692,7 +707,7 @@ private void removeAll(PackFile pack) { final Entry e1 = table.get(s); boolean hasDead = false; for (Entry e = e1; e != null; e = e.next) { - if (e.ref.pack == pack) { + if (e.ref.getPack() == pack) { e.kill(); hasDead = true; } else if (e.dead) @@ -705,20 +720,7 @@ private void removeAll(PackFile pack) { } private void gc() { - Ref r; - while ((r = (Ref) queue.poll()) != null) { - clear(r); - - final int s = slot(r.pack, r.position); - final Entry e1 = table.get(s); - for (Entry n = e1; n != null; n = n.next) { - if (n.ref == r) { - n.dead = true; - table.compareAndSet(s, e1, clean(e1)); - break; - } - } - } + queue.gc(); } private int slot(PackFile pack, long position) { @@ -731,7 +733,7 @@ private Lock lock(PackFile pack, long position) { private static Entry clean(Entry top) { while (top != null && top.dead) { - top.ref.enqueue(); + top.ref.kill(); top = top.next; } if (top == null) @@ -745,7 +747,7 @@ private static class Entry { final Entry next; /** The referenced object. */ - final Ref ref; + final PageRef ref; /** * Marked true when ref.get() returns null and the ref is dead. @@ -756,34 +758,275 @@ private static class Entry { */ volatile boolean dead; - Entry(Entry n, Ref r) { + Entry(Entry n, PageRef r) { next = n; ref = r; } final void kill() { dead = true; - ref.enqueue(); + ref.kill(); } } + private static interface PageRef { + /** + * Returns this reference object's referent. If this reference object + * has been cleared, either by the program or by the garbage collector, + * then this method returns null. + * + * @return The object to which this reference refers, or + * null if this reference object has been cleared + */ + T get(); + + /** + * Kill this ref + * + * @return true if this reference object was successfully + * killed; false if it was already killed + */ + boolean kill(); + + /** + * Get the packfile the referenced cache page is allocated for + * + * @return the packfile the referenced cache page is allocated for + */ + PackFile getPack(); + + /** + * Get the position of the referenced cache page in the packfile + * + * @return the position of the referenced cache page in the packfile + */ + long getPosition(); + + /** + * Get size of cache page + * + * @return size of cache page + */ + int getSize(); + + /** + * Get pseudo time of last access to this cache page + * + * @return pseudo time of last access to this cache page + */ + long getLastAccess(); + + /** + * Set pseudo time of last access to this cache page + * + * @param time + * pseudo time of last access to this cache page + */ + void setLastAccess(long time); + + /** + * Whether this is a strong reference. + * @return {@code true} if this is a strong reference + */ + boolean isStrongRef(); + } + /** A soft reference wrapped around a cached object. */ - private static class Ref extends SoftReference { - final PackFile pack; + private static class SoftRef extends SoftReference + implements PageRef { + private final PackFile pack; - final long position; + private final long position; - final int size; + private final int size; - long lastAccess; + private long lastAccess; - protected Ref(final PackFile pack, final long position, - final ByteWindow v, final ReferenceQueue queue) { + protected SoftRef(final PackFile pack, final long position, + final ByteWindow v, final SoftCleanupQueue queue) { super(v, queue); this.pack = pack; this.position = position; this.size = v.size(); } + + @Override + public PackFile getPack() { + return pack; + } + + @Override + public long getPosition() { + return position; + } + + @Override + public int getSize() { + return size; + } + + @Override + public long getLastAccess() { + return lastAccess; + } + + @Override + public void setLastAccess(long time) { + this.lastAccess = time; + } + + @Override + public boolean kill() { + return enqueue(); + } + + @Override + public boolean isStrongRef() { + return false; + } + } + + /** A strong reference wrapped around a cached object. */ + private static class StrongRef implements PageRef { + private ByteWindow referent; + + private final PackFile pack; + + private final long position; + + private final int size; + + private long lastAccess; + + private CleanupQueue queue; + + protected StrongRef(final PackFile pack, final long position, + final ByteWindow v, final CleanupQueue queue) { + this.pack = pack; + this.position = position; + this.referent = v; + this.size = v.size(); + this.queue = queue; + } + + @Override + public PackFile getPack() { + return pack; + } + + @Override + public long getPosition() { + return position; + } + + @Override + public int getSize() { + return size; + } + + @Override + public long getLastAccess() { + return lastAccess; + } + + @Override + public void setLastAccess(long time) { + this.lastAccess = time; + } + + @Override + public ByteWindow get() { + return referent; + } + + @Override + public boolean kill() { + if (referent == null) { + return false; + } + referent = null; + return queue.enqueue(this); + } + + @Override + public boolean isStrongRef() { + return true; + } + } + + private static interface CleanupQueue { + boolean enqueue(PageRef r); + void gc(); + } + + private static class SoftCleanupQueue extends ReferenceQueue + implements CleanupQueue { + private final WindowCache wc; + + SoftCleanupQueue(WindowCache cache) { + this.wc = cache; + } + + @Override + public boolean enqueue(PageRef r) { + // no need to explicitly add soft references which are enqueued by + // the JVM + return false; + } + + @Override + public void gc() { + SoftRef r; + while ((r = (SoftRef) poll()) != null) { + wc.clear(r); + + final int s = wc.slot(r.getPack(), r.getPosition()); + final Entry e1 = wc.table.get(s); + for (Entry n = e1; n != null; n = n.next) { + if (n.ref == r) { + n.dead = true; + wc.table.compareAndSet(s, e1, clean(e1)); + break; + } + } + } + } + } + + private static class StrongCleanupQueue implements CleanupQueue { + private final WindowCache wc; + + private final ConcurrentLinkedQueue> queue = new ConcurrentLinkedQueue<>(); + + StrongCleanupQueue(WindowCache wc) { + this.wc = wc; + } + + @Override + public boolean enqueue(PageRef r) { + if (queue.contains(r)) { + return false; + } + return queue.add(r); + } + + @Override + public void gc() { + PageRef r; + while ((r = queue.poll()) != null) { + wc.clear(r); + + final int s = wc.slot(r.getPack(), r.getPosition()); + final Entry e1 = wc.table.get(s); + for (Entry n = e1; n != null; n = n.next) { + if (n.ref == r) { + n.dead = true; + wc.table.compareAndSet(s, e1, clean(e1)); + break; + } + } + } + } } private static final class Lock { diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java index 9d292f642..003ce238a 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/ConfigConstants.java @@ -224,6 +224,12 @@ public final class ConfigConstants { */ public static final String CONFIG_KEY_PACKED_GIT_OPENFILES = "packedgitopenfiles"; + /** + * The "packedGitUseStrongRefs" key + * @since 5.1.13 + */ + public static final String CONFIG_KEY_PACKED_GIT_USE_STRONGREFS = "packedgitusestrongrefs"; + /** The "remote" key */ public static final String CONFIG_KEY_REMOTE = "remote"; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/WindowCacheConfig.java b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/WindowCacheConfig.java index c04b677df..f066ee18c 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/WindowCacheConfig.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/WindowCacheConfig.java @@ -50,6 +50,7 @@ import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_PACKED_GIT_OPENFILES; import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_PACKED_GIT_WINDOWSIZE; import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_STREAM_FILE_TRESHOLD; +import static org.eclipse.jgit.lib.ConfigConstants.CONFIG_KEY_PACKED_GIT_USE_STRONGREFS; import org.eclipse.jgit.internal.storage.file.WindowCache; import org.eclipse.jgit.lib.Config; @@ -69,6 +70,8 @@ public class WindowCacheConfig { private long packedGitLimit; + private boolean useStrongRefs; + private int packedGitWindowSize; private boolean packedGitMMAP; @@ -83,6 +86,7 @@ public class WindowCacheConfig { public WindowCacheConfig() { packedGitOpenFiles = 128; packedGitLimit = 10 * MB; + useStrongRefs = false; packedGitWindowSize = 8 * KB; packedGitMMAP = false; deltaBaseCacheLimit = 10 * MB; @@ -133,6 +137,31 @@ public void setPackedGitLimit(long newLimit) { packedGitLimit = newLimit; } + /** + * Get whether the window cache should use strong references or + * SoftReferences + * + * @return {@code true} if the window cache should use strong references, + * otherwise it will use {@link java.lang.ref.SoftReference}s + * @since 5.1.13 + */ + public boolean isPackedGitUseStrongRefs() { + return useStrongRefs; + } + + /** + * Set if the cache should use strong refs or soft refs + * + * @param useStrongRefs + * if @{code true} the cache strongly references cache pages + * otherwise it uses {@link java.lang.ref.SoftReference}s which + * can be evicted by the Java gc if heap is almost full + * @since 5.1.13 + */ + public void setPackedGitUseStrongRefs(boolean useStrongRefs) { + this.useStrongRefs = useStrongRefs; + } + /** * Get size in bytes of a single window mapped or read in from the pack * file. @@ -235,6 +264,9 @@ public void setStreamFileThreshold(int newLimit) { * @since 3.0 */ public WindowCacheConfig fromConfig(Config rc) { + setPackedGitUseStrongRefs(rc.getBoolean(CONFIG_CORE_SECTION, + CONFIG_KEY_PACKED_GIT_USE_STRONGREFS, + isPackedGitUseStrongRefs())); setPackedGitOpenFiles(rc.getInt(CONFIG_CORE_SECTION, null, CONFIG_KEY_PACKED_GIT_OPENFILES, getPackedGitOpenFiles())); setPackedGitLimit(rc.getLong(CONFIG_CORE_SECTION, null, From 187b7ad72e97adb3b20aea3b44c3f23c451ea190 Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Mon, 27 Jan 2020 23:00:57 +0100 Subject: [PATCH 4/6] pgm daemon: fallback to user and system config if no config specified If a config file is passed via option --config-file then use only the options defined in that file. This helps to concisely configure the daemon without side effects from global and system level git configs. Otherwise fallback to user and system level configs. Change-Id: I242de248f257579874ad0bfe4882a22502353b1f Signed-off-by: Matthias Sohn --- .../src/org/eclipse/jgit/pgm/Daemon.java | 29 ++++++++++++++----- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Daemon.java b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Daemon.java index 319b5e39d..f989d2e8b 100644 --- a/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Daemon.java +++ b/org.eclipse.jgit.pgm/src/org/eclipse/jgit/pgm/Daemon.java @@ -44,6 +44,7 @@ package org.eclipse.jgit.pgm; import java.io.File; +import java.io.IOException; import java.net.InetSocketAddress; import java.net.URISyntaxException; import java.text.MessageFormat; @@ -51,12 +52,14 @@ import java.util.List; import java.util.concurrent.Executors; +import org.eclipse.jgit.errors.ConfigInvalidException; import org.eclipse.jgit.internal.ketch.KetchLeader; import org.eclipse.jgit.internal.ketch.KetchLeaderCache; import org.eclipse.jgit.internal.ketch.KetchPreReceive; import org.eclipse.jgit.internal.ketch.KetchSystem; import org.eclipse.jgit.internal.ketch.KetchText; import org.eclipse.jgit.lib.Repository; +import org.eclipse.jgit.lib.StoredConfig; import org.eclipse.jgit.pgm.internal.CLIText; import org.eclipse.jgit.storage.file.FileBasedConfig; import org.eclipse.jgit.storage.file.WindowCacheConfig; @@ -69,6 +72,7 @@ import org.eclipse.jgit.transport.resolver.ServiceNotAuthorizedException; import org.eclipse.jgit.transport.resolver.ServiceNotEnabledException; import org.eclipse.jgit.util.FS; +import org.eclipse.jgit.util.SystemReader; import org.kohsuke.args4j.Argument; import org.kohsuke.args4j.Option; @@ -121,19 +125,20 @@ protected boolean requiresRepository() { @Override protected void run() throws Exception { PackConfig packConfig = new PackConfig(); - - if (configFile != null) { + StoredConfig cfg; + if (configFile == null) { + cfg = getUserConfig(); + } else { if (!configFile.exists()) { throw die(MessageFormat.format( CLIText.get().configFileNotFound, // configFile.getAbsolutePath())); } - - FileBasedConfig cfg = new FileBasedConfig(configFile, FS.DETECTED); - cfg.load(); - new WindowCacheConfig().fromConfig(cfg).install(); - packConfig.fromConfig(cfg); + cfg = new FileBasedConfig(configFile, FS.DETECTED); } + cfg.load(); + new WindowCacheConfig().fromConfig(cfg).install(); + packConfig.fromConfig(cfg); int threads = packConfig.getThreads(); if (threads <= 0) @@ -173,6 +178,16 @@ protected void run() throws Exception { outw.println(MessageFormat.format(CLIText.get().listeningOn, d.getAddress())); } + private StoredConfig getUserConfig() throws IOException { + StoredConfig userConfig = null; + try { + userConfig = SystemReader.getInstance().getUserConfig(); + } catch (ConfigInvalidException e) { + throw die(e.getMessage()); + } + return userConfig; + } + private static DaemonService service( final org.eclipse.jgit.transport.Daemon d, final String n) { From 1f9c717ff709e26d44090bc1b71be7c4c2b2a0fd Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Fri, 3 Jan 2020 18:16:42 +0100 Subject: [PATCH 5/6] WindowCache: add metric for cached bytes per repository Since ObjectDatabase and PackFile don't know their repository use the packfile's grand-grand-parent directory as an identifier for the repository the packfile resides in. Remove metric for a repository if the number of cached bytes for the repository drops to 0 in order to ensure the map of cached bytes per repository doesn't contain repositories which have no data cached in the WindowCache. Change-Id: I969ab8029db0a292e7585cbb36ca0baa797da20b Signed-off-by: Matthias Sohn --- .../internal/storage/file/WindowCache.java | 53 +++++++++++++++---- .../jgit/storage/file/WindowCacheStats.java | 9 ++++ 2 files changed, 52 insertions(+), 10 deletions(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/WindowCache.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/WindowCache.java index e8ab73538..3b4f662b9 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/WindowCache.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/WindowCache.java @@ -47,12 +47,16 @@ import java.io.IOException; import java.lang.ref.ReferenceQueue; import java.lang.ref.SoftReference; +import java.util.Collections; +import java.util.Map; import java.util.Random; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentLinkedQueue; import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReferenceArray; import java.util.concurrent.atomic.LongAdder; import java.util.concurrent.locks.ReentrantLock; +import java.util.stream.Collectors; import org.eclipse.jgit.annotations.NonNull; import org.eclipse.jgit.internal.JGitText; @@ -184,18 +188,21 @@ static interface StatsRecorder { /** * Record files opened by cache * - * @param count + * @param delta * delta of number of files opened by cache */ - void recordOpenFiles(int count); + void recordOpenFiles(int delta); /** * Record cached bytes * - * @param count + * @param pack + * pack file the bytes are read from + * + * @param delta * delta of cached bytes */ - void recordOpenBytes(int count); + void recordOpenBytes(PackFile pack, int delta); /** * Returns a snapshot of this recorder's stats. Note that this may be an @@ -217,6 +224,7 @@ static class StatsRecorderImpl private final LongAdder evictionCount; private final LongAdder openFileCount; private final LongAdder openByteCount; + private final Map openByteCountPerRepository; /** * Constructs an instance with all counts initialized to zero. @@ -230,6 +238,7 @@ public StatsRecorderImpl() { evictionCount = new LongAdder(); openFileCount = new LongAdder(); openByteCount = new LongAdder(); + openByteCountPerRepository = new ConcurrentHashMap<>(); } @Override @@ -260,13 +269,28 @@ public void recordEvictions(int count) { } @Override - public void recordOpenFiles(int count) { - openFileCount.add(count); + public void recordOpenFiles(int delta) { + openFileCount.add(delta); } @Override - public void recordOpenBytes(int count) { - openByteCount.add(count); + public void recordOpenBytes(PackFile pack, int delta) { + openByteCount.add(delta); + String repositoryId = repositoryId(pack); + LongAdder la = openByteCountPerRepository + .computeIfAbsent(repositoryId, k -> new LongAdder()); + la.add(delta); + if (delta < 0) { + openByteCountPerRepository.computeIfPresent(repositoryId, + (k, v) -> v.longValue() == 0 ? null : v); + } + } + + private static String repositoryId(PackFile pack) { + // use repository's gitdir since packfile doesn't know its + // repository + return pack.getPackFile().getParentFile().getParentFile() + .getParent(); } @Override @@ -323,6 +347,15 @@ public void resetCounters() { totalLoadTime.reset(); evictionCount.reset(); } + + @Override + public Map getOpenByteCountPerRepository() { + return Collections.unmodifiableMap( + openByteCountPerRepository.entrySet().stream() + .collect(Collectors.toMap(Map.Entry::getKey, + e -> Long.valueOf(e.getValue().sum()), + (u, v) -> v))); + } } private static final int bits(int newSize) { @@ -519,12 +552,12 @@ private PageRef createRef(PackFile p, long o, ByteWindow v) { final PageRef ref = useStrongRefs ? new StrongRef(p, o, v, queue) : new SoftRef(p, o, v, (SoftCleanupQueue) queue); - statsRecorder.recordOpenBytes(ref.getSize()); + statsRecorder.recordOpenBytes(ref.getPack(), ref.getSize()); return ref; } private void clear(PageRef ref) { - statsRecorder.recordOpenBytes(-ref.getSize()); + statsRecorder.recordOpenBytes(ref.getPack(), -ref.getSize()); statsRecorder.recordEvictions(1); close(ref.getPack()); } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/WindowCacheStats.java b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/WindowCacheStats.java index b7f6394df..65f8dae34 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/WindowCacheStats.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/storage/file/WindowCacheStats.java @@ -42,6 +42,8 @@ */ package org.eclipse.jgit.storage.file; +import java.util.Map; + import javax.management.MXBean; import org.eclipse.jgit.internal.storage.file.WindowCache; @@ -225,6 +227,13 @@ default double getAverageLoadTime() { */ long getOpenByteCount(); + /** + * Number of bytes cached per repository + * + * @return number of bytes cached per repository + */ + Map getOpenByteCountPerRepository(); + /** * Reset counters. Does not reset open bytes and open files counters. */ From 7d5300023e867ae1e259bc5ec3839dff2bc3fab8 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Fri, 31 Jan 2020 08:38:24 +0900 Subject: [PATCH 6/6] Fix string format parameter for invalidRefAdvertisementLine The externalized error message added in f4fc640 ("BasePackConnection: Check for expected length of ref advertisement", Dec 18, 2019) uses a malformed string format. Since there is only one formatting argument, it should be referenced with '{0}' rather than '{1}'. Change-Id: Ibda864dfb0bb902fe07ae4bba73117b212046e8a Signed-off-by: David Pursehouse --- .../resources/org/eclipse/jgit/internal/JGitText.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties index fb454d6f2..4251be812 100644 --- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties +++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties @@ -348,7 +348,7 @@ invalidPacketLineHeader=Invalid packet line header: {0} invalidPath=Invalid path: {0} invalidPurgeFactor=Invalid purgeFactor {0}, values have to be in range between 0 and 1 invalidRedirectLocation=Invalid redirect location {0} -> {1} -invalidRefAdvertisementLine=Invalid ref advertisement line: ''{1}'' +invalidRefAdvertisementLine=Invalid ref advertisement line: ''{0}'' invalidReflogRevision=Invalid reflog revision: {0} invalidRefName=Invalid ref name: {0} invalidReftableBlock=Invalid reftable block