* [PATCH JGIT] Method invokes inefficient Number constructor; use static valueOf instead
@ 2009-03-19 9:14 Yann Simon
2009-03-19 9:45 ` Ferry Huberts (Pelagic)
2009-03-19 15:49 ` Shawn O. Pearce
0 siblings, 2 replies; 6+ messages in thread
From: Yann Simon @ 2009-03-19 9:14 UTC (permalink / raw)
To: Robin Rosenberg, Shawn O. Pearce; +Cc: git
>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(-)
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH JGIT] Method invokes inefficient Number constructor; use static valueOf instead
2009-03-19 9:14 [PATCH JGIT] Method invokes inefficient Number constructor; use static valueOf instead Yann Simon
@ 2009-03-19 9:45 ` Ferry Huberts (Pelagic)
2009-03-19 15:49 ` Shawn O. Pearce
1 sibling, 0 replies; 6+ messages in thread
From: Ferry Huberts (Pelagic) @ 2009-03-19 9:45 UTC (permalink / raw)
To: Yann Simon; +Cc: Robin Rosenberg, Shawn O. Pearce, git
Yann Simon wrote:
> From FindBugs:
+1
also, junit would be another +1 AFAIC :-)
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH JGIT] Method invokes inefficient Number constructor; use static valueOf instead
2009-03-19 9:14 [PATCH JGIT] Method invokes inefficient Number constructor; use static valueOf instead Yann Simon
2009-03-19 9:45 ` Ferry Huberts (Pelagic)
@ 2009-03-19 15:49 ` Shawn O. Pearce
2009-03-23 10:36 ` Yann Simon
1 sibling, 1 reply; 6+ messages in thread
From: Shawn O. Pearce @ 2009-03-19 15:49 UTC (permalink / raw)
To: Yann Simon; +Cc: Robin Rosenberg, git
Yann Simon <yann.simon.fr@gmail.com> wrote:
> 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 [...]
...
> 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.
> @@ -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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH JGIT] Method invokes inefficient Number constructor; use static valueOf instead
2009-03-19 15:49 ` Shawn O. Pearce
@ 2009-03-23 10:36 ` Yann Simon
0 siblings, 0 replies; 6+ messages in thread
From: Yann Simon @ 2009-03-23 10:36 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Robin Rosenberg, git
2009/3/19 Shawn O. Pearce <spearce@spearce.org>:
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH JGIT] Method invokes inefficient Number constructor; use static valueOf instead
2009-04-27 23:08 ` [PATCH JGIT] Method invokes inefficient Number constructor; use static valueOf instead Sohn, Matthias
@ 2009-04-27 23:15 ` Shawn O. Pearce
0 siblings, 0 replies; 6+ messages in thread
From: Shawn O. Pearce @ 2009-04-27 23:15 UTC (permalink / raw)
To: Sohn, Matthias; +Cc: Robin Rosenberg, git
"Sohn, Matthias" <matthias.sohn@sap.com> wrote:
> 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.
NAK.
This thread came up about 5 weeks ago. Please see my reply here:
http://thread.gmane.org/gmane.comp.version-control.git/113738/focus=113785
--
Shawn.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH JGIT] Method invokes inefficient Number constructor; use static valueOf instead
2009-04-27 23:05 ` [PATCH JGIT] Method invokes inefficient new String(String) constructor Sohn, Matthias
@ 2009-04-27 23:08 ` Sohn, Matthias
2009-04-27 23:15 ` Shawn O. Pearce
0 siblings, 1 reply; 6+ messages in thread
From: Sohn, Matthias @ 2009-04-27 23:08 UTC (permalink / raw)
To: Shawn O. Pearce, Robin Rosenberg; +Cc: git
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.
Unless the class must be compatible with JVMs predating Java 1.5, use
either autoboxing or the valueOf() method when
creating instances of Long, Integer, Short, Character, and Byte.
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
---
.../src/org/spearce/jgit/transport/IndexPack.java | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
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.2.2.1669.g7eaf8
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-04-27 23:16 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-03-19 9:14 [PATCH JGIT] Method invokes inefficient Number constructor; use static valueOf instead Yann Simon
2009-03-19 9:45 ` Ferry Huberts (Pelagic)
2009-03-19 15:49 ` Shawn O. Pearce
2009-03-23 10:36 ` Yann Simon
2009-04-27 23:02 [PATCH JGIT] Computation of average could overflow Sohn, Matthias
2009-04-27 23:05 ` [PATCH JGIT] Method invokes inefficient new String(String) constructor Sohn, Matthias
2009-04-27 23:08 ` [PATCH JGIT] Method invokes inefficient Number constructor; use static valueOf instead Sohn, Matthias
2009-04-27 23:15 ` Shawn O. Pearce
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.