{"thread":{"id":"19148","subject":"[JGIT PATCH v3] Replace inefficient new String(String) constructor to silence FindBugs","startedAt":"2009-05-01T15:54:32Z","lastAt":"2009-05-04T21:23:51Z","messageCount":2,"participants":["Shawn O. Pearce","Sohn, Matthias"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"112826","messageId":"1241193272-20247-1-git-send-email-spearce@spearce.org","threadId":"19148","inReplyTo":null,"subject":"[JGIT PATCH v3] Replace inefficient new String(String) constructor to silence FindBugs","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-05-01T15:54:32Z","receivedAt":"2009-05-01T15:54:32Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"FindBugs keeps reporting that our usage of new String(String)\nis not the most efficient way to construct a string.\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/113739/focus=113787\n> I had a specific reason for forcing a new String object here.\n>\n> The line in question, p, is from the packed-refs file and\n> contains the entire SHA-1 in hex form at the beginning of it.\n> We've converted that into binary as an ObjectId, it uses 1/4 the\n> space of the string portion.\n>\n> The Ref object, its ObjectId, and its name string, are going to be\n> cached in a Map, probably long-term. We're better off shedding the\n> 80 bytes of memory used to hold the hex SHA-1 then risk substring()\n> deciding its \"faster\" to reuse the char[] then to make a copy of it.\n\nAnother way to force this new unique String instance with its own\nprivate char[] is to use a StringBuilder and append onto it the\nref name. This shouldn't be a warning for FindBugs, but it would\naccomplish the same goal of producing 1 clean copy, with no extra\ntransient temporary array.\n\nSigned-off-by: Shawn O. Pearce \nCC: Yann Simon \nCC: Matthias Sohn \n---\n\n A less ugly version ?\n\n .../src/org/spearce/jgit/lib/RefDatabase.java | 9 ++++++++-\n 1 files changed, 8 insertions(+), 1 deletions(-)\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java b/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java\nindex 87f26bf..a865fba 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java\n@@ -447,7 +447,7 @@ private synchronized void refreshPackedRefs() {\n \n \t\t\t\t\tfinal int sp = p.indexOf(' ');\n \t\t\t\t\tfinal ObjectId id = ObjectId.fromString(p.substring(0, sp));\n-\t\t\t\t\tfinal String name = new String(p.substring(sp + 1));\n+\t\t\t\t\tfinal String name = copy(p.substring(sp + 1));\n \t\t\t\t\tlast = new Ref(Ref.Storage.PACKED, name, name, id);\n \t\t\t\t\tnewPackedRefs.put(last.getName(), last);\n \t\t\t\t}\n@@ -469,6 +469,13 @@ private synchronized void refreshPackedRefs() {\n \t\t}\n \t}\n \n+\tprivate static String copy(final String src) {\n+\t\t// Force a deep copy of the underlying char[] so that we can\n+\t\t// discard any garbage from any shared char[] within src.\n+\t\t//\n+\t\treturn new StringBuilder(src.length()).append(src).toString();\n+\t}\n+\n \tprivate void lockAndWriteFile(File file, byte[] content) throws IOException {\n \t\tString name = file.getName();\n \t\tfinal LockFile lck = new LockFile(file);\n-- \n1.6.3.rc3.212.g8c698\n"},{"id":"113006","messageId":"366BBB1215D0AB4B8A153AF047A2878003073DC3@dewdfe18.wdf.sap.corp","threadId":"19148","inReplyTo":"1241193272-20247-1-git-send-email-spearce@spearce.org","subject":"RE: [JGIT PATCH v3] Replace inefficient new String(String) constructor to silence FindBugs","fromName":"Sohn, Matthias","fromEmail":"matthias.sohn@sap.com","sentAt":"2009-05-04T21:23:51Z","receivedAt":"2009-05-04T21:23:51Z","isPatch":true,"sender":{"key":"matthias.sohn@sap.com","avatar":"https://gravatar.com/avatar/88bbb2733bcb977ec2d2cc1916ba8a70d6d41432c146bb8dab4f7f802261194e?d=mp&s=160"},"body":"Shawn O. Pearce [mailto:spearce@spearce.org] wrote :\n \n> FindBugs keeps reporting that our usage of new String(String)\n> is not the most efficient way to construct a string.\n> \n> http://thread.gmane.org/gmane.comp.version-\n> control.git/113739/focus=113787\n> > I had a specific reason for forcing a new String object here.\n> >\n> > The line in question, p, is from the packed-refs file and\n> > contains the entire SHA-1 in hex form at the beginning of it.\n> > We've converted that into binary as an ObjectId, it uses 1/4 the\n> > space of the string portion.\n> >\n> > The Ref object, its ObjectId, and its name string, are going to be\n> > cached in a Map, probably long-term. We're better off shedding the\n> > 80 bytes of memory used to hold the hex SHA-1 then risk substring()\n> > deciding its \"faster\" to reuse the char[] then to make a copy of it.\n> \n> Another way to force this new unique String instance with its own\n> private char[] is to use a StringBuilder and append onto it the\n> ref name. This shouldn't be a warning for FindBugs, but it would\n> accomplish the same goal of producing 1 clean copy, with no extra\n> transient temporary array.\n> \n> Signed-off-by: Shawn O. Pearce \n> CC: Yann Simon \n> CC: Matthias Sohn \n> ---\n> \n> A less ugly version ?\n\nI agree, this looks better than the previous proposal but still a simple\nString copy constructor looks even simpler.\n\nI tried the alternative approach Robin proposed using FindBugs filter \nmechanisms to suppress the undesired warning. I will post that in my \nnext mail.\n\n--\nMatthias\n"}]}