{"thread":{"id":"19097","subject":"[PATCH JGIT] Method invokes inefficient Number constructor; use static valueOf instead","startedAt":"2009-04-27T23:02:55Z","lastAt":"2009-04-28T22:26:15Z","messageCount":9,"participants":["Sohn, Matthias","Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"112487","messageId":"366BBB1215D0AB4B8A153AF047A2878002FCE7E7@dewdfe18.wdf.sap.corp","threadId":"19097","inReplyTo":null,"subject":"[PATCH JGIT] Computation of average could overflow","fromName":"Sohn, Matthias","fromEmail":"matthias.sohn@sap.com","sentAt":"2009-04-27T23:02:55Z","receivedAt":"2009-04-27T23:02:55Z","isPatch":true,"sender":{"key":"matthias.sohn@sap.com","avatar":"https://gravatar.com/avatar/88bbb2733bcb977ec2d2cc1916ba8a70d6d41432c146bb8dab4f7f802261194e?d=mp&s=160"},"body":"The code computes the average of two integers using either division or\nsigned right shift, and then uses the result as the\nindex of an array. If the values being averaged are very large, this can\noverflow (resulting in the computation of a negative\naverage). Assuming that the result is intended to be nonnegative, you\ncan use an unsigned right shift instead. In other\nwords, rather that using (low+high)/2, use (low+high) >>> 1\n\nThis bug exists in many earlier implementations of binary search and\nmerge sort. Martin Buchholz found and fixed it\n(http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6412541) in the JDK\nlibraries, and Joshua Bloch widely publicized\nthe bug pattern\n(http://googleresearch.blogspot.com/2006/06/extra-extra-read-all-about-i\nt-nearly.html).\n\nSigned-off-by: Matthias Sohn \n---\n .../src/org/spearce/jgit/dircache/DirCache.java | 2 +-\n .../src/org/spearce/jgit/lib/Tree.java | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git\na/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCache.java\nb/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCache.java\nindex 58da014..fa906fa 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCache.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCache.java\n@@ -593,7 +593,7 @@ int findEntry(final byte[] p, final int pLen) {\n \t\tint low = 0;\n \t\tint high = entryCnt;\n \t\tdo {\n-\t\t\tint mid = (low + high) >> 1;\n+\t\t\tint mid = (low + high) >>> 1;\n \t\t\tfinal int cmp = cmp(p, pLen,\nsortedEntries[mid]);\n \t\t\tif (cmp < 0)\n \t\t\t\thigh = mid;\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/Tree.java\nb/org.spearce.jgit/src/org/spearce/jgit/lib/Tree.java\nindex 0ecd04d..ff9e666 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/lib/Tree.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/Tree.java\n@@ -136,7 +136,7 @@ private static final int binarySearch(final\nTreeEntry[] entries,\n \t\tint high = entries.length;\n \t\tint low = 0;\n \t\tdo {\n-\t\t\tfinal int mid = (low + high) / 2;\n+\t\t\tfinal int mid = (low + high) >>> 1;\n \t\t\tfinal int cmp =\ncompareNames(entries[mid].getNameUTF8(), nameUTF8,\n \t\t\t\t\tnameStart, nameEnd,\nTreeEntry.lastChar(entries[mid]), nameUTF8last);\n \t\t\tif (cmp < 0)\n-- \n1.6.2.2.1669.g7eaf8\n"},{"id":"112492","messageId":"366BBB1215D0AB4B8A153AF047A2878002FCE7E8@dewdfe18.wdf.sap.corp","threadId":"19097","inReplyTo":"366BBB1215D0AB4B8A153AF047A2878002FCE7E7@dewdfe18.wdf.sap.corp","subject":"[PATCH JGIT] Method invokes inefficient new String(String) constructor","fromName":"Sohn, Matthias","fromEmail":"matthias.sohn@sap.com","sentAt":"2009-04-27T23:05:43Z","receivedAt":"2009-04-27T23:05:43Z","isPatch":true,"sender":{"key":"matthias.sohn@sap.com","avatar":"https://gravatar.com/avatar/88bbb2733bcb977ec2d2cc1916ba8a70d6d41432c146bb8dab4f7f802261194e?d=mp&s=160"},"body":"Using the java.lang.String(String) constructor wastes memory because the\nobject so constructed will be functionally\nindistinguishable from the String passed as a parameter. Just use the\nargument String directly.\n\nSigned-off-by: Matthias Sohn \n---\n .../src/org/spearce/jgit/lib/RefDatabase.java | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java\nb/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java\nindex 87f26bf..49da538 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 =\nObjectId.fromString(p.substring(0, sp));\n-\t\t\t\t\tfinal String name = new\nString(p.substring(sp + 1));\n+\t\t\t\t\tfinal String name =\np.substring(sp + 1);\n \t\t\t\t\tlast = new\nRef(Ref.Storage.PACKED, name, name, id);\n \nnewPackedRefs.put(last.getName(), last);\n \t\t\t\t}\n-- \n1.6.2.2.1669.g7eaf8\n"},{"id":"112474","messageId":"366BBB1215D0AB4B8A153AF047A2878002FCE7E9@dewdfe18.wdf.sap.corp","threadId":"19097","inReplyTo":"366BBB1215D0AB4B8A153AF047A2878002FCE7E8@dewdfe18.wdf.sap.corp","subject":"[PATCH JGIT] Method invokes inefficient Number constructor; use static valueOf instead","fromName":"Sohn, Matthias","fromEmail":"matthias.sohn@sap.com","sentAt":"2009-04-27T23:08:17Z","receivedAt":"2009-04-27T23:08:17Z","isPatch":true,"sender":{"key":"matthias.sohn@sap.com","avatar":"https://gravatar.com/avatar/88bbb2733bcb977ec2d2cc1916ba8a70d6d41432c146bb8dab4f7f802261194e?d=mp&s=160"},"body":"Using new Integer(int) is guaranteed to always result in a new object\nwhereas Integer.valueOf(int) allows caching of values\nto be done by the compiler, class library, or JVM. Using of cached\nvalues avoids object allocation and the code will be faster.\n\nValues between -128 and 127 are guaranteed to have corresponding cached\ninstances and using valueOf is approximately\n3.5 times faster than using constructor. For values outside the constant\nrange the performance of both styles is the same.\n\nUnless the class must be compatible with JVMs predating Java 1.5, use\neither autoboxing or the valueOf() method when\ncreating instances of Long, Integer, Short, Character, and Byte.\n\nSigned-off-by: Matthias Sohn \n---\n .../src/org/spearce/jgit/transport/IndexPack.java | 6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git\na/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java\nb/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java\nindex e0e4855..04ef59d 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java\n@@ -383,7 +383,7 @@ private void resolveDeltas(final ProgressMonitor\nprogress)\n \tprivate void resolveDeltas(final PackedObjectInfo oe) throws\nIOException {\n \t\tfinal int oldCRC = oe.getCRC();\n \t\tif (baseById.containsKey(oe)\n-\t\t\t\t|| baseByPos.containsKey(new\nLong(oe.getOffset())))\n+\t\t\t\t||\nbaseByPos.containsKey(Long.valueOf(oe.getOffset())))\n \t\t\tresolveDeltas(oe.getOffset(), oldCRC,\nConstants.OBJ_BAD, null, oe);\n \t}\n \n@@ -448,7 +448,7 @@ private void resolveDeltas(final long pos, final int\noldCRC, int type,\n \tprivate void resolveChildDeltas(final long pos, int type, byte[]\ndata,\n \t\t\tPackedObjectInfo oe) throws IOException {\n \t\tfinal ArrayList a =\nbaseById.remove(oe);\n-\t\tfinal ArrayList b =\nbaseByPos.remove(new Long(pos));\n+\t\tfinal ArrayList b =\nbaseByPos.remove(Long.valueOf(pos));\n \t\tint ai = 0, bi = 0;\n \t\tif (a != null && b != null) {\n \t\t\twhile (ai < a.size() && bi < b.size()) {\n@@ -679,7 +679,7 @@ private void indexOneObject() throws IOException {\n \t\t\t\tofs <<= 7;\n \t\t\t\tofs += (c & 127);\n \t\t\t}\n-\t\t\tfinal Long base = new Long(pos - ofs);\n+\t\t\tfinal Long base = Long.valueOf(pos - ofs);\n \t\t\tArrayList r =\nbaseByPos.get(base);\n \t\t\tif (r == null) {\n \t\t\t\tr = new ArrayList(8);\n-- \n1.6.2.2.1669.g7eaf8\n"},{"id":"112476","messageId":"366BBB1215D0AB4B8A153AF047A2878002FCE7EA@dewdfe18.wdf.sap.corp","threadId":"19097","inReplyTo":"366BBB1215D0AB4B8A153AF047A2878002FCE7E9@dewdfe18.wdf.sap.corp","subject":"[PATCH JGIT] Method ignores results of InputStream.skip()","fromName":"Sohn, Matthias","fromEmail":"matthias.sohn@sap.com","sentAt":"2009-04-27T23:10:35Z","receivedAt":"2009-04-27T23:10:35Z","isPatch":true,"sender":{"key":"matthias.sohn@sap.com","avatar":"https://gravatar.com/avatar/88bbb2733bcb977ec2d2cc1916ba8a70d6d41432c146bb8dab4f7f802261194e?d=mp&s=160"},"body":"This method ignores the return value of java.io.InputStream.skip() which\ncan skip multiple bytes. If the return value is not\nchecked, the caller will not be able to correctly handle the case where\nfewer bytes were skipped than the caller requested.\nThis is a particularly insidious kind of bug, because in many programs,\nskips from input streams usually do skip the full amount\nof data requested, causing the program to fail only sporadically. With\nBuffered streams, however, skip() will only skip data\nin the buffer, and will routinely fail to skip the requested number of\nbytes.\n\nSigned-off-by: Matthias Sohn \n---\n .../jgit/transport/BundleFetchConnection.java | 16\n++++++++++++++--\n 1 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git\na/org.spearce.jgit/src/org/spearce/jgit/transport/BundleFetchConnection.\njava\nb/org.spearce.jgit/src/org/spearce/jgit/transport/BundleFetchConnection.\njava\nindex 40bf7db..642c984 100644\n---\na/org.spearce.jgit/src/org/spearce/jgit/transport/BundleFetchConnection.\njava\n+++\nb/org.spearce.jgit/src/org/spearce/jgit/transport/BundleFetchConnection.\njava\n@@ -39,6 +39,7 @@\n package org.spearce.jgit.transport;\n \n import java.io.BufferedInputStream;\n+import java.io.EOFException;\n import java.io.IOException;\n import java.io.InputStream;\n import java.util.ArrayList;\n@@ -139,12 +140,23 @@ private String readLine(final byte[] hdrbuf)\nthrows IOException {\n \t\twhile (lf < cnt && hdrbuf[lf] != '\\n')\n \t\t\tlf++;\n \t\tbin.reset();\n-\t\tbin.skip(lf);\n+\t\tskipFully(bin, lf);\n \t\tif (lf < cnt && hdrbuf[lf] == '\\n')\n-\t\t\tbin.skip(1);\n+\t\t\tskipFully(bin, 1);\n \t\treturn RawParseUtils.decode(Constants.CHARSET, hdrbuf,\n0, lf);\n \t}\n \n+\t// skip given number of bytes on InputStream respecting return\nvalue of InputStream.skip()\n+\tstatic private void skipFully(InputStream in, long nBytes)\nthrows IOException {\n+\t\tlong remaining = nBytes;\n+\t\twhile (remaining != 0) {\n+\t\t\tlong skipped = in.skip(remaining);\n+\t\t\tif (skipped == 0) // EOF\n+\t\t\t\tthrow new EOFException();\n+\t\t\tremaining -= skipped;\n+\t\t}\n+\t}\n+\t\n \tpublic boolean didFetchTestConnectivity() {\n \t\treturn false;\n \t}\n-- \n1.6.2.2.1669.g7eaf8\n"},{"id":"112479","messageId":"20090427231550.GK23604@spearce.org","threadId":"19097","inReplyTo":"366BBB1215D0AB4B8A153AF047A2878002FCE7E9@dewdfe18.wdf.sap.corp","subject":"Re: [PATCH JGIT] Method invokes inefficient Number constructor; use static valueOf instead","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-04-27T23:15:50Z","receivedAt":"2009-04-27T23:15:50Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"\"Sohn, Matthias\" wrote:\n> Using new Integer(int) is guaranteed to always result in a new object\n> whereas Integer.valueOf(int) allows caching of values\n> to be done by the compiler, class library, or JVM. Using of cached\n> values avoids object allocation and the code will be faster.\n\nNAK.\n\nThis thread came up about 5 weeks ago. Please see my reply here:\n\n http://thread.gmane.org/gmane.comp.version-control.git/113738/focus=113785\n\n \n-- \nShawn.\n"},{"id":"112480","messageId":"20090427231701.GL23604@spearce.org","threadId":"19097","inReplyTo":"366BBB1215D0AB4B8A153AF047A2878002FCE7E8@dewdfe18.wdf.sap.corp","subject":"Re: [PATCH JGIT] Method invokes inefficient new String(String) constructor","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-04-27T23:17:01Z","receivedAt":"2009-04-27T23:17:01Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"\"Sohn, Matthias\" wrote:\n> Using the java.lang.String(String) constructor wastes memory because the\n> object so constructed will be functionally\n> indistinguishable from the String passed as a parameter. Just use the\n> argument String directly.\n\nNAK.\n\nLike the Long.valueOf() case this came up before:\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/113739/focus=113787\n \n-- \nShawn.\n"},{"id":"112481","messageId":"20090427231757.GM23604@spearce.org","threadId":"19097","inReplyTo":"366BBB1215D0AB4B8A153AF047A2878002FCE7E7@dewdfe18.wdf.sap.corp","subject":"Re: [PATCH JGIT] Computation of average could overflow","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-04-27T23:17:57Z","receivedAt":"2009-04-27T23:17:57Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"\"Sohn, Matthias\" wrote:\n> The code computes the average of two integers using either division or\n> signed right shift, and then uses the result as the\n> index of an array. If the values being averaged are very large, this can\n> overflow (resulting in the computation of a negative\n> average). Assuming that the result is intended to be nonnegative, you\n> can use an unsigned right shift instead. In other\n> words, rather that using (low+high)/2, use (low+high) >>> 1\n\nThanks, applied. But your patch was line wrapped. I had to unwrap\nit by hand. Please try to configure your MUA not to line wrap\npatches when it sends them. :-|\n \n> .../src/org/spearce/jgit/dircache/DirCache.java | 2 +-\n> .../src/org/spearce/jgit/lib/Tree.java | 2 +-\n> 2 files changed, 2 insertions(+), 2 deletions(-)\n\n-- \nShawn.\n"},{"id":"112483","messageId":"20090427232112.GN23604@spearce.org","threadId":"19097","inReplyTo":"366BBB1215D0AB4B8A153AF047A2878002FCE7EA@dewdfe18.wdf.sap.corp","subject":"Re: [PATCH JGIT] Method ignores results of InputStream.skip()","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-04-27T23:21:12Z","receivedAt":"2009-04-27T23:21:12Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"\"Sohn, Matthias\" wrote:\n> This method ignores the return value of java.io.InputStream.skip()\n\nDoh. In theory the skip should always succeed because the buffer\nheld the entire block we want to skip over due to the mark/reset\nusage around this region. But I agree, a skipFully() pattern is\nbetter here.\n \n> @@ -139,12 +140,23 @@ private String readLine(final byte[] hdrbuf)\n> throws IOException {\n> \t\twhile (lf < cnt && hdrbuf[lf] != '\\n')\n> \t\t\tlf++;\n> \t\tbin.reset();\n> -\t\tbin.skip(lf);\n> +\t\tskipFully(bin, lf);\n> \t\tif (lf < cnt && hdrbuf[lf] == '\\n')\n> -\t\t\tbin.skip(1);\n> +\t\t\tskipFully(bin, 1);\n> \t\treturn RawParseUtils.decode(Constants.CHARSET, hdrbuf,\n> 0, lf);\n> \t}\n> \n> +\t// skip given number of bytes on InputStream respecting return\n> value of InputStream.skip()\n> +\tstatic private void skipFully(InputStream in, long nBytes)\n\nWe already have this method; see NB.skipFully().\n\nNB also has readFully() and a few other useful functions for\ndealing with common IO related patterns.\n\nPlease respin by calling NB.skipFully above rather than creating\na new package level method, and fix the line wrapping issue so we\ncan more easily apply it. :-)\n\n-- \nShawn.\n"},{"id":"112568","messageId":"366BBB1215D0AB4B8A153AF047A287800302A241@dewdfe18.wdf.sap.corp","threadId":"19097","inReplyTo":"366BBB1215D0AB4B8A153AF047A2878002FCE7EA@dewdfe18.wdf.sap.corp","subject":"[PATCH JGIT] Method ignores results of InputStream.skip()","fromName":"Sohn, Matthias","fromEmail":"matthias.sohn@sap.com","sentAt":"2009-04-28T22:26:15Z","receivedAt":"2009-04-28T22:26:15Z","isPatch":true,"sender":{"key":"matthias.sohn@sap.com","avatar":"https://gravatar.com/avatar/88bbb2733bcb977ec2d2cc1916ba8a70d6d41432c146bb8dab4f7f802261194e?d=mp&s=160"},"body":"This method ignores the return value of java.io.InputStream.skip() which can skip multiple bytes. If the return value is not checked, the caller will not be able to correctly handle the case where fewer bytes were skipped than the caller requested. This is a particularly insidious kind of bug, because in many programs, skips from input streams usually do skip the full amount of data requested, causing the program to fail only sporadically. With buffered streams, however, skip() will only skip data in the buffer, and will routinely fail to skip the requested number of bytes.\n\nSigned-off-by: Matthias Sohn \n---\nhopefully this time Exchange server doesn't mangle the patch\n\n .../jgit/transport/BundleFetchConnection.java | 5 +++--\n 1 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/BundleFetchConnection.java b/org.spearce.jgit/src/org/spearce/jgit/transport/BundleFetchConnection.java\nindex 40bf7db..2e2977e 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/transport/BundleFetchConnection.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/BundleFetchConnection.java\n@@ -60,6 +60,7 @@\n import org.spearce.jgit.revwalk.RevFlag;\n import org.spearce.jgit.revwalk.RevObject;\n import org.spearce.jgit.revwalk.RevWalk;\n+import org.spearce.jgit.util.NB;\n import org.spearce.jgit.util.RawParseUtils;\n \n /**\n@@ -139,9 +140,9 @@ private String readLine(final byte[] hdrbuf) throws IOException {\n \t\twhile (lf < cnt && hdrbuf[lf] != '\\n')\n \t\t\tlf++;\n \t\tbin.reset();\n-\t\tbin.skip(lf);\n+\t\tNB.skipFully(bin, lf);\n \t\tif (lf < cnt && hdrbuf[lf] == '\\n')\n-\t\t\tbin.skip(1);\n+\t\t\tNB.skipFully(bin, 1);\n \t\treturn RawParseUtils.decode(Constants.CHARSET, hdrbuf, 0, lf);\n \t}\n \n-- \n1.6.2.2.1669.g7eaf8\n"}]}