RFR(S): 8153267: nmethod's exception cache not multi-thread safe

[Jamsheed C m]

Hi Vladimir,

PIT testing is in progress, link is available  in bug report.

Best Regards,
Jamsheed

On 4/15/2016 7:14 AM, Vladimir Kozlov wrote:
> Looks fine to me. Jamsheed, please, run our PIT testing with these 
> changes and analyze results.
>
> Thanks,
> Vladimir
>
> On 4/12/16 2:45 AM, Doerr, Martin wrote:
>> Hi,
>>
>> I think we have come to a common understanding and there was no 
>> complaint about my latest webrev:
>> http://cr.openjdk.java.net/~mdoerr/8153267_exception_cache/webrev.02/
>>
>> Can I consider it reviewed?
>> Can somebody sponsor, please?
>>
>> Thanks and best regards,
>> Martin
>>
>>
>> -----Original Message-----
>> From: hotspot-compiler-dev 
>> [mailto:hotspot-compiler-dev-bounces at openjdk.java.net] On Behalf Of 
>> Doerr, Martin
>> Sent: Donnerstag, 7. April 2016 12:52
>> To: Andrew Haley <aph at redhat.com>; Jamsheed C m 
>> <jamsheed.c.m at oracle.com>; hotspot-compiler-dev at openjdk.java.net
>> Subject: RE: RFR(S): 8153267: nmethod's exception cache not 
>> multi-thread safe
>>
>> Hi Andrew, Jamsheed and all,
>>
>> thank you very much for your input.
>>
>> As Andrew, Jamsheed and I think, it's better to have a releasing 
>> store in increment_count().
>> Therefore, I have replaced the storestore barrier introduced with 
>> JDK-8143897  (even though this barrier was also correct).
>>
>> My change still contains a releasing store for newly created 
>> ExceptionCache instances.
>> As Jamsheed has pointed out, this should not be strictly required as 
>> we have the other barrier. It may only produce additional false 
>> negatives on weak memory model platforms.
>> I think having the release doesn't hurt too much and makes the design 
>> a little cleaner.
>>
>> I also added comments based on your input.
>>
>> The new webrev is here:
>> http://cr.openjdk.java.net/~mdoerr/8153267_exception_cache/webrev.02/
>>
>> Please review. I will also need a sponsor from Oracle, please.
>>
>> Thanks again and best regards,
>> Martin
>>
>>
>> -----Original Message-----
>> From: Andrew Haley [mailto:aph at redhat.com]
>> Sent: Donnerstag, 7. April 2016 12:14
>> To: Doerr, Martin <martin.doerr at sap.com>; Jamsheed C m 
>> <jamsheed.c.m at oracle.com>; hotspot-compiler-dev at openjdk.java.net
>> Subject: Re: RFR(S): 8153267: nmethod's exception cache not 
>> multi-thread safe
>>
>> On 07/04/16 10:08, Doerr, Martin wrote:
>>
>>> atomic update for the _count would only be required if there were
>>> multiply threads which attempt to increment it
>>> concurrently. However, updates are under lock, so we only have
>>> concurrent readers which is ok.
>>>
>>> I still think "volatile" does what we need here. Especially the xlC
>>> compiler on AIX tends to reload variables from memory. Exactly this
>>> can be prevented by making the field volatile.
>>
>> I think your latest patch is OK.  Whether volatile is really good
>> enough, I don't know.  The new(ish) C++ memory model treats this as a
>> race, and therefore undefined behaviour.  Old C++ didn't have a memory
>> model, so the best we can do with racy code is guess about what our
>> compilers might do.
>>
>> I certainly much prefer a release_store to the storestore fence used
>> in the fix for 8143897.
>>
>> Andrew.
>>