* [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
* [PATCH JGIT] Computation of average could overflow
@ 2009-04-27 23:02 Sohn, Matthias
2009-04-27 23:05 ` [PATCH JGIT] Method invokes inefficient new String(String) constructor Sohn, Matthias
0 siblings, 1 reply; 6+ messages in thread
From: Sohn, Matthias @ 2009-04-27 23:02 UTC (permalink / raw)
To: Shawn O. Pearce, Robin Rosenberg; +Cc: git
The code computes the average of two integers using either division or
signed right shift, and then uses the result as the
index of an array. If the values being averaged are very large, this can
overflow (resulting in the computation of a negative
average). Assuming that the result is intended to be nonnegative, you
can use an unsigned right shift instead. In other
words, rather that using (low+high)/2, use (low+high) >>> 1
This bug exists in many earlier implementations of binary search and
merge sort. Martin Buchholz found and fixed it
(http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6412541) in the JDK
libraries, and Joshua Bloch widely publicized
the bug pattern
(http://googleresearch.blogspot.com/2006/06/extra-extra-read-all-about-i
t-nearly.html).
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
---
.../src/org/spearce/jgit/dircache/DirCache.java | 2 +-
.../src/org/spearce/jgit/lib/Tree.java | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git
a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCache.java
b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCache.java
index 58da014..fa906fa 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCache.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCache.java
@@ -593,7 +593,7 @@ int findEntry(final byte[] p, final int pLen) {
int low = 0;
int high = entryCnt;
do {
- int mid = (low + high) >> 1;
+ int mid = (low + high) >>> 1;
final int cmp = cmp(p, pLen,
sortedEntries[mid]);
if (cmp < 0)
high = mid;
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/Tree.java
b/org.spearce.jgit/src/org/spearce/jgit/lib/Tree.java
index 0ecd04d..ff9e666 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/Tree.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/Tree.java
@@ -136,7 +136,7 @@ private static final int binarySearch(final
TreeEntry[] entries,
int high = entries.length;
int low = 0;
do {
- final int mid = (low + high) / 2;
+ final int mid = (low + high) >>> 1;
final int cmp =
compareNames(entries[mid].getNameUTF8(), nameUTF8,
nameStart, nameEnd,
TreeEntry.lastChar(entries[mid]), nameUTF8last);
if (cmp < 0)
--
1.6.2.2.1669.g7eaf8
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH JGIT] Method invokes inefficient new String(String) constructor
2009-04-27 23:02 [PATCH JGIT] Computation of average could overflow Sohn, Matthias
@ 2009-04-27 23:05 ` Sohn, Matthias
2009-04-27 23:08 ` [PATCH JGIT] Method invokes inefficient Number constructor; use static valueOf instead Sohn, Matthias
0 siblings, 1 reply; 6+ messages in thread
From: Sohn, Matthias @ 2009-04-27 23:05 UTC (permalink / raw)
To: Shawn O. Pearce, Robin Rosenberg; +Cc: git
Using the java.lang.String(String) constructor wastes memory because the
object so constructed will be functionally
indistinguishable from the String passed as a parameter. Just use the
argument String directly.
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>
---
.../src/org/spearce/jgit/lib/RefDatabase.java | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
b/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
index 87f26bf..49da538 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/lib/RefDatabase.java
@@ -447,7 +447,7 @@ private synchronized void refreshPackedRefs() {
final int sp = p.indexOf(' ');
final ObjectId id =
ObjectId.fromString(p.substring(0, sp));
- final String name = new
String(p.substring(sp + 1));
+ final String name =
p.substring(sp + 1);
last = new
Ref(Ref.Storage.PACKED, name, name, id);
newPackedRefs.put(last.getName(), last);
}
--
1.6.2.2.1669.g7eaf8
^ permalink raw reply related [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
-- strict thread matches above, loose matches on Subject: below --
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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).