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 <[email protected]
<mailto:[email protected]>> 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 > 0) {
hash = isLatin1() ? StringLatin1.hashCode(value)
}
return hash;
}
There is a constructor:
public String(String original) {
this.value = original.value;
this.coder = original.coder;
this.hash = original.hash;
}
that might write zero to the mutable field "hash".
The object created by this constructor might be shared using
plain reads
and writes between two threads[1] and the write of 0 in the
constructor
might be interleaved with the reads and write in hashCode. Does this
capture the problem?
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 always thought that the freeze action only freezes final fields. The
hash field in String is not final and example 17.5-1 is applicable as
far as I can see
(https://docs.oracle.com/javase/specs/jls/se8/html/jls-17.html#jls-17.5). Has
the memory model changed in JDK9 to invalidate example 17.5-1 or I am
missing something about String.
Sorry - I was confusing what the spec says versus what the VM actually
does - as Vitaly pointed out.
David
Carsten
David
[1]: Meaning the is no happens-before relationship established
between
object construction and another thread calling hashCode on the
object.
Carsten
On Wed, Sep 28, 2016 at 10:13 AM, Vitaly Davidovich
<[email protected] <mailto:[email protected]>
<mailto:[email protected] <mailto:[email protected]>>> wrote:
On Wednesday, September 28, 2016, David Holmes
<[email protected] <mailto:[email protected]>
<mailto:[email protected] <mailto:[email protected]>>>
wrote:
> On 28/09/2016 10:44 PM, Peter Levart wrote:
>
>> Hi,
>>
>> According to discussion here:
>>
>> http://cs.oswego.edu/pipermail/concurrency-interest/2016-
<http://cs.oswego.edu/pipermail/concurrency-interest/2016->
<http://cs.oswego.edu/pipermail/concurrency-interest/2016-
<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 patch:
>>
>>
http://cr.openjdk.java.net/~plevart/jdk9-dev/8166842_String
<http://cr.openjdk.java.net/~plevart/jdk9-dev/8166842_String>
<http://cr.openjdk.java.net/~plevart/jdk9-dev/8166842_String
<http://cr.openjdk.java.net/~plevart/jdk9-dev/8166842_String>>.
>> hashCode/webrev.01/
>>
>
> I'm far from convinced that the bug exists - theoretical or
otherwise -
> but the "fix" is harmless.
>
> When we were working on JSR-133 one of the conceptual
models is
that every
> write to a variable goes into the set of values that a
read may
potentially
> return (so no out-of-thin-air for example). happens-before
establishes
> constraints on which value can legally be returned - the most
recent. An
> additional property was that once a value was returned, a
later
read could
> not return an earlier value - in essence once a read returns a
given value,
> all earlier written values are removed from the set of
potential
values
> that can be read.
>
> Your bug requires that the code act as-if written:
>
> int temp = hash;
> if (temp == 0) {
> hash = ...
> }
> return temp;
It's the other way I think:
int temp = hash; // read 0
if (hash == 0) // reread a non 0
hash = temp = ...
return temp // return 0
It's unlikely but what prohibits that?
>
> and I do not believe that is allowed.
>
> David
>
>
>> For the bug:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8166842
<https://bugs.openjdk.java.net/browse/JDK-8166842>
<https://bugs.openjdk.java.net/browse/JDK-8166842
<https://bugs.openjdk.java.net/browse/JDK-8166842>>
>>
>>
>>
>> JDK 8 did not have this problem, so no back-porting
necessary.
>>
>> Regards, Peter
>>
>>
--
Sent from my phone
