Re: RFR: 8166842: String.hashCode() has a non-benign data race

2016-09-30 Thread Peter Levart
Hi, Thank you all for reviews and comments, especially instructive was Hans Boehm's explanation about why JMM must allow reordering of plain reads from the same location. The fix is now pushed. Regards, Peter

Re: RFR: 8166842: String.hashCode() has a non-benign data race

2016-09-30 Thread Carsten Varming
Dear Peter, On Thu, Sep 29, 2016 at 5:31 AM, Peter Levart wrote: > I think Carsten has a point. > I think that this is enough to apply the fix, thanks. > You are welcome. I added my thoughts to the jira ticket. Carsten

Re: RFR: 8166842: String.hashCode() has a non-benign data race

2016-09-30 Thread Martin Buchholz
On Thu, Sep 29, 2016 at 2:31 AM, Peter Levart wrote: > > I think that this is enough to apply the fix, thanks. > I think general concurent software hygiene considerations are enough to commit this fix. Don't re-read fields, especially ones that are involved in a race!

Re: RFR: 8166842: String.hashCode() has a non-benign data race

2016-09-29 Thread Peter Levart
On 09/29/2016 02:58 AM, Vitaly Davidovich wrote: On Wednesday, September 28, 2016, Carsten Varming wrote: Dear David, See inline. On Wed, Sep 28, 2016 at 7:47 PM, David Holmes > wrote:

Re: RFR: 8166842: String.hashCode() has a non-benign data race

2016-09-29 Thread Andrew Haley
On 29/09/16 05:31, David Holmes wrote: > > On 29/09/2016 10:49 AM, Carsten Varming wrote: >> Because String has final fields there is a freeze action at the end >> of construction so that String instances are always safely published >> even if not "safely published". >> >> >> I

Re: RFR: 8166842: String.hashCode() has a non-benign data race

2016-09-28 Thread David Holmes
On 29/09/2016 10:49 AM, Carsten Varming wrote: Dear David, See inline. On Wed, Sep 28, 2016 at 7:47 PM, David Holmes > wrote: On 29/09/2016 3:44 AM, Carsten Varming wrote: Dear Vitaly and David, Looking at your

Re: RFR: 8166842: String.hashCode() has a non-benign data race

2016-09-28 Thread Vitaly Davidovich
On Wednesday, September 28, 2016, David Holmes wrote: > On 28/09/2016 11:24 PM, Peter Levart wrote: > >> >> On 09/28/2016 03:05 PM, David Holmes wrote: >> >>> On 28/09/2016 10:44 PM, Peter Levart wrote: >>> Hi, According to discussion here:

Re: RFR: 8166842: String.hashCode() has a non-benign data race

2016-09-28 Thread Vitaly Davidovich
On Wednesday, September 28, 2016, Carsten Varming wrote: > Dear David, > > See inline. > > On Wed, Sep 28, 2016 at 7:47 PM, David Holmes > wrote: > >> On 29/09/2016 3:44 AM, Carsten Varming

Re: RFR: 8166842: String.hashCode() has a non-benign data race

