[JGIT PATCH v2] FindBugs: don't use new String(String) in RefDatabase

From: Shawn O. Pearce <spearce@spearce.org>
Date: Jul 13, 2009, 14:53 UTC
Message-ID: <20090713145308.GI11191@spearce.org>
In-Reply-To: <551f769b0907130107j51d32e4er54e125f9dc61dd80@mail.gmail.com>

>From FindBugs:

  Using the java.lang.String(String) constructor wastes memory
  because the object so constructed will be functionally
  indistinguishable from the String passed as a parameter. Just
  use the argument String directly.

Actually, here we want to get a new String object that covers only the portion of the source string that we are selected out.

The line in question, p, is from the packed-refs file and contains the entire SHA-1 in hex form at the beginning of it. We have already converted that into binary as an ObjectId, which uses 1/4 the space of the string portion.

The Ref object, its ObjectId, and its name string, are going to be cached in a Map, probably long-term, as the packed-refs file does not change frequently. We are better off shedding the 80 bytes of memory used to hold the hex SHA-1 then risk substring() deciding its "better" to reuse the same char[] internally.

By creating a new StringBuilder of the exact required capacity for the name, and then copying in the region of characters we really want, we defeat the reuse substring() would normally perform, at the tiny cost of an extra StringBuilder temporary. Some JITs are able to stack allocate that here, making it a trivial cost.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
CC: Yann Simon <yann.simon.fr@gmail.com>
---
 Yann Simon <yann.simon.fr@gmail.com> wrote:
 > This method is quite clear.
 > One line javadoc would make it even clearer... :p (and maybe make Robin happy)

 Javadoc is overrated.  Private utility methods like this that are one
 line long don't need documentation.  The rationale for why this line
 does what it does is something that `git blame` can answer better.

 > And you're right: by using a StringBuilder, we need one less arraycopy.
 > 
 > After committing your change, we can remove the entry to silent FindBugs.
 > (commit 21c3d82824075cd1f140b3bcf252dfaffe0fc96c)

 My patch is updated (below).  Thanks, I forgot about that filter.

 .../findBugs/FindBugsExcludeFilter.xml             |    7 -------
 .../src/org/spearce/jgit/lib/RefDatabase.java      |    6 +++++-
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/org.spearce.jgit/findBugs/FindBugsExcludeFilter.xml b/org.spearce.jgit/findBugs/FindBugsExcludeFilter.xml
index 2af9348..a553170 100644
--- a/org.spearce.jgit/findBugs/FindBugsExcludeFilter.xml
+++ b/org.spearce.jgit/findBugs/FindBugsExcludeFilter.xml
@@ -1,12 +1,5 @@
 <?xml version="1.0" encoding="UTF-8" ?>
 <FindBugsFilter>
-     <!-- Silence inefficient new String(String) constructor warning, see http://thread.gmane.org/gmane.comp.version-control.git/117831/focus=117937 -->
-     <Match>
-       <Class name="org.spearce.jgit.lib.RefDatabase" />
-       <Method name="refreshPackedRefs" />
-       <Bug pattern="DM_STRING_CTOR" />
-     </Match>
-
      <!-- Silence PackFile.mmap calls GC, we need to force it to remove stale
           memory mapped segments if the JVM heap is out of address space.
        -->
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
index 6d4f374..383877f 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
@@ -438,7 +438,7 @@ private synchronized void refreshPackedRefs() {
 
 					final int sp = p.indexOf(' ');
 					final ObjectId id = ObjectId.fromString(p.substring(0, sp));
-					final String name = new String(p.substring(sp + 1));
+					final String name = copy(p, sp + 1, p.length());
 					last = new Ref(Ref.Storage.PACKED, name, name, id);
 					newPackedRefs.put(last.getName(), last);
 				}
@@ -460,6 +460,10 @@ private synchronized void refreshPackedRefs() {
 		}
 	}
 
+	private static String copy(final String src, final int off, final int end) {
+		return new StringBuilder(end - off).append(src, off, end).toString();
+	}
+
 	private void lockAndWriteFile(File file, byte[] content) throws IOException {
 		String name = file.getName();
 		final LockFile lck = new LockFile(file);

-- 
1.6.4.rc0.117.g28cb