threads / patch / 18398

patchMethod invokes inefficient Number constructor; use static valueOf instead

Subject: [PATCH JGIT] Method invokes inefficient Number constructor; use static valueOf instead

## tl;dr

4 messages between Mar 19, 2009 and Mar 23, 2009. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Yann Simon· Mar 19, 2009, 09:14 UTC · lore
>From FindBugs:

Using new Integer(int) is guaranteed to always result in a new object whereas Integer.valueOf(int) allows caching of values to be done by the compiler, class library, or JVM. Using of cached values avoids object allocation and the code will be faster. Values between -128 and 127 are guaranteed to have corresponding cached instances and using valueOf is approximately 3.5 times faster than using constructor. For values outside the constant range the performance of both styles is the same.

Signed-off-by: Yann Simon <yann.simon.fr@gmail.com>
---
 .../jgit/errors/NoClosingBracketException.java     |    4 ++--
 .../src/org/spearce/jgit/transport/IndexPack.java  |    6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)
Show changes to 2 files +5 −5

org.spearce.jgit/src/org/spearce/jgit/errors/NoClosingBracketException.java, org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java

diff --git a/org.spearce.jgit/src/org/spearce/jgit/errors/NoClosingBracketException.java b/org.spearce.jgit/src/org/spearce/jgit/errors/NoClosingBracketException.java
index 8fe9ab1..b325b45 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/errors/NoClosingBracketException.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/errors/NoClosingBracketException.java
@@ -64,7 +64,7 @@ super(createMessage(indexOfOpeningBracket, openingBracket,
 	private static String createMessage(final int indexOfOpeningBracket,
 			final String openingBracket, final String closingBracket) {
 		return String.format("No closing %s found for %s at index %s.",
-				closingBracket, openingBracket, new Integer(
-						indexOfOpeningBracket));
+				closingBracket, openingBracket,
+				Integer.valueOf(indexOfOpeningBracket));
 	}
 }
diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java b/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java
index e0e4855..04ef59d 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java
@@ -383,7 +383,7 @@ private void resolveDeltas(final ProgressMonitor progress)
 	private void resolveDeltas(final PackedObjectInfo oe) throws IOException {
 		final int oldCRC = oe.getCRC();
 		if (baseById.containsKey(oe)
-				|| baseByPos.containsKey(new Long(oe.getOffset())))
+				|| baseByPos.containsKey(Long.valueOf(oe.getOffset())))
 			resolveDeltas(oe.getOffset(), oldCRC, Constants.OBJ_BAD, null, oe);
 	}
 