2016-09-28 Thread Carsten Varming
Dear David, See inline. On Wed, Sep 28, 2016 at 7:47 PM, David Holmes wrote: > On 29/09/2016 3:44 AM, Carsten Varming wrote: > >> Dear Vitaly and David, >> >> Looking at your webrev the original code is: >> >> public int hashCode() { >> if (hash == 0 && value.length

Re: RFR: 8166842: String.hashCode() has a non-benign data race

2016-09-28 Thread David Holmes
On 28/09/2016 11:24 PM, Peter Levart wrote: On 09/28/2016 03:05 PM, David Holmes wrote: On 28/09/2016 10:44 PM, Peter Levart wrote: Hi, According to discussion here: http://cs.oswego.edu/pipermail/concurrency-interest/2016-September/015414.html it seems compact strings introduced (at least

Re: RFR: 8166842: String.hashCode() has a non-benign data race

2016-09-28 Thread David Holmes
On 28/09/2016 11:26 PM, Aleksey Shipilev wrote: Peter, David, Would you mind discussing the theoretical topics on concurrency-interest@, for the benefits of others who don't track high-traffic OpenJDK list? Well, noone responded to my comment on c-i, and I didn't expect a patch to appear on

Re: RFR: 8166842: String.hashCode() has a non-benign data race

2016-09-28 Thread David Holmes
On 29/09/2016 3:44 AM, Carsten Varming wrote: Dear Vitaly and David, Looking at your webrev the original code is: public int hashCode() { if (hash == 0 && value.length > 0) { hash = isLatin1() ? StringLatin1.hashCode(value) } return hash; } There is a constructor: public

Re: RFR: 8166842: String.hashCode() has a non-benign data race

2016-09-28 Thread Vitaly Davidovich
On Wednesday, September 28, 2016, Carsten Varming wrote: > Dear Vitaly and David, > > Looking at your webrev the original code is: > > public int hashCode() { > if (hash == 0 && value.length > 0) { > hash = isLatin1() ? StringLatin1.hashCode(value) > } > return

Re: RFR: 8166842: String.hashCode() has a non-benign data race

2016-09-28 Thread Carsten Varming
Dear Vitaly and David, Looking at your webrev the original code is: public int hashCode() { if (hash == 0 && value.length > 0) { hash = isLatin1() ? StringLatin1.hashCode(value) } return hash; } There is a constructor: public String(String original) { this.value = original.value;

Re: RFR: 8166842: String.hashCode() has a non-benign data race

2016-09-28 Thread Vitaly Davidovich
On Wednesday, September 28, 2016, David Holmes wrote: > On 28/09/2016 10:44 PM, Peter Levart wrote: > >> Hi, >> >> According to discussion here: >> >> http://cs.oswego.edu/pipermail/concurrency-interest/2016- >> September/015414.html >> >> >> it seems compact strings

Re: RFR: 8166842: String.hashCode() has a non-benign data race

2016-09-28 Thread Aleksey Shipilev
Peter, David, Would you mind discussing the theoretical topics on concurrency-interest@, for the benefits of others who don't track high-traffic OpenJDK list? Thanks, -Aleksey On 09/28/2016 03:24 PM, Peter Levart wrote: > > On 09/28/2016 03:05 PM, David Holmes wrote: >> On 28/09/2016 10:44 PM,

Re: RFR: 8166842: String.hashCode() has a non-benign data race

2016-09-28 Thread Claes Redestad
Hi, tangentially, is there any reason the Compact Strings work chose to revert to checking value.length > 0 rather than not writing to hash if the calculated hash code is 0? http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/6837759aa403 /Claes On 2016-09-28 14:47, Aleksey Shipilev wrote: On

Re: RFR: 8166842: String.hashCode() has a non-benign data race

2016-09-28 Thread Peter Levart
On 09/28/2016 03:05 PM, David Holmes wrote: On 28/09/2016 10:44 PM, Peter Levart wrote: Hi, According to discussion here: http://cs.oswego.edu/pipermail/concurrency-interest/2016-September/015414.html it seems compact strings introduced (at least theoretical) non-benign data race into

Re: RFR: 8166842: String.hashCode() has a non-benign data race

2016-09-28 Thread Alan Bateman
On 28/09/2016 13:44, Peter Levart wrote: Hi, According to discussion here: http://cs.oswego.edu/pipermail/concurrency-interest/2016-September/015414.html it seems compact strings introduced (at least theoretical) non-benign data race into String.hasCode() method. Here is a proposed

Re: RFR: 8166842: String.hashCode() has a non-benign data race

2016-09-28 Thread David Holmes
On 28/09/2016 10:44 PM, Peter Levart wrote: Hi, According to discussion here: http://cs.oswego.edu/pipermail/concurrency-interest/2016-September/015414.html it seems compact strings introduced (at least theoretical) non-benign data race into String.hasCode() method. Here is a proposed

Re: RFR: 8166842: String.hashCode() has a non-benign data race

2016-09-28 Thread Aleksey Shipilev
On 09/28/2016 02:44 PM, Peter Levart wrote: > http://cr.openjdk.java.net/~plevart/jdk9-dev/8166842_String.hashCode/webrev.01/ +1, thanks. This is partially my fault for not spotting it earlier during the Compact Strings work. -Aleksey