RFR(S): 8153267: nmethod's exception cache not multi-thread safe
vladimir.kozlov at oracle.com
Fri Apr 15 01:44:44 UTC 2016
Looks fine to me. Jamsheed, please, run our PIT testing with these changes and analyze results.
On 4/12/16 2:45 AM, Doerr, Martin wrote:
> I think we have come to a common understanding and there was no complaint about my latest webrev:
> Can I consider it reviewed?
> Can somebody sponsor, please?
> Thanks and best regards,
> -----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:
> Please review. I will also need a sponsor from Oracle, please.
> Thanks again and best regards,
> -----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.
More information about the hotspot-compiler-dev