@@ -448,7 +448,7 @@ private void resolveDeltas(final long pos, final int oldCRC, int type,
 	private void resolveChildDeltas(final long pos, int type, byte[] data,
 			PackedObjectInfo oe) throws IOException {
 		final ArrayList<UnresolvedDelta> a = baseById.remove(oe);
-		final ArrayList<UnresolvedDelta> b = baseByPos.remove(new Long(pos));
+		final ArrayList<UnresolvedDelta> b = baseByPos.remove(Long.valueOf(pos));
 		int ai = 0, bi = 0;
 		if (a != null && b != null) {
 			while (ai < a.size() && bi < b.size()) {
@@ -679,7 +679,7 @@ private void indexOneObject() throws IOException {
 				ofs <<= 7;
 				ofs += (c & 127);
 			}
-			final Long base = new Long(pos - ofs);
+			final Long base = Long.valueOf(pos - ofs);
 			ArrayList<UnresolvedDelta> r = baseByPos.get(base);
 			if (r == null) {
 				r = new ArrayList<UnresolvedDelta>(8);

-- 
1.6.1.2
Shawn O. Pearce· Mar 19, 2009, 15:49 UTC · re: Yann Simon · lore

Re: [PATCH JGIT] Method invokes inefficient Number constructor; use static valueOf instead

Yann Simon <yann.simon.fr@gmail.com> wrote:
Show 5 quoted lines
> From FindBugs:
> Using new Integer(int) is guaranteed to always result in a new object
> whereas Integer.valueOf(int) allows caching of values to be done by the
> compiler, class library, or JVM. Using of cached values avoids object
> allocation and the code will be faster.
Ah, yes.
> Values between -128 and 127 are guaranteed to have corresponding cached
> instances [...]
...
Show 10 quoted lines
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java b/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java
> index e0e4855..04ef59d 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/transport/IndexPack.java
> @@ -383,7 +383,7 @@ private void resolveDeltas(final ProgressMonitor progress)
>  	private void resolveDeltas(final PackedObjectInfo oe) throws IOException {
>  		final int oldCRC = oe.getCRC();
>  		if (baseById.containsKey(oe)
> -				|| baseByPos.containsKey(new Long(oe.getOffset())))
> +				|| baseByPos.containsKey(Long.valueOf(oe.getOffset())))
Why I box with new Long() over Long.valueOf():
 The standard only requires -128..127 to be cached.  A JRE can
 cache value outside of this range if it chooses, but long has a
 huge range, its unlikely to cache much beyond this required region.
 Most pack files are in the 10 MB...100+ MB range.  Most objects
 take more than 100 bytes in a pack, even compressed delta encoded.
 Thus any object after the first is going to have its offset outside
 of the cached range.
 In other words, why waste the CPU cycles on the "cached range
 bounds check" when I'm always going to fail and allocate.  I might
 as well just allocate
 These sections of code are rather performance critical for the
 indexing phase of a pack receive, on either side of a connection.
 I need to shave even more instructions out of the critical paths,
 as its not fast enough as-is.  Using new Long() is quicker than
 using Long.valueOf(), so new Long() it is.
Show 16 quoted lines
> @@ -448,7 +448,7 @@ private void resolveDeltas(final long pos, final int oldCRC, int type,
>  	private void resolveChildDeltas(final long pos, int type, byte[] data,
>  			PackedObjectInfo oe) throws IOException {
>  		final ArrayList<UnresolvedDelta> a = baseById.remove(oe);
> -		final ArrayList<UnresolvedDelta> b = baseByPos.remove(new Long(pos));
> +		final ArrayList<UnresolvedDelta> b = baseByPos.remove(Long.valueOf(pos));
>  		int ai = 0, bi = 0;
>  		if (a != null && b != null) {
>  			while (ai < a.size() && bi < b.size()) {
> @@ -679,7 +679,7 @@ private void indexOneObject() throws IOException {
>  				ofs <<= 7;
>  				ofs += (c & 127);
>  			}
> -			final Long base = new Long(pos - ofs);
> +			final Long base = Long.valueOf(pos - ofs);
>  			ArrayList<UnresolvedDelta> r = baseByPos.get(base);

See above for why these two hunks allocate rather than use valueOf() or even the implicit compiler produced auto-boxing (which uses valueOf).

But your first hunk (which I snipped) is fine to apply.
-- 
Shawn.
Yann Simon· Mar 23, 2009, 10:36 UTC · re: Shawn O. Pearce · lore

Re: [PATCH JGIT] Method invokes inefficient Number constructor; use static valueOf instead

2009/3/19 Shawn O. Pearce <spearce@spearce.org>:
Show 20 quoted lines
> Why I box with new Long() over Long.valueOf():
>
>  The standard only requires -128..127 to be cached.  A JRE can
>  cache value outside of this range if it chooses, but long has a
>  huge range, its unlikely to cache much beyond this required region.
>
>  Most pack files are in the 10 MB...100+ MB range.  Most objects
>  take more than 100 bytes in a pack, even compressed delta encoded.
>  Thus any object after the first is going to have its offset outside
>  of the cached range.
>
>  In other words, why waste the CPU cycles on the "cached range
>  bounds check" when I'm always going to fail and allocate.  I might
>  as well just allocate
>
>  These sections of code are rather performance critical for the
>  indexing phase of a pack receive, on either side of a connection.
>  I need to shave even more instructions out of the critical paths,
>  as its not fast enough as-is.  Using new Long() is quicker than
>  using Long.valueOf(), so new Long() it is.

It makes sense. Thank you for the explanation.

Yann

← back to recent threads