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: http://cr.openjdk.java.n

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: http://cr.

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

2010-02-25 Thread Kelly O'Hair
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 instead of statvfs64 and so fail on 32-bit Solaris and Linux with very larg

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

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 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 instead

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

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 Th

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

2010-02-25 Thread Jason Mehrens
t; From: ulf.zi...@gmx.de > To: alan.bate...@sun.com > Subject: Re: Need reviewer for forward port of 6815768 (File.getXXXSpace) and > 6815768 (String.hashCode) > CC: core-libs-dev@openjdk.java.net; kelly.oh...@sun.com > > Why don't you use the faster local copy of count for

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

2010-02-25 Thread 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.

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 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

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

2010-02-26 Thread Ulf Zibis
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 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

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) { Stri

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 differ

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 we

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

2010-02-26 Thread Ulf Zibis
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 boolean equals(Object anObject) { if (this == anObject) { return tr

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 altho

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 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 boolean

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 = offset

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 syn

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; // f

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 "a

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. Here the benchmark for my different

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

2010-02-27 Thread Dmitry Nadezhin
Ulf, I ran Caliper benchmarks on different variants of hashCode(). http://code.google.com/p/caliper/ The results and the Caliper code are below: -Dima - -d32 -clien 0% Scenario{vm=java -d32 -client, benchmark=HashCodeEmpty} 8,85ns; σ=0,06ns @ 3 trials 8% Scenario{vm=java -d32 -client

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 Jason Mehrens
= value; for (int i = 0; i < len; i++) { h = 31*h + val[off++]; } hash = h; } return h; } Jason Date: Sat, 27 Feb 2010 17:00:14 +0300 Subject: Re: Need reviewer for forward port of 6815768 (File.getXXXSpace) and 68

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

2010-02-27 Thread Ulf Zibis
e: Sat, 27 Feb 2010 17:00:14 +0300 Subject: Re: Need reviewer for forward port of 6815768 (File.getXXXSpace) and 6815768 (String.hashCode) From: dmitry.nadez...@gmail.com To: core-libs-dev@openjdk.java.net Ulf, I ran Caliper benchmarks on different variants of hashCode(). http://code.google.com/p/

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

2010-02-27 Thread Ulf Zibis
Am 26.02.2010 16:27, schrieb Dmitry Nadezhin: On Fri, Feb 26, 2010 at 6:19 PM, Ulf Zibis > 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/002

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

2010-02-27 Thread Jason Mehrens
that makes version 6 a good candidate to benchmark. Jason Date: Sat, 27 Feb 2010 19:11:05 +0100 From: ulf.zi...@gmx.de To: jason_mehr...@hotmail.com CC: dmitry.nadez...@gmail.com; core-libs-dev@openjdk.java.net Subject: Re: Need reviewer for forward port of 6815768 (File.getXXXSpace

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" str

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 = Integ

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 for

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 : > 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 const

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

2010-03-04 Thread Ulf Zibis
uot; -Ulf 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 (File.getXXXSpace) and 6815768 (String.hashCode) > CC: core-libs-dev@openjdk.java.net; dmitry.nadez...@gmail.

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: Tune String's hashCode() + equals() [was: Need reviewer for forward port of 6815768 (File.getXXXSpace) and 6815768 (String.hashCode)]

2010-03-04 Thread Marek Kozieł
@Ulf Few explanations: 1. > Intersting alternative, but I'm afraid, this is against the spec. > Shifting all 0's to 1 would break String's hash definition: h = 31 * h + > val[i++]. Yes it does, any way i think spec is to tight here. Do we really need hash of each value even if String have length