Re: [JGIT PATCH 2/2] Allow core.packedGitLimit to exceed "2 g"

[Ferry Huberts <ferry.huberts@pelagic.nl>]

Shawn O. Pearce wrote:
> A 64 bit JVM might actually be able to dedicate more than 2 GiB of
> memory to the window cache, and on a busy server, this may be an
> ideal configuration since JGit can't always reliably use mmap.  By
> treating the limit as a long we increase our range to 2^63, which
> is far beyond what any JVM heap would be able to actually support.
> 
> Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
> ---
>  .../src/org/spearce/jgit/lib/WindowCache.java      |   13 +++++++------
>  .../org/spearce/jgit/lib/WindowCacheConfig.java    |    8 ++++----
>  2 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/WindowCache.java b/org.spearce.jgit/src/org/spearce/jgit/lib/WindowCache.java
> index 0c60853..b6a35ad 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/lib/WindowCache.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/WindowCache.java
> @@ -41,6 +41,7 @@
>  import java.io.IOException;
>  import java.lang.ref.ReferenceQueue;
>  import java.util.concurrent.atomic.AtomicInteger;
> +import java.util.concurrent.atomic.AtomicLong;
>  
>  /**
>   * Caches slices of a {@link PackFile} in memory for faster read access.
> @@ -140,7 +141,7 @@ static final void purge(final PackFile pack) {
>  
>  	private final int maxFiles;
>  
> -	private final int maxBytes;
> +	private final long maxBytes;
>  
>  	private final boolean mmap;
>  
> @@ -150,7 +151,7 @@ static final void purge(final PackFile pack) {
>  
>  	private final AtomicInteger openFiles;
>  
> -	private final AtomicInteger openBytes;
> +	private final AtomicLong openBytes;
>  
>  	private WindowCache(final WindowCacheConfig cfg) {
>  		super(tableSize(cfg), lockCount(cfg));
> @@ -161,7 +162,7 @@ private WindowCache(final WindowCacheConfig cfg) {
>  		windowSize = 1 << windowSizeShift;
>  
>  		openFiles = new AtomicInteger();
> -		openBytes = new AtomicInteger();
> +		openBytes = new AtomicLong();
>  
>  		if (maxFiles < 1)
>  			throw new IllegalArgumentException("Open files must be >= 1");
> @@ -173,7 +174,7 @@ int getOpenFiles() {
>  		return openFiles.get();
>  	}
>  
> -	int getOpenBytes() {
> +	long getOpenBytes() {
>  		return openBytes.get();
>  	}
>  
> @@ -233,12 +234,12 @@ private long toStart(final long offset) {
>  
>  	private static int tableSize(final WindowCacheConfig cfg) {
>  		final int wsz = cfg.getPackedGitWindowSize();
> -		final int limit = cfg.getPackedGitLimit();
> +		final long limit = cfg.getPackedGitLimit();
>  		if (wsz <= 0)
>  			throw new IllegalArgumentException("Invalid window size");
>  		if (limit < wsz)
>  			throw new IllegalArgumentException("Window size must be < limit");
> -		return 5 * (limit / wsz) / 2;
> +		return (int) Math.min(5 * (limit / wsz) / 2, 2000000000);

Math.min returns a long because the prototype Math.min(long,long) will
be chosen. The cast can then overflow and fail. Better change the return
type to a long:
+ return Math.min(5 * (limit / wsz) / 2, 2000000000L);

>  	}
>  
>  	private static int lockCount(final WindowCacheConfig cfg) {
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/WindowCacheConfig.java b/org.spearce.jgit/src/org/spearce/jgit/lib/WindowCacheConfig.java
> index ea28164..97edd3a 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/lib/WindowCacheConfig.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/WindowCacheConfig.java
> @@ -47,7 +47,7 @@
>  
>  	private int packedGitOpenFiles;
>  
> -	private int packedGitLimit;
> +	private long packedGitLimit;
>  
>  	private int packedGitWindowSize;
>  
> @@ -85,7 +85,7 @@ public void setPackedGitOpenFiles(final int fdLimit) {
>  	 * @return maximum number bytes of heap memory to dedicate to caching pack
>  	 *         file data. <b>Default is 10 MB.</b>
>  	 */
> -	public int getPackedGitLimit() {
> +	public long getPackedGitLimit() {
>  		return packedGitLimit;
>  	}
>  
> @@ -94,7 +94,7 @@ public int getPackedGitLimit() {
>  	 *            maximum number bytes of heap memory to dedicate to caching
>  	 *            pack file data.
>  	 */
> -	public void setPackedGitLimit(final int newLimit) {
> +	public void setPackedGitLimit(final long newLimit) {
>  		packedGitLimit = newLimit;
>  	}
>  
> @@ -162,7 +162,7 @@ public void setDeltaBaseCacheLimit(final int newLimit) {
>  	 */
>  	public void fromConfig(final RepositoryConfig rc) {
>  		setPackedGitOpenFiles(rc.getInt("core", null, "packedgitopenfiles", getPackedGitOpenFiles()));
> -		setPackedGitLimit(rc.getInt("core", null, "packedgitlimit", getPackedGitLimit()));
> +		setPackedGitLimit(rc.getLong("core", null, "packedgitlimit", getPackedGitLimit()));
>  		setPackedGitWindowSize(rc.getInt("core", null, "packedgitwindowsize", getPackedGitWindowSize()));
>  		setPackedGitMMAP(rc.getBoolean("core", null, "packedgitmmap", isPackedGitMMAP()));
>  		setDeltaBaseCacheLimit(rc.getInt("core", null, "deltabasecachelimit", getDeltaBaseCacheLimit()));