Re: Need reviewer for forward port of 6815768 (File.getXXXSpace) and 6815768 (String.hashCode)

2010-03-04 Thread Marek Kozieł
2010/2/28 Ulf Zibis ulf.zi...@gmx.de: Am 25.02.2010 23:07, schrieb Alan Bateman: Kelly O'Hair wrote: Yup.  My eyes must be tired, I didn't see that. :^( Too many repositories in the air at the same time. The webrev has been refreshed. Thanks Ulf. Another thought: In the constructors

Re: Tune String's hashCode() + equals() [was: Need reviewer for forward port of 6815768 (File.getXXXSpace) and 6815768 (String.hashCode)]

2010-03-04 Thread Ulf Zibis
Am 04.03.2010 19:33, schrieb Marek Kozieł: Hello, I would suggest: public int hashCode() { int h = hash; if (h == 0) { h = 0; char[] val = value; for (int i = offset, limit = count + i; i != limit; ) h = 31 * h + val[i++];

Re: Need reviewer for forward port of 6815768 (File.getXXXSpace) and 6815768 (String.hashCode)

2010-03-04 Thread Ulf Zibis
Am 04.03.2010 03:08, schrieb Jason Mehrens: String.hash should only have two known states, zero and the actual computed hash code. http://bugs.sun.com/view_bug.do?bug_id=6611830 I far theory yes. But have you read the evaluation ? This bug pattern is endemic in the JDK sources. -Ulf

RE: Need reviewer for forward port of 6815768 (File.getXXXSpace) and 6815768 (String.hashCode)

2010-03-03 Thread Jason Mehrens
String.hash should only have two known states, zero and the actual computed hash code. http://bugs.sun.com/view_bug.do?bug_id=6611830 Jason Date: Sun, 28 Feb 2010 17:09:15 +0100 From: ulf.zi...@gmx.de To: alan.bate...@sun.com Subject: Re: Need reviewer for forward port of 6815768

Re: Need reviewer for forward port of 6815768 (File.getXXXSpace) and 6815768 (String.hashCode)

2010-02-28 Thread Ulf Zibis
Am 25.02.2010 23:07, schrieb Alan Bateman: Kelly O'Hair wrote: Yup. My eyes must be tired, I didn't see that. :^( Too many repositories in the air at the same time. The webrev has been refreshed. Thanks Ulf. Another thought: In the constructors of String we could initialize hash =

Re: Need reviewer for forward port of 6815768 (File.getXXXSpace) and 6815768 (String.hashCode)

2010-02-27 Thread Ulf Zibis
Dmitry, thanks for the interesting results. Your results are not as promising as mine. I guess, referring to the HotSpot's dissassembler dump would give more hints. Additionally different string lengths, repeat factors, -Xbatch, and GC settings would help. Good trick to use substring(1) for

Re: Need reviewer for forward port of 6815768 (File.getXXXSpace) and 6815768 (String.hashCode)

2010-02-27 Thread Ulf Zibis
Hm, hashCode7() has exactly the same program logic than hashCode1(), but by different syntax. hashCode8() should be worse than hashCode7(), because it loads the count variable twice. If you compare the byte code by javah or jclasslib bytecode viewer (very nice tool), you will see that, am I

RE: Need reviewer for forward port of 6815768 (File.getXXXSpace) and 6815768 (String.hashCode)

2010-02-27 Thread Jason Mehrens
Javap shows different byte code for hashCode7 vs hashCode1 when I compiled and compared them. I'll have not tried jclasslib. I agree with you on version 7 should be faster than version 6. However, in version 6 count is only loaded twice when running the path rarely chosen. I think that

Re: Need reviewer for forward port of 6815768 (File.getXXXSpace) and 6815768 (String.hashCode)

2010-02-27 Thread Ulf Zibis
Am 26.02.2010 20:32, schrieb Alan Bateman: Ulf Zibis wrote: For these other suggestions it would be great to create micro-benchmarks and try our your changes. For now though, as I said, I'm just fixing the method to avoid the store for the empty string case. I've added benchmark for same

Re: Need reviewer for forward port of 6815768 (File.getXXXSpace) and 6815768 (String.hashCode)

2010-02-26 Thread Dmitry Nadezhin
I found two alternatives in the link http://mail.openjdk.java.net/pipermail/coin-dev/2009-December/002618.html The first alternative int equalByHashThreshold = 2; public boolean equals(Object anObject) { if (this == anObject) { return true; } if (anObject instanceof String) {

Re: Need reviewer for forward port of 6815768 (File.getXXXSpace) and 6815768 (String.hashCode)

2010-02-26 Thread Alan Bateman
Jason Mehrens wrote: Alan, Shouldn't the loading of 'this.count' into 'len' be only performed if 'h' is zero? Otherwise, when hash is not zero we perform a little unnecessary work every time hashCode is called. Jason That's a good suggestion although it might be hard to measure any

Re: Need reviewer for forward port of 6815768 (File.getXXXSpace) and 6815768 (String.hashCode)

2010-02-26 Thread Alan Bateman
Ulf Zibis wrote: Have you seen, that there are some chances to make String#equals little faster too: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6912520 -Ulf Yes, I have seen it, but for now anyway, I'm not actually doing any work on String but rather just bringing forward fixes that

Re: Need reviewer for forward port of 6815768 (File.getXXXSpace) and 6815768 (String.hashCode)

2010-02-26 Thread Ulf Zibis
Am 26.02.2010 13:58, schrieb Alan Bateman: Jason Mehrens wrote: Alan, Shouldn't the loading of 'this.count' into 'len' be only performed if 'h' is zero? Otherwise, when hash is not zero we perform a little unnecessary work every time hashCode is called. Jason That's a good suggestion

Re: Need reviewer for forward port of 6815768 (File.getXXXSpace) and 6815768 (String.hashCode)

2010-02-26 Thread Dmitry Nadezhin
On Fri, Feb 26, 2010 at 6:19 PM, Ulf Zibis ulf.zi...@gmx.de wrote: Am 26.02.2010 12:52, schrieb Dmitry Nadezhin: I found two alternatives in the link http://mail.openjdk.java.net/pipermail/coin-dev/2009-December/002618.html The first alternative int equalByHashThreshold = 2; public

Re: Need reviewer for forward port of 6815768 (File.getXXXSpace) and 6815768 (String.hashCode)

2010-02-26 Thread Ulf Zibis
Am 26.02.2010 16:25, schrieb Ulf Zibis: So optimum could be: I have one more ... Additionally we can save double incrementing of int i and off: public int hashCode() { int h = hash; if (h == 0) { int len = count; if (len 0) { char[] val = value; // for (int i =

Re: Need reviewer for forward port of 6815768 (File.getXXXSpace) and 6815768 (String.hashCode)

2010-02-26 Thread Ulf Zibis
Additionally I think: char[] val = value; looks better then: char val[] = value; -Ulf Am 25.02.2010 20:08, schrieb Alan Bateman: Kelly O'Hair wrote: Looks fine to me. -kto Thanks Kelly and also to Joe for pointing out that I botched the link to the second one - here is the

Re: Need reviewer for forward port of 6815768 (File.getXXXSpace) and 6815768 (String.hashCode)

2010-02-26 Thread Ulf Zibis
Am 26.02.2010 17:10, schrieb Ulf Zibis: Additionally we can save double incrementing of int i and off: public int hashCode() { int h = hash; if (h == 0) { int len = count; if (len 0) { char[] val = value; // for (int i = offset, len += i; i len; ) // would be nice

Re: Need reviewer for forward port of 6815768 (File.getXXXSpace) and 6815768 (String.hashCode)

2010-02-26 Thread Alan Bateman
Ulf Zibis wrote: Am 26.02.2010 16:25, schrieb Ulf Zibis: So optimum could be: I have one more ... Additionally we can save double incrementing of int i and off: public int hashCode() { int h = hash; if (h == 0) { int len = count; if (len 0) { char[] val = value; //

Re: Need reviewer for forward port of 6815768 (File.getXXXSpace) and 6815768 (String.hashCode)

2010-02-26 Thread Ulf Zibis
Am 26.02.2010 20:32, schrieb Alan Bateman: Ulf Zibis wrote: For these other suggestions it would be great to create micro-benchmarks and try our your changes. For now though, as I said, I'm just fixing the method to avoid the store for the empty string case. I've run a benchmark for your

Re: Need reviewer for forward port of 6815768 (File.getXXXSpace) and 6815768 (String.hashCode)

2010-02-25 Thread Kelly O'Hair
DOH! Yeah that one looks better... ;^) Thought they looked the same before. -kto On 2/25/10 11:08 AM, Alan Bateman wrote: Kelly O'Hair wrote: Looks fine to me. -kto Thanks Kelly and also to Joe for pointing out that I botched the link to the second one - here is the right webrev:

Re: Need reviewer for forward port of 6815768 (File.getXXXSpace) and 6815768 (String.hashCode)

2010-02-25 Thread Alan Bateman
Kelly O'Hair wrote: Looks fine to me. -kto Thanks Kelly and also to Joe for pointing out that I botched the link to the second one - here is the right webrev: http://cr.openjdk.java.net/~alanb/6921374/webrev/ -Alan

Need reviewer for forward port of 6815768 (File.getXXXSpace) and 6815768 (String.hashCode)

2010-02-25 Thread Alan Bateman
There are two fixes in Sun's proprietary jdk6 updates that should also be fixed in OpenJDK. The first one is that the File.getXXXSpace methods use statvfs instead of statvfs64 and so fail on 32-bit Solaris and Linux with very large file systems. The webrev is here:

Re: Need reviewer for forward port of 6815768 (File.getXXXSpace) and 6815768 (String.hashCode)

2010-02-25 Thread Andrew John Hughes
On 25 February 2010 18:19, Kelly O'Hair kelly.oh...@sun.com wrote: Looks fine to me. -kto On 2/25/10 9:51 AM, Alan Bateman wrote: There are two fixes in Sun's proprietary jdk6 updates that should also be fixed in OpenJDK. The first one is that the File.getXXXSpace methods use statvfs

Re: Need reviewer for forward port of 6815768 (File.getXXXSpace) and 6815768 (String.hashCode)

2010-02-25 Thread Ulf Zibis
Why don't you use the faster local copy of count for the junction like: if (h == 0 len 0) { ? -Ulf Am 25.02.2010 20:08, schrieb Alan Bateman: Kelly O'Hair wrote: Looks fine to me. -kto Thanks Kelly and also to Joe for pointing out that I botched the link to the second one - here is

Re: Need reviewer for forward port of 6815768 (File.getXXXSpace) and 6815768 (String.hashCode)

2010-02-25 Thread Kelly O'Hair
Yup. My eyes must be tired, I didn't see that. :^( -kto On 2/25/10 12:17 PM, Ulf Zibis wrote: Why don't you use the faster local copy of count for the junction like: if (h == 0 len 0) { ? -Ulf Am 25.02.2010 20:08, schrieb Alan Bateman: Kelly O'Hair wrote: Looks fine to me. -kto

RE: Need reviewer for forward port of 6815768 (File.getXXXSpace) and 6815768 (String.hashCode)

2010-02-25 Thread Jason Mehrens
Alan, Shouldn't the loading of 'this.count' into 'len' be only performed if 'h' is zero? Otherwise, when hash is not zero we perform a little unnecessary work every time hashCode is called. Jason Date: Thu, 25 Feb 2010 21:17:37 +0100 From: ulf.zi...@gmx.de To: alan.bate...@sun.com

Re: Need reviewer for forward port of 6815768 (File.getXXXSpace) and 6815768 (String.hashCode)

2010-02-25 Thread Joseph D. Darcy
Andrew John Hughes wrote: On 25 February 2010 18:19, Kelly O'Hair kelly.oh...@sun.com wrote: Looks fine to me. -kto On 2/25/10 9:51 AM, Alan Bateman wrote: There are two fixes in Sun's proprietary jdk6 updates that should also be fixed in OpenJDK. The first one is that the