[JGIT PATCH 1/2] Add getLong to RepositoryConfig

Subsystems: the rest

STALE3730d

7 messages, 2 authors, 2016-06-15 · open the first message on its own page

[JGIT PATCH 1/2] Add getLong to RepositoryConfig

From: Shawn O. Pearce <hidden>
Date: 2016-06-15 22:46:56

This supports parsing 64 bit configuration values.  We want to use
it for values like core.packedGitLimit where a 64 bit JVM may want
to support a very large value, well past the 2 GiB barrier.

Signed-off-by: Shawn O. Pearce <redacted>
---
 .../org/spearce/jgit/lib/RepositoryConfigTest.java |   26 ++++++++++++++++++++
 .../src/org/spearce/jgit/lib/RepositoryConfig.java |   26 ++++++++++++++++++-
 2 files changed, 50 insertions(+), 2 deletions(-)
diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryConfigTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryConfigTest.java
index ed573e1..5e2328b 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryConfigTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryConfigTest.java
@@ -233,6 +233,32 @@ public void testReadBoolean_OnOff2() throws IOException {
 		assertFalse(c.getBoolean("s", "b", true));
 	}
 
+	public void testReadLong() throws IOException {
+		assertReadLong(1L);
+		assertReadLong(-1L);
+		assertReadLong(Long.MIN_VALUE);
+		assertReadLong(Long.MAX_VALUE);
+		assertReadLong(4L * 1024 * 1024 * 1024, "4g");
+		assertReadLong(3L * 1024 * 1024, "3 m");
+		assertReadLong(8L * 1024, "8 k");
+
+		try {
+			assertReadLong(-1, "1.5g");
+			fail("incorrectly accepted 1.5g");
+		} catch (IllegalArgumentException e) {
+			assertEquals("Invalid integer value: s.a=1.5g", e.getMessage());
+		}
+	}
+
+	private void assertReadLong(long exp) throws IOException {
+		assertReadLong(exp, String.valueOf(exp));
+	}
+
+	private void assertReadLong(long exp, String act) throws IOException {
+		final RepositoryConfig c = read("[s]\na = " + act + "\n");
+		assertEquals(exp, c.getLong("s", null, "a", 0L));
+	}
+
 	private RepositoryConfig read(final String content) throws IOException {
 		final File p = writeTrashFile(getName() + ".config", content);
 		final RepositoryConfig c = new RepositoryConfig(null, p);
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RepositoryConfig.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RepositoryConfig.java
index b816604..a339514 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/RepositoryConfig.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RepositoryConfig.java
@@ -183,6 +183,28 @@ public int getInt(final String section, final String name,
 	 */
 	public int getInt(final String section, String subsection,
 			final String name, final int defaultValue) {
+		final long val = getLong(section, subsection, name, defaultValue);
+		if (Integer.MIN_VALUE <= val && val <= Integer.MAX_VALUE)
+			return (int) val;
+		throw new IllegalArgumentException("Integer value " + section + "."
+				+ name + " out of range");
+	}
+
+	/**
+	 * Obtain an integer value from the configuration.
+	 *
+	 * @param section
+	 *            section the key is grouped within.
+	 * @param subsection
+	 *            subsection name, such a remote or branch name.
+	 * @param name
+	 *            name of the key to get.
+	 * @param defaultValue
+	 *            default value to return if no value was present.
+	 * @return an integer value from the configuration, or defaultValue.
+	 */
+	public long getLong(final String section, String subsection,
+			final String name, final long defaultValue) {
 		final String str = getString(section, subsection, name);
 		if (str == null)
 			return defaultValue;
@@ -191,7 +213,7 @@ public int getInt(final String section, String subsection,
 		if (n.length() == 0)
 			return defaultValue;
 
-		int mul = 1;
+		long mul = 1;
 		switch (Character.toLowerCase(n.charAt(n.length() - 1))) {
 		case 'g':
 			mul = 1024 * 1024 * 1024;
@@ -209,7 +231,7 @@ public int getInt(final String section, String subsection,
 			return defaultValue;
 
 		try {
-			return mul * Integer.parseInt(n);
+			return mul * Long.parseLong(n);
 		} catch (NumberFormatException nfe) {
 			throw new IllegalArgumentException("Invalid integer value: "
 					+ section + "." + name + "=" + str);
-- 
1.6.3.2.367.gf0de

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

From: Shawn O. Pearce <hidden>
Date: 2016-06-15 22:46:56

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 <redacted>
---
 .../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);
 	}
 
 	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()));
-- 
1.6.3.2.367.gf0de

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

From: Ferry Huberts <hidden>
Date: 2016-06-15 22:46:56

Shawn O. Pearce wrote:
quoted hunk
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 <redacted>
---
 .../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);
quoted hunk
 	}
 
 	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()));

Re: [JGIT PATCH 1/2] Add getLong to RepositoryConfig

From: Ferry Huberts <hidden>
Date: 2016-06-15 22:46:56

Shawn O. Pearce wrote:
quoted hunk
This supports parsing 64 bit configuration values.  We want to use
it for values like core.packedGitLimit where a 64 bit JVM may want
to support a very large value, well past the 2 GiB barrier.

Signed-off-by: Shawn O. Pearce <redacted>
---
 .../org/spearce/jgit/lib/RepositoryConfigTest.java |   26 ++++++++++++++++++++
 .../src/org/spearce/jgit/lib/RepositoryConfig.java |   26 ++++++++++++++++++-
 2 files changed, 50 insertions(+), 2 deletions(-)
diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryConfigTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryConfigTest.java
index ed573e1..5e2328b 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryConfigTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/lib/RepositoryConfigTest.java
@@ -233,6 +233,32 @@ public void testReadBoolean_OnOff2() throws IOException {
 		assertFalse(c.getBoolean("s", "b", true));
 	}
 
+	public void testReadLong() throws IOException {
+		assertReadLong(1L);
+		assertReadLong(-1L);
+		assertReadLong(Long.MIN_VALUE);
+		assertReadLong(Long.MAX_VALUE);
+		assertReadLong(4L * 1024 * 1024 * 1024, "4g");
+		assertReadLong(3L * 1024 * 1024, "3 m");
+		assertReadLong(8L * 1024, "8 k");
+
+		try {
+			assertReadLong(-1, "1.5g");
+			fail("incorrectly accepted 1.5g");
+		} catch (IllegalArgumentException e) {
+			assertEquals("Invalid integer value: s.a=1.5g", e.getMessage());
+		}
+	}
+
+	private void assertReadLong(long exp) throws IOException {
+		assertReadLong(exp, String.valueOf(exp));
+	}
+
+	private void assertReadLong(long exp, String act) throws IOException {
+		final RepositoryConfig c = read("[s]\na = " + act + "\n");
+		assertEquals(exp, c.getLong("s", null, "a", 0L));
+	}
+
 	private RepositoryConfig read(final String content) throws IOException {
 		final File p = writeTrashFile(getName() + ".config", content);
 		final RepositoryConfig c = new RepositoryConfig(null, p);
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RepositoryConfig.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RepositoryConfig.java
index b816604..a339514 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/RepositoryConfig.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RepositoryConfig.java
@@ -183,6 +183,28 @@ public int getInt(final String section, final String name,
 	 */
 	public int getInt(final String section, String subsection,
 			final String name, final int defaultValue) {
+		final long val = getLong(section, subsection, name, defaultValue);
+		if (Integer.MIN_VALUE <= val && val <= Integer.MAX_VALUE)
+			return (int) val;
+		throw new IllegalArgumentException("Integer value " + section + "."
+				+ name + " out of range");
+	}
+
+	/**
+	 * Obtain an integer value from the configuration.
s/integer/long/g in this (copy&paste) javadoc part
quoted hunk
+	 *
+	 * @param section
+	 *            section the key is grouped within.
+	 * @param subsection
+	 *            subsection name, such a remote or branch name.
+	 * @param name
+	 *            name of the key to get.
+	 * @param defaultValue
+	 *            default value to return if no value was present.
+	 * @return an integer value from the configuration, or defaultValue.
+	 */
+	public long getLong(final String section, String subsection,
+			final String name, final long defaultValue) {
 		final String str = getString(section, subsection, name);
 		if (str == null)
 			return defaultValue;
@@ -191,7 +213,7 @@ public int getInt(final String section, String subsection,
 		if (n.length() == 0)
 			return defaultValue;
 
-		int mul = 1;
+		long mul = 1;
 		switch (Character.toLowerCase(n.charAt(n.length() - 1))) {
 		case 'g':
 			mul = 1024 * 1024 * 1024;
@@ -209,7 +231,7 @@ public int getInt(final String section, String subsection,
 			return defaultValue;
 
 		try {
-			return mul * Integer.parseInt(n);
+			return mul * Long.parseLong(n);
 		} catch (NumberFormatException nfe) {
 			throw new IllegalArgumentException("Invalid integer value: "
 					+ section + "." + name + "=" + str);

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

From: Shawn O. Pearce <hidden>
Date: 2016-06-15 22:46:57

Ferry Huberts [off-list ref] wrote:
Shawn O. Pearce wrote:
quoted
A 64 bit JVM might actually be able to dedicate more than 2 GiB of
Please don't quote everything if you are only replying to a tiny
part.
quoted
 	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);
If you looked at that, 2,000,000,000 is within the range of an int.
We select the smallest value.  The first argument expression is
computed as a long, so we shouldn't ever overflow and cause the
first argument to be negative.  If the first argument is larger
than 2 billion, then it does risk overflow, but the 2nd argument
is smaller, so it is returned.

The code is fine as is.
 
-- 
Shawn.

Re: [JGIT PATCH 1/2] Add getLong to RepositoryConfig

From: Shawn O. Pearce <hidden>
Date: 2016-06-15 22:46:57

Ferry Huberts [off-list ref] wrote:
Shawn O. Pearce wrote:
quoted
This supports parsing 64 bit configuration values.
...
quoted
+	/**
+	 * Obtain an integer value from the configuration.
s/integer/long/g in this (copy&paste) javadoc part
Its still integer as in from the integer number space, and not say
a floating point decimal.  Thus I didn't bother with this edit,
I figured the method name was clear enough that it would return
a long, but the concept of it parsing only "2g" and not "1.5g"
should go in the documentation.
 
And again, please trim parts you aren't replying to.

-- 
Shawn.

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

From: Ferry Huberts <hidden>
Date: 2016-06-15 22:46:57

If you looked at that, 2,000,000,000 is within the range of an int.
We select the smallest value.  The first argument expression is
computed as a long, so we shouldn't ever overflow and cause the
first argument to be negative.  If the first argument is larger
than 2 billion, then it does risk overflow, but the 2nd argument
is smaller, so it is returned.

The code is fine as is.
 
I stand corrected :-)
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help