# [JGIT PATCH v3] Replace inefficient new String(String) constructor to silence FindBugs 2 messages from 2009-05-01 to 2009-05-04. Participants: Shawn O. Pearce, Sohn, Matthias. Thread: https://gitlist.dev/t/19148 ## Shawn O. Pearce, 2009-05-01 15:54 Subject: [JGIT PATCH v3] Replace inefficient new String(String) constructor to silence FindBugs Message-ID: <1241193272-20247-1-git-send-email-spearce@spearce.org> URL: https://gitlist.dev/e/1241193272-20247-1-git-send-email-spearce%40spearce.org ``` FindBugs keeps reporting that our usage of new String(String) is not the most efficient way to construct a string. http://thread.gmane.org/gmane.comp.version-control.git/113739/focus=113787 > I had a specific reason for forcing a new String object here. > > 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've converted that into binary as an ObjectId, it 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. We're better off shedding the > 80 bytes of memory used to hold the hex SHA-1 then risk substring() > deciding its "faster" to reuse the char[] then to make a copy of it. Another way to force this new unique String instance with its own private char[] is to use a StringBuilder and append onto it the ref name. This shouldn't be a warning for FindBugs, but it would accomplish the same goal of producing 1 clean copy, with no extra transient temporary array. Signed-off-by: Shawn O. Pearce CC: Yann Simon CC: Matthias Sohn --- A less ugly version ? .../src/org/spearce/jgit/lib/RefDatabase.java | 9 ++++++++- 1 files changed, 8 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 87f26bf..a865fba 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java @@ -447,7 +447,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.substring(sp + 1)); last = new Ref(Ref.Storage.PACKED, name, name, id); newPackedRefs.put(last.getName(), last); } @@ -469,6 +469,13 @@ private synchronized void refreshPackedRefs() { } } + private static String copy(final String src) { + // Force a deep copy of the underlying char[] so that we can + // discard any garbage from any shared char[] within src. + // + return new StringBuilder(src.length()).append(src).toString(); + } + private void lockAndWriteFile(File file, byte[] content) throws IOException { String name = file.getName(); final LockFile lck = new LockFile(file); -- 1.6.3.rc3.212.g8c698 ``` ## Sohn, Matthias, 2009-05-04 21:23 Subject: RE: [JGIT PATCH v3] Replace inefficient new String(String) constructor to silence FindBugs Message-ID: <366BBB1215D0AB4B8A153AF047A2878003073DC3@dewdfe18.wdf.sap.corp> URL: https://gitlist.dev/e/366BBB1215D0AB4B8A153AF047A2878003073DC3%40dewdfe18.wdf.sap.corp In-Reply-To: <1241193272-20247-1-git-send-email-spearce@spearce.org> ``` Shawn O. Pearce [mailto:spearce@spearce.org] wrote : > FindBugs keeps reporting that our usage of new String(String) > is not the most efficient way to construct a string. > > http://thread.gmane.org/gmane.comp.version- > control.git/113739/focus=113787 > > I had a specific reason for forcing a new String object here. > > > > 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've converted that into binary as an ObjectId, it 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. We're better off shedding the > > 80 bytes of memory used to hold the hex SHA-1 then risk substring() > > deciding its "faster" to reuse the char[] then to make a copy of it. > > Another way to force this new unique String instance with its own > private char[] is to use a StringBuilder and append onto it the > ref name. This shouldn't be a warning for FindBugs, but it would > accomplish the same goal of producing 1 clean copy, with no extra > transient temporary array. > > Signed-off-by: Shawn O. Pearce > CC: Yann Simon > CC: Matthias Sohn > --- > > A less ugly version ? I agree, this looks better than the previous proposal but still a simple String copy constructor looks even simpler. I tried the alternative approach Robin proposed using FindBugs filter mechanisms to suppress the undesired warning. I will post that in my next mail. -- Matthias ```