RFR(S): 8050972: Concurrency problem in PcDesc cache

[Lindenmaier, Goetz]

Hi Vladimir,

I fixed that in the bug, as well as for 
8050978: Fix bad field access check in C1 and C2

Sorry for not doing that right away.

Best regards,
  Goetz.

-----Original Message-----
From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com] 
Sent: Mittwoch, 23. Juli 2014 16:29
To: Lindenmaier, Goetz; Doerr, Martin; hotspot-dev at openjdk.java.net
Subject: Re: RFR(S): 8050972: Concurrency problem in PcDesc cache

But the bug is P4!

Change it to P2 (or P1). Add 8u20 to affected versions. Add 8u20-critical-request label and add comments with 
8u20-critical-request justification: explaining why it is showstopper. See 8051378 as example.

Vladimir

On 7/23/14 2:14 AM, Lindenmaier, Goetz wrote:
> Hi Vladimir,
>
> Thanks for pushing the change!
>
> I would consider this a showstopper.
> Jvm2008/sunflow does a wrong resolve and then throws an incompatible
> class change error on AIX if the VM is built with xlc12.
> This happens on about every second try running this benchmark.
>
> Therefore we would appreciate if the fix could go to 8u20.
>
> Best regards,
>    Goetz.
>
>
> -----Original Message-----
> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
> Sent: Dienstag, 22. Juli 2014 21:33
> To: Lindenmaier, Goetz; Doerr, Martin; hotspot-dev at openjdk.java.net
> Subject: Re: RFR(S): 8050972: Concurrency problem in PcDesc cache
>
> I am pushing it into hs-comp.
>
> Unfortunately it 8u20 is closed, only showstoppers are allowed. I will
> push it into 8u40 later.
>
> Thanks,
> Vladimir
>
> On 7/22/14 2:55 AM, Lindenmaier, Goetz wrote:
>> Hi,
>>
>> could somebody please sponsor this change?  It also needs to go to jdk8u20.
>>
>> Thanks and best regards,
>>     Goetz.
>>
>> -----Original Message-----
>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>> Sent: Freitag, 18. Juli 2014 16:48
>> To: Lindenmaier, Goetz; Doerr, Martin; hotspot-dev at openjdk.java.net
>> Subject: Re: RFR(S): 8050972: Concurrency problem in PcDesc cache
>>
>> Looks good.
>>
>> Thanks,
>> Vladimir
>>
>> On 7/18/14 2:08 AM, Lindenmaier, Goetz wrote:
>>> Hi Vladimir,
>>>
>>> We fixed the comment and camel case stuff.
>>> http://cr.openjdk.java.net/~goetz/webrevs/8050972-pcDescConc/webrev-01/
>>>
>>> We think it looks better if volatile is before the type.
>>>
>>> Best regards,
>>>      Martin and Goetz.
>>>
>>> -----Original Message-----
>>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>>> Sent: Donnerstag, 17. Juli 2014 17:52
>>> To: Doerr, Martin; Lindenmaier, Goetz; hotspot-dev at openjdk.java.net
>>> Subject: Re: RFR(S): 8050972: Concurrency problem in PcDesc cache
>>>
>>> First, comments needs to be fixed:
>>>
>>> "The array elements must be volatile" but in changeset comments: "// Array MUST be volatile!"
>>>
>>> Second, type name should be camel style (PcDescPtr).
>>>
>>> Someone have to double check this volatile declaration. Your example is more clear for me than typedef.
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 7/17/14 8:19 AM, Doerr, Martin wrote:
>>>> Hi Vladimir,
>>>>
>>>> the following line should also work:
>>>> PcDesc* volatile _pc_descs[cache_size];
>>>> But we thought that the typedef would improve readability.
>>>> The array elements must be volatile, not the PcDescs which are pointed to.
>>>>
>>>> Best regards,
>>>> Martin
>>>>
>>>> -----Original Message-----
>>>> From: hotspot-dev [mailto:hotspot-dev-bounces at openjdk.java.net] On Behalf Of Vladimir Kozlov
>>>> Sent: Donnerstag, 17. Juli 2014 17:09
>>>> To: Lindenmaier, Goetz; hotspot-dev at openjdk.java.net
>>>> Subject: Re: RFR(S): 8050972: Concurrency problem in PcDesc cache
>>>>
>>>> Hi Goetz,
>>>>
>>>> What is the reason for new typedef?
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> On 7/17/14 1:54 AM, Lindenmaier, Goetz wrote:
>>>>> Hi,
>>>>>
>>>>> This webrev fixes an important concurrency issue in nmethod.
>>>>> Please review and test this change.  I please need a sponsor.
>>>>> http://cr.openjdk.java.net/~goetz/webrevs/8050972-pcDescConc/webrev-01/
>>>>>
>>>>> This should be fixed into 8u20, too.
>>>>>
>>>>> The entries of the PcDesc cache in nmethods are not declared as volatile, but they are accessed and modified by several threads concurrently. Some compilers (namely xlC 12 on AIX) duplicate some memory accesses to non-volatile fields. In this case, this has led to the situation that a thread had successfully matched a pc in the cache, but returned the reloaded value which was already overwritten by another thread.
>>>>> Best regards,
>>>>>        Martin and Goetz.
>>>>>