>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@xxxxxxxxxxx>
CC: Yann Simon <yann.simon.fr@xxxxxxxxx>
---
Yann Simon <yann.simon.fr@xxxxxxxxx> wrote:
> 2009/3/19 Shawn O. Pearce <spearce@xxxxxxxxxxx>:
> > Yann Simon <yann.simon.fr@xxxxxxxxx> wrote:
> >> From FindBugs:
> >> Using the java.lang.String(String) constructor wastes [...]
>
> However, using the trick newString = new String(aString.substring(),
> i) does not work on all JVM.
> With an IBM JVM, the newString will still contain the original array of chars.
>
> Another solution that work on all JVM could be:
> newString = new String(aString.substring(i).toCharArray())
> Or
> newString = new String(aString.toCharArray(), i, aString.length() - i)
>
> I like the latter one.
I prefer this. It should always do what we want, and at a lower
temporary memory footprint (one less copy of the name). IIRC Robin
rejected it earlier because it wasn't obvious what we were doing. I
say hogwash, its clear as mud.
.../src/org/spearce/jgit/lib/RefDatabase.java | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
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.3.3.507.gc6b5a
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html