{"thread":{"id":"18398","subject":"[PATCH JGIT] Method invokes inefficient Number constructor; use static valueOf instead","startedAt":"2009-03-19T09:14:59Z","lastAt":"2009-03-23T10:36:50Z","messageCount":4,"participants":["Yann Simon","Ferry Huberts (Pelagic)","Shawn O. Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"108494","messageId":"49C20D13.2050908@gmail.com","threadId":"18398","inReplyTo":null,"subject":"[PATCH JGIT] Method invokes inefficient Number constructor; use static valueOf instead","fromName":"Yann Simon","fromEmail":"yann.simon.fr@gmail.com","sentAt":"2009-03-19T09:14:59Z","receivedAt":"2009-03-19T09:14:59Z","isPatch":true,"sender":{"key":"yann.simon.fr@gmail.com","avatar":"https://gravatar.com/avatar/2d926895d27ac988c5c8e591887e5a6a4c7036390403c74fce92519119b887a0?d=mp&s=160"},"body":">From FindBugs:\nUsing new Integer(int) is guaranteed to always result in a new object\nwhereas Integer.valueOf(int) allows caching of values to be done by the\ncompiler, class library, or JVM. Using of cached values avoids object\nallocation and the code will be faster.\nValues between -128 and 127 are guaranteed to have corresponding cached\ninstances and using valueOf is approximately 3.5 times faster than using\nconstructor. For values outside the constant range the performance of\nboth styles is the same.\n\nSigned-off-by: Yann Simon <yann.simon.fr@gmail.com>\n---\n .../jgit/errors/NoClosingBracketException.java     |    4 ++--\n .../src/org/spearce/jgit/transport/IndexPack.java  |    6 +++---\n 2 files changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/errors/NoClosingBracketException.java b/org.spearce.jgit/src/org/spearce/jgit/errors/NoClosingBracketException.java\nindex 8fe9ab1..b325b45 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/errors/NoClosingBracketException.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/errors/NoClosingBracketException.java\n@@ -64,7 +64,7 @@ super(createMessage(indexOfOpeningBracket, openingBracket,\n \tprivate static String createMessage(final int indexOfOpeningBracket,\n \t\t\tfinal String openingBracket, final String closingBracket) {\n \t\treturn String.format(\"No closing %s found for %s at index %s.\",\n-\t\t\t\tclosingBracket, openingBracket, new Integer(\n-\t\t\t\t\t\tindexOfOpeningBracket));\n+\t\t\t\tclosingBracket, openingBracket,\n+\t\t\t\tInteger.valueOf(indexOfOpeningBracket));\n \t}\n }\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java b/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 progress)\n \tprivate void resolveDeltas(final PackedObjectInfo oe) throws IOException {\n \t\tfinal int oldCRC = oe.getCRC();\n \t\tif (baseById.containsKey(oe)\n-\t\t\t\t|| baseByPos.containsKey(new Long(oe.getOffset())))\n+\t\t\t\t|| baseByPos.containsKey(Long.valueOf(oe.getOffset())))\n \t\t\tresolveDeltas(oe.getOffset(), oldCRC, Constants.OBJ_BAD, null, oe);\n \t}\n \n@@ -448,7 +448,7 @@ private void resolveDeltas(final long pos, final int oldCRC, int type,\n \tprivate void resolveChildDeltas(final long pos, int type, byte[] data,\n \t\t\tPackedObjectInfo oe) throws IOException {\n \t\tfinal ArrayList<UnresolvedDelta> a = baseById.remove(oe);\n-\t\tfinal ArrayList<UnresolvedDelta> b = baseByPos.remove(new Long(pos));\n+\t\tfinal ArrayList<UnresolvedDelta> b = baseByPos.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<UnresolvedDelta> r = baseByPos.get(base);\n \t\t\tif (r == null) {\n \t\t\t\tr = new ArrayList<UnresolvedDelta>(8);\n-- \n1.6.1.2\n"},{"id":"108498","messageId":"49C21441.80000@pelagic.nl","threadId":"18398","inReplyTo":"49C20D13.2050908@gmail.com","subject":"Re: [PATCH JGIT] Method invokes inefficient Number constructor; use static valueOf instead","fromName":"Ferry Huberts (Pelagic)","fromEmail":"ferry.huberts@pelagic.nl","sentAt":"2009-03-19T09:45:37Z","receivedAt":"2009-03-19T09:45:37Z","isPatch":true,"sender":{"key":"ferry.huberts@pelagic.nl","avatar":"https://gravatar.com/avatar/9f63c0289ad23cbdef0f7609a0af85ff0f4b3babfd066de9ff58f62d48cfd6f2?d=mp&s=160"},"body":"\n\nYann Simon wrote:\n> From FindBugs:\n\n+1\n\nalso, junit would be another +1 AFAIC :-)\n"},{"id":"108541","messageId":"20090319154958.GP23521@spearce.org","threadId":"18398","inReplyTo":"49C20D13.2050908@gmail.com","subject":"Re: [PATCH JGIT] Method invokes inefficient Number constructor; use static valueOf instead","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-03-19T15:49:58Z","receivedAt":"2009-03-19T15:49:58Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Yann Simon <yann.simon.fr@gmail.com> wrote:\n> From FindBugs:\n> Using new Integer(int) is guaranteed to always result in a new object\n> whereas Integer.valueOf(int) allows caching of values to be done by the\n> compiler, class library, or JVM. Using of cached values avoids object\n> allocation and the code will be faster.\n\nAh, yes.\n\n> Values between -128 and 127 are guaranteed to have corresponding cached\n> instances [...]\n...\n> diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java b/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java\n> index 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 progress)\n>  \tprivate void resolveDeltas(final PackedObjectInfo oe) throws IOException {\n>  \t\tfinal int oldCRC = oe.getCRC();\n>  \t\tif (baseById.containsKey(oe)\n> -\t\t\t\t|| baseByPos.containsKey(new Long(oe.getOffset())))\n> +\t\t\t\t|| baseByPos.containsKey(Long.valueOf(oe.getOffset())))\n\nWhy I box with new Long() over Long.valueOf():\n\n The standard only requires -128..127 to be cached.  A JRE can\n cache value outside of this range if it chooses, but long has a\n huge range, its unlikely to cache much beyond this required region.\n\n Most pack files are in the 10 MB...100+ MB range.  Most objects\n take more than 100 bytes in a pack, even compressed delta encoded.\n Thus any object after the first is going to have its offset outside\n of the cached range.\n\n In other words, why waste the CPU cycles on the \"cached range\n bounds check\" when I'm always going to fail and allocate.  I might\n as well just allocate\n\n These sections of code are rather performance critical for the\n indexing phase of a pack receive, on either side of a connection.\n I need to shave even more instructions out of the critical paths,\n as its not fast enough as-is.  Using new Long() is quicker than\n using Long.valueOf(), so new Long() it is.\n\n> @@ -448,7 +448,7 @@ private void resolveDeltas(final long pos, final int oldCRC, int type,\n>  \tprivate void resolveChildDeltas(final long pos, int type, byte[] data,\n>  \t\t\tPackedObjectInfo oe) throws IOException {\n>  \t\tfinal ArrayList<UnresolvedDelta> a = baseById.remove(oe);\n> -\t\tfinal ArrayList<UnresolvedDelta> b = baseByPos.remove(new Long(pos));\n> +\t\tfinal ArrayList<UnresolvedDelta> b = baseByPos.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<UnresolvedDelta> r = baseByPos.get(base);\n\nSee above for why these two hunks allocate rather than use valueOf()\nor even the implicit compiler produced auto-boxing (which uses\nvalueOf).\n\nBut your first hunk (which I snipped) is fine to apply.\n\n-- \nShawn.\n"},{"id":"109034","messageId":"551f769b0903230336v116ce40bn8ce6a1a28b997fd@mail.gmail.com","threadId":"18398","inReplyTo":"20090319154958.GP23521@spearce.org","subject":"Re: [PATCH JGIT] Method invokes inefficient Number constructor; use static valueOf instead","fromName":"Yann Simon","fromEmail":"yann.simon.fr@gmail.com","sentAt":"2009-03-23T10:36:50Z","receivedAt":"2009-03-23T10:36:50Z","isPatch":true,"sender":{"key":"yann.simon.fr@gmail.com","avatar":"https://gravatar.com/avatar/2d926895d27ac988c5c8e591887e5a6a4c7036390403c74fce92519119b887a0?d=mp&s=160"},"body":"2009/3/19 Shawn O. Pearce <spearce@spearce.org>:\n> Why I box with new Long() over Long.valueOf():\n>\n>  The standard only requires -128..127 to be cached.  A JRE can\n>  cache value outside of this range if it chooses, but long has a\n>  huge range, its unlikely to cache much beyond this required region.\n>\n>  Most pack files are in the 10 MB...100+ MB range.  Most objects\n>  take more than 100 bytes in a pack, even compressed delta encoded.\n>  Thus any object after the first is going to have its offset outside\n>  of the cached range.\n>\n>  In other words, why waste the CPU cycles on the \"cached range\n>  bounds check\" when I'm always going to fail and allocate.  I might\n>  as well just allocate\n>\n>  These sections of code are rather performance critical for the\n>  indexing phase of a pack receive, on either side of a connection.\n>  I need to shave even more instructions out of the critical paths,\n>  as its not fast enough as-is.  Using new Long() is quicker than\n>  using Long.valueOf(), so new Long() it is.\n\nIt makes sense.\nThank you for the explanation.\n\nYann\n"}]}