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

[Vladimir Kozlov]

